Loader rewrite

[popematt]

Issue #, if available:

None. However, an offline discussion with someone who was trying out IonElement indicated that loading all values as IonElement was allocating up to 4x as memory as compared to loading the equivalent data as IonValue.

Description of changes:

This rewrites the loadCurrentElement function to eliminate some unnecessary allocations, and minimize the amount of branching in this function. The result is a very modest memory improvement, but a fairly decent speed improvement, and the size of the compiled bytecode of the rewritten function is about 40% of that of the current version of the function.

Performance testing results

Using arbitrary data that I put together from a few different sources (~5kB)

API	Time (ms/op)	gc.alloc.rate.norm (B/op)
IonValue	0.053 ± 0.001	81128.034 ± 0.004
IonElement 1.2.0	0.047 ± 0.001	215416.030 ± 0.004
ion-element-kotlin#100	0.032 ± 0.001	131720.020 ± 0.003

Ion binary application log file (~20 MB)

API	Time (ms/op)	gc.alloc.rate.norm (B/op)
IonValue	635.450 ± 75.384	625408257.218 ± 49.211
IonElement 1.2.0	630.640 ± 215.099	2348935671.047 ± 146.942
ion-element-kotlin#100	452.451 ± 283.069	2245469209.217 ± 113.536

Sample catalog data (~40 kB)

API	Time (ms/op)	gc.alloc.rate.norm (B/op)
IonValue	0.616 ± 0.093	696584.433 ± 0.297
IonElement 1.2.0	0.559 ± 0.028	2999688.359 ± 0.046
ion-element-kotlin#100	0.513 ± 0.007	2854072.329 ± 0.041

This was not as much of an improvement as I had hoped, but I will be following up this PR with another change that is orthogonal to this that should have a more significant impact on the memory allocation rate.

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

[jobarr-amzn]

Is the win here predominantly from branching reduction? Can you say anything else about performance in theory or practice of the pieces here?

[rmarrowstone]

Sweet. Nice win!

My only specific question is if making the IonReader.forEachValue method inline with a crossinline block param would help be the same win here and wherever else it was used.

[popematt]

Is the win here predominantly from branching reduction? Can you say anything else about performance in theory or practice of the pieces here?

It's not practical to try to attribute specific amounts of improvement to specific parts of the change (I'd have to test the individually, and hope that the sum of the parts is equal to the whole, which is not guaranteed on the JVM). However, here is a list of the things that I did which, in theory, should all contribute to the improvement:

• I got rid of forEachValue function calls (and the function itself) in favor of just having a while loop at the former call sites.
• The old version would create an IonElement instance without annotations, and then if there were annotations, it would create a new copy of that value with the annotations added. I changed it so that the values are constructed with the annotations. I think this accounts for some of the reduced memory allocations, especially in the first dataset which had a higher proportion of annotated values than the other datasets. This also has the effect of entirely eliminating the branching that checked whether the annotations were non-empty and needed to be added to the value.
• I made the same changes for "metas" as I did for annotations, but I did not use any "metas" in my perf testing. In theory, the "metas" change should be even more impactful than the annotations change because if you are populating location metadata, every value has that data whereas not every value has annotations.
• I replaced the use of the factory functions (e.g. ionInt()) with the internal-only constructors. This removes 1-2 levels of indirection when creating a new IonElement instance.
• By changing the APIs that are used to construct the IonElement instances and adding the annotations in the initial construction, I was able to minimize the amount of defensive copying of collections. It is safe to do that in this case because the lists that we construct in loadCurrentElement are all local and cannot be leaked elsewhere.
• The main logic of the function had 3 levels branching. (Is it a datagram or null? Is it a container? What is the IonType?) All of those were flattened into one when expression. It is worth mentioning that a when expression that contains arbitrary expressions (such as ionReader.isNullValue on L114 of the old side of the diff) will be compiled as a chain of if/else if/else statements. However, a when (value) for an enum (or integer) can be compiled using the tableswitch instruction, which is O(1) to evaluate, so we're not just reducing the number of times we have to make a branching decision, we're also using a more efficient form of branching.

My only specific question is if making the IonReader.forEachValue method inline with a crossinline block param would help be the same win here and wherever else it was used.

I don't know because I did not test that. Since it was private to this file, I just got rid of the function entirely.