Adds an IntArray pool to MacroEvaluator.Session and modifies Session's ExpansionInfo pool to match the pattern. #1038

tgregg · 2025-01-19T02:50:16Z

Description of changes:
The main idea here is that Session manages all pools relevant to macro evaluation. This works well because Session instances are already reused, so we can just reset its pools in bulk in Session.reset(), rather than worrying about releasing individual items back into their pools. I modified the existing ExpansionInfo pool to match this pattern, as well as adding a new IntArray pool for the array used to track argument end indices. This reduced allocation rate.

Speed: 247 ms/op -> 245 ms /op (-<1%)
Allocation rate: 175 KB/op -> 163 KB/op (-6.9%)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

jobarr-amzn

With this we have by my count 6 pool use cases in ion-java.

Using Pool/Poolable:

utf8 string encoder pool
utf8 string decoder pool
bytebuffer pool

Not using Pool/Poolable:

Block allocator pool
expression pool
expander pool (this)

When do we use Pooled/Poolable, and when not?

jobarr-amzn · 2025-01-21T17:16:37Z

src/main/java/com/amazon/ion/impl/macro/MacroEvaluator.kt

+        /**
+         * Gets an [IntArray] from the pool, or allocates a new one if necessary. The returned array is valid until
+         * [reset] is called.
+         * */
+        fun intArrayForSize(size: Int): IntArray {
+            if (size >= intArrayPools.size) {
+                // Don't attempt to pool arbitrarily large arrays.
+                return IntArray(size)
+            }


Arrays smaller than intArrayPools.size are zeroed out when reset() is called, but larger arrays are not. I suppose one could say that the larger vended arrays are still only valid until reset is called?

reset() doesn't zero any of the arrays in the pool, just the indices that point to the next available array in the pool for a given size. Arrays returned from this method are zeroed if freshly allocated, but not otherwise. I'll add a note that the returned array will not necessarily be zeroed.

popematt · 2025-01-21T22:07:13Z

src/main/java/com/amazon/ion/impl/macro/MacroEvaluator.kt

@@ -44,19 +44,35 @@ class MacroEvaluator {
     * This state pertains to a single "session" of the macro evaluator, and is reset every time [initExpansion] is called.
     * For now, this includes managing the pool of [ExpansionInfo] and tracking the expansion step limit.
     */
-    private class Session(


If this can't be private, can it at least be internal?

Yes; I should not have assumed IntelliJ's quick fix would suggest the most restrictive possible access modifier.

popematt · 2025-01-21T22:07:35Z

src/main/java/com/amazon/ion/impl/macro/MacroEvaluator.kt

@@ -116,7 +151,7 @@ class MacroEvaluator {
     */
    // TODO(PERF): It might be possible to optimize this by changing it to an enum without any methods (or even a set of
    //             integer constants) and converting all their implementations to static methods.
-    private enum class ExpansionKind {
+    enum class ExpansionKind {


If not private, then internal?

popematt · 2025-01-21T22:08:02Z

src/main/java/com/amazon/ion/impl/macro/MacroEvaluator.kt

@@ -692,7 +727,7 @@ class MacroEvaluator {
     *       Alternately, consider ExpansionOperator to reflect the fact that these are
     *       like operators in an expression tree.
     */
-    private class ExpansionInfo(@JvmField val session: Session) {
+    class ExpansionInfo(@JvmField val session: Session) {


Should be internal.

popematt · 2025-01-21T22:08:15Z

src/main/java/com/amazon/ion/util/Assumptions.kt

@@ -51,4 +51,4 @@ internal inline fun confirm(assumption: Boolean, lazyMessage: () -> String) {
 /**
 * Marks a branch as unreachable (for human readability).
 */
-internal fun unreachable(reason: String? = null): Nothing = throw IllegalStateException(reason)
+fun unreachable(reason: String? = null): Nothing = throw IllegalStateException(reason)


This should stay internal.

tgregg · 2025-01-22T00:05:23Z

With this we have by my count 6 pool use cases in ion-java.

Using Pool/Poolable:
* utf8 string encoder pool

* utf8 string decoder pool

* bytebuffer pool
Not using Pool/Poolable:
* Block allocator pool

* expression pool

* expander pool (this)
When do we use Pooled/Poolable, and when not?

Pool/Poolable is a fairly heavy-weight abstraction that is good when the resources are fairly large, infrequently accessed, and accessed/freed individually. The BlockAllocator could be migrated to the Pool/Poolable abstraction if we felt like spending the time to do so.

For expressions and expanders, we need to access chunks of many at a time, and they can all be freed at once. Pooled/Poolable is heavy for this use case.

…s ExpansionInfo pool to match the pattern.

tgregg requested a review from popematt January 19, 2025 02:50

jobarr-amzn reviewed Jan 21, 2025

View reviewed changes

popematt reviewed Jan 21, 2025

View reviewed changes

tgregg force-pushed the ion-11-encoding-optimize-initial-expression-array-size-session-pools branch from 8161ef7 to 3c029c3 Compare January 22, 2025 01:31

jobarr-amzn approved these changes Jan 22, 2025

View reviewed changes

tgregg changed the base branch from ion-11-encoding-optimize-initial-expression-array-size-merge to ion-11-encoding January 28, 2025 00:12

tgregg changed the base branch from ion-11-encoding to ion-11-encoding-pooled-expressions January 28, 2025 00:17

Adds an IntArray pool to MacroEvaluator.Session and modifies Session'…

4d109cb

…s ExpansionInfo pool to match the pattern.

tgregg force-pushed the ion-11-encoding-optimize-initial-expression-array-size-session-pools branch from 3c029c3 to 4d109cb Compare January 28, 2025 00:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds an IntArray pool to MacroEvaluator.Session and modifies Session's ExpansionInfo pool to match the pattern. #1038

Adds an IntArray pool to MacroEvaluator.Session and modifies Session's ExpansionInfo pool to match the pattern. #1038

tgregg commented Jan 19, 2025

jobarr-amzn left a comment

jobarr-amzn Jan 21, 2025

tgregg Jan 22, 2025

popematt Jan 21, 2025

tgregg Jan 22, 2025

popematt Jan 21, 2025

popematt Jan 21, 2025

popematt Jan 21, 2025

tgregg commented Jan 22, 2025

Adds an IntArray pool to MacroEvaluator.Session and modifies Session's ExpansionInfo pool to match the pattern. #1038

Are you sure you want to change the base?

Adds an IntArray pool to MacroEvaluator.Session and modifies Session's ExpansionInfo pool to match the pattern. #1038

Conversation

tgregg commented Jan 19, 2025

jobarr-amzn left a comment

Choose a reason for hiding this comment

jobarr-amzn Jan 21, 2025

Choose a reason for hiding this comment

tgregg Jan 22, 2025

Choose a reason for hiding this comment

popematt Jan 21, 2025

Choose a reason for hiding this comment

tgregg Jan 22, 2025

Choose a reason for hiding this comment

popematt Jan 21, 2025

Choose a reason for hiding this comment

popematt Jan 21, 2025

Choose a reason for hiding this comment

popematt Jan 21, 2025

Choose a reason for hiding this comment

tgregg commented Jan 22, 2025