Repository: amzn/ion-java
Pull request #363 (2021-05-24 15:59:08)

Makes the PooledBlockAllocatorProvider a singleton
### Description

Many applications need to initialize IonWriters repeatedly for
short-lived tasks. For example, a webserver might create a new
IonWriter for each HTTP response that it needs to write. In such
cases, the bulk of the compute time needed to serialize the
response data is often spent initializing the byte array buffer
backing the IonWriter rather than performing the write logic itself.

Each time that a (e.g.) binary IonWriter is initialized, a new
PooledBlockAllocatorProvider (PBAP) is initialized with it to dole
out new byte array buffers. It attempts to reuse these byte arrays
wherever possible during its lifespan. Unfortunately, the PBAP only
lives as long as the IonWriter itself. If a new IonWriter is created,
a new PBAP (and therefore new byte arrays) are created as well.

This change modifies the PooledBlockAllocatorProvider (which was
already thread-safe) to be a globally shared singleton. This makes
repeatedly initializing new IonWriters substantially cheaper.

### Benchmarks

I wrote [a simple, unscientific benchmark](https://gist.github.com/zslayton/45194bfab526703e89e655a2eb8a7232) that creates a binary `IonWriter`, writes a value to a `ByteArrayOutputStream`, and then closes the `IonWriter` in a loop. The results below are the median of 5 runs each.

| Before | After | Difference |
|--|--|--|
| 1046 ms | 399 ms | **-61.85%** |

### Testing

This PR passes our current test suite but does not include new tests. I'm open to ideas for how to demonstrate that this won't cause problems.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.


codecov[bot]: # [Codecov](https://codecov.io/gh/amzn/ion-java/pull/363?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) Report
> Merging [#363](https://codecov.io/gh/amzn/ion-java/pull/363?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (041c327) into [master](https://codecov.io/gh/amzn/ion-java/commit/45234885de653eae67db4d8d3ccb6bb85bb82fd9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (4523488) will **increase** coverage by `0.02%`.
> The diff coverage is `80.00%`.

[![Impacted file tree graph](https://codecov.io/gh/amzn/ion-java/pull/363/graphs/tree.svg?width=650&height=150&src=pr&token=M93Y3e3kKn&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)](https://codecov.io/gh/amzn/ion-java/pull/363?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

```diff
@@             Coverage Diff              @@
##             master     #363      +/-   ##
============================================
+ Coverage     64.00%   64.02%   +0.02%     
- Complexity     4830     4835       +5     
============================================
  Files           136      136              
  Lines         21095    21098       +3     
  Branches       3815     3815              
============================================
+ Hits          13502    13509       +7     
+ Misses         6257     6256       -1     
+ Partials       1336     1333       -3     
```


| [Impacted Files](https://codecov.io/gh/amzn/ion-java/pull/363?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...mazon/ion/impl/bin/\_PrivateIon\_HashTrampoline.java](https://codecov.io/gh/amzn/ion-java/pull/363/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2NvbS9hbWF6b24vaW9uL2ltcGwvYmluL19Qcml2YXRlSW9uX0hhc2hUcmFtcG9saW5lLmphdmE=) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (ø)` | |
| [...zon/ion/impl/bin/PooledBlockAllocatorProvider.java](https://codecov.io/gh/amzn/ion-java/pull/363/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2NvbS9hbWF6b24vaW9uL2ltcGwvYmluL1Bvb2xlZEJsb2NrQWxsb2NhdG9yUHJvdmlkZXIuamF2YQ==) | `83.87% <100.00%> (+1.72%)` | `5.00 <2.00> (+2.00)` | |
| [...pl/bin/\_Private\_IonManagedBinaryWriterBuilder.java](https://codecov.io/gh/amzn/ion-java/pull/363/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2NvbS9hbWF6b24vaW9uL2ltcGwvYmluL19Qcml2YXRlX0lvbk1hbmFnZWRCaW5hcnlXcml0ZXJCdWlsZGVyLmphdmE=) | `70.73% <100.00%> (ø)` | `23.00 <0.00> (ø)` | |
| [src/com/amazon/ion/impl/BlockedBuffer.java](https://codecov.io/gh/amzn/ion-java/pull/363/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2NvbS9hbWF6b24vaW9uL2ltcGwvQmxvY2tlZEJ1ZmZlci5qYXZh) | `51.21% <0.00%> (+0.48%)` | `80.00% <0.00%> (+3.00%)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/amzn/ion-java/pull/363?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/amzn/ion-java/pull/363?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Last update [4523488...041c327](https://codecov.io/gh/amzn/ion-java/pull/363?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).

almann: > I wrote [a simple, unscientific benchmark](https://gist.github.com/zslayton/45194bfab526703e89e655a2eb8a7232) that creates a binary `IonWriter`, writes a value to a `ByteArrayOutputStream`, and then closes the `IonWriter` in a loop. The results below are the median of 5 runs each.

For a change like this, it might be worth writing a proper [JMH benchmark][1] to make sure we're not fooling ourselves.

[1]: https://github.com/openjdk/jmh
almann: > The provider should construct instances of the allocator and users should use the allocator, this changes the contract a bit--in other words do we have a problem in downstream code not creating this once using it everywhere?

Here is what the API documentation says for `BlockAllocatorProvider`:

https://github.com/amzn/ion-java/blob/45234885de653eae67db4d8d3ccb6bb85bb82fd9/src/com/amazon/ion/impl/bin/BlockAllocatorProvider.java#L18-L24

Doesn't this change violate the contract?
zslayton: > > I wrote [a simple, unscientific benchmark](https://gist.github.com/zslayton/45194bfab526703e89e655a2eb8a7232) that creates a binary `IonWriter`, writes a value to a `ByteArrayOutputStream`, and then closes the `IonWriter` in a loop. The results below are the median of 5 runs each.
> 
> For a change like this, it might be worth writing a proper [JMH benchmark](https://github.com/openjdk/jmh) to make sure we're not fooling ourselves.

I can definitely do that. 👍 I wanted to see if folks had any objections to the PR in principle before I invested the time needed to write a full on benchmark.
tgregg: I recommend using the [ion-java-benchmark-cli](https://github.com/amzn/ion-java-benchmark-cli) instead of writing a bespoke JMH benchmark. It's perfect for stuff like this.
tgregg: While we're in here, we should consider declaring the `PooledBlockAllocator` inner class `static`.
almann: > > The provider should construct instances of the allocator and users should use the allocator, this changes the contract a bit--in other words do we have a problem in downstream code not creating this once using it everywhere?
> 
> Here is what the API documentation says for `BlockAllocatorProvider`:
> 
> https://github.com/amzn/ion-java/blob/45234885de653eae67db4d8d3ccb6bb85bb82fd9/src/com/amazon/ion/impl/bin/BlockAllocatorProvider.java#L18-L24

Re-reading this a bunch, I think there is no technical violation here.  The *Provider* itself is the thread-safe thing, if we want to make the it a global singleton, I see no contract that would be violated by that.

Also, all of this is internal, so we should be okay here, but just surveying the usage and implications is a good thing.
zslayton: > I recommend using the ion-java-benchmark-cli instead of writing a bespoke JMH benchmark. It's perfect for stuff like this.

🤦 I should've used this, but I've already got the JMH code. Next time!

> While we're in here, we should consider declaring the PooledBlockAllocator inner class static.

Done.

zslayton: ### JMH Benchmark results

Source can be found [here](https://gist.github.com/zslayton/849bf528c9ad4389c098099afc798a14).

**Before**
```
Benchmark                                 Score           Error   Units
time                                   1630.774 ±       135.671   ms/op
gc.alloc.rate                          7820.193 ±       492.074  MB/sec
gc.alloc.rate.norm              17549795353.600 ±    152386.604    B/op
gc.churn.G1_Eden_Space                 7652.005 ±       518.475  MB/sec
gc.churn.G1_Eden_Space.norm     17173158297.600 ± 841670600.565    B/op
gc.churn.G1_Old_Gen                       0.213 ±         0.307  MB/sec
gc.churn.G1_Old_Gen.norm             478707.200 ±    697129.834    B/op
gc.churn.G1_Survivor_Space                0.934 ±         0.059  MB/sec
gc.churn.G1_Survivor_Space.norm     2097152.000 ±         0.001    B/op
gc.count                                166.000                  counts
gc.time                                 299.000                      ms
```
**After**
```
Benchmark                                 Score           Error   Units
time                                    844.091 ±       124.800   ms/op
gc.alloc.rate                          3098.544 ±       299.204  MB/sec
gc.alloc.rate.norm               4390142753.600 ±     93107.441    B/op
gc.churn.G1_Eden_Space                 2833.914 ±       884.221  MB/sec
gc.churn.G1_Eden_Space.norm      4011432345.600 ± 913155271.731    B/op
gc.churn.G1_Old_Gen                       0.064 ±         0.120  MB/sec
gc.churn.G1_Old_Gen.norm              90323.200 ±    161209.039    B/op
gc.churn.G1_Survivor_Space                1.480 ±         0.143  MB/sec
gc.churn.G1_Survivor_Space.norm     2097152.000 ±         0.001    B/op
gc.count                                 87.000                  counts
gc.time                                 197.000                      ms
```

Patch:
diff --git a/src/com/amazon/ion/impl/bin/PooledBlockAllocatorProvider.java b/src/com/amazon/ion/impl/bin/PooledBlockAllocatorProvider.java
index cbd30ae7..b3d58e2c 100644
--- a/src/com/amazon/ion/impl/bin/PooledBlockAllocatorProvider.java
+++ b/src/com/amazon/ion/impl/bin/PooledBlockAllocatorProvider.java
@@ -20,8 +20,9 @@ import java.util.concurrent.ConcurrentLinkedQueue;
 import java.util.concurrent.ConcurrentMap;
 
 /**
- * A simple pooling implementation of {@link BlockAllocatorProvider} with a global thread-safe free block list
+ * A singleton implementation of {@link BlockAllocatorProvider} offering a thread-safe free block list
  * for each block size.
+ *
  * <p>
  * This implementation is thread-safe.
  */
@@ -32,7 +33,7 @@ import java.util.concurrent.ConcurrentMap;
      * <p>
      * This implementation is thread-safe.
      */
-    private final class PooledBlockAllocator extends BlockAllocator
+    private static final class PooledBlockAllocator extends BlockAllocator
     {
         private final int blockSize, blockLimit;
         private final ConcurrentLinkedQueue<Block> freeBlocks;
@@ -76,13 +77,21 @@ import java.util.concurrent.ConcurrentMap;
         public void close() {}
     }
 
+    // A globally shared instance of the PooledBlockAllocatorProvider.
+    // This instance allows BlockAllocators to be re-used across instantiations of classes like
+    // the binary Ion writer, thereby avoiding costly array initializations.
+    private static final PooledBlockAllocatorProvider INSTANCE = new PooledBlockAllocatorProvider();
     private final ConcurrentMap<Integer, BlockAllocator> allocators;
 
-    public PooledBlockAllocatorProvider()
+    private PooledBlockAllocatorProvider()
     {
         allocators = new ConcurrentHashMap<Integer, BlockAllocator>();
     }
 
+    public static PooledBlockAllocatorProvider getInstance() {
+        return INSTANCE;
+    }
+
     @Override
     public BlockAllocator vendAllocator(final int blockSize)
     {
diff --git a/src/com/amazon/ion/impl/bin/_PrivateIon_HashTrampoline.java b/src/com/amazon/ion/impl/bin/_PrivateIon_HashTrampoline.java
index aea1e31a..6d3537c1 100644
--- a/src/com/amazon/ion/impl/bin/_PrivateIon_HashTrampoline.java
+++ b/src/com/amazon/ion/impl/bin/_PrivateIon_HashTrampoline.java
@@ -28,7 +28,7 @@ import java.io.IOException;
 @Deprecated
 public class _PrivateIon_HashTrampoline
 {
-    private static final PooledBlockAllocatorProvider ALLOCATOR_PROVIDER = new PooledBlockAllocatorProvider();
+    private static final PooledBlockAllocatorProvider ALLOCATOR_PROVIDER = PooledBlockAllocatorProvider.getInstance();
     
     public static IonWriter newIonWriter(ByteArrayOutputStream baos) throws IOException
     {
diff --git a/src/com/amazon/ion/impl/bin/_Private_IonManagedBinaryWriterBuilder.java b/src/com/amazon/ion/impl/bin/_Private_IonManagedBinaryWriterBuilder.java
index b439d160..35d87e11 100644
--- a/src/com/amazon/ion/impl/bin/_Private_IonManagedBinaryWriterBuilder.java
+++ b/src/com/amazon/ion/impl/bin/_Private_IonManagedBinaryWriterBuilder.java
@@ -52,7 +52,7 @@ public final class _Private_IonManagedBinaryWriterBuilder
             @Override
             BlockAllocatorProvider createAllocatorProvider()
             {
-                return new PooledBlockAllocatorProvider();
+                return PooledBlockAllocatorProvider.getInstance();
             }
         },
         BASIC


Test patch:
diff --git a/test/com/amazon/ion/impl/bin/PooledBlockAllocatorProviderTest.java b/test/com/amazon/ion/impl/bin/PooledBlockAllocatorProviderTest.java
index 97ccef55..807f8d14 100644
--- a/test/com/amazon/ion/impl/bin/PooledBlockAllocatorProviderTest.java
+++ b/test/com/amazon/ion/impl/bin/PooledBlockAllocatorProviderTest.java
@@ -30,7 +30,7 @@ public class PooledBlockAllocatorProviderTest
     @Before
     public void setup()
     {
-        provider = new PooledBlockAllocatorProvider();
+        provider = PooledBlockAllocatorProvider.getInstance();
     }
 
     @After