Replaces persistent collections with immutable collections

[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 replaces PersistentList and PersistentMap with custom ImmutableList and ImmutableMap implementations. These implementations are much simpler than PersistentList and PersistentMap—they can wrap any existing List/Map with very little overhead. I chose to do this because a lightweight wrapper will require less allocation than having to copy the data to a completely new instance of a List or Map. I think my implementations are pretty much the most minimal implementation one could make for an immutable wrapper of List or Map.

I've also added an ArrayBackedImmutableList so that we don't need to allocate to convert the String[] of annotations into a List before wrapping it to make an ImmutableList.

Finally, I've added an IonLocationBackedImmutableMap—a special-purpose map implementation that can only contain one key-value pair, and the key can only be $ion_location. This saves a huge amount of allocation and significantly speeds up reading with location metadata to the point where reading IonElement with the additional overhead of location data is faster than reading IonValue (which has no location metadata).

Compatibility

The public API only exposes List and Map. PersistentList and PersistentMap have never been part of the API, so it is allowable to replace them with a different implementation.

However, Java users can still call mutating methods on a Kotlin List, so I did some manual testing to confirm that the behavior has not changed in that case. Both PersistentList and my ImmutableList implementations will throw UnsupportedOperationException if a method such as add() is called. The PersistentMap and my ImmutableMap implementations are the same.

Performance Data

Composition of some arbitrary data (~5kb)

API	Time (ms/op)	gc.alloc.rate.norm (B/op)
IonValue	0.053 ± 0.001	81128.034 ± 0.004
[email protected]	0.047 ± 0.001	215416.030 ± 0.004
#100	0.032 ± 0.001	131720.020 ± 0.003
#101 (this PR)	0.024 ± 0.001	51952.015 ± 0.002

Sample binary Ion application logs (~20MB)

API	Time (ms/op)	gc.alloc.rate.norm (B/op)
IonValue	635.450 ± 75.384	625408257.218 ± 49.211
[email protected]	630.640 ± 215.099	2348935671.047 ± 146.942
#100	452.451 ± 283.069	2245469209.217 ± 113.536
#101 (this PR)	312.335 ± 24.048	446810105.670 ± 14.961

Sample product catalog data (~40kb)

API	Time (ms/op)	gc.alloc.rate.norm (B/op)
IonValue	0.616 ± 0.093	696584.433 ± 0.297
[email protected]	0.559 ± 0.028	2999688.359 ± 0.046
#100	0.513 ± 0.007	2854072.329 ± 0.041
#101 (this PR)	0.286 ± 0.016	631800.182 ± 0.021

For two of the samples, this PR has a normalized allocation rate that is about 1/4th of the current rate. For the one sample that displayed less of an improvement, it might be because it does not have as many lists/structs as the other data or it could be that it has a higher incidence of annotations. Either way, I'm not too concerned. Cumulatively, this change and #100 have improved the normalized memory allocation rate for all these samples from being ~4x more than IonValue to being less than IonValue (and in some cases, significantly less).

Performance Data (with IonLocation metadata)

(*) indicates performance without location because location data is not supported by that API

Composition of some arbitrary data (~5kb)

API	Time (ms/op)	gc.alloc.rate.norm (B/op)
IonValue	(*) 0.053 ± 0.001	(*) 81128.034 ± 0.004
[email protected]	0.129 ± 0.003	714456.083 ± 0.011
#100	0.076 ± 0.001	368472.048 ± 0.006
#101 (this PR)	0.033 ± 0.001	113096.021 ± 0.002

Sample binary Ion application logs (~20MB)

API	Time (ms/op)	gc.alloc.rate.norm (B/op)
IonValue	(*) 635.450 ± 75.384	(*) 625408257.218 ± 49.211
[email protected]	2492.157 ± 460.771	8622042546.800 ± 685.087
#100	1296.107 ± 182.448	3979394547.314 ± 175.082
#101 (this PR)	453.957 ± 62.876	679669196.884 ± 42.721

Sample product catalog data (~40kb)

API	Time (ms/op)	gc.alloc.rate.norm (B/op)
IonValue	(*) 0.616 ± 0.093	(*) 696584.433 ± 0.297
[email protected]	1.806 ± 0.055	10717049.154 ± 0.136
#100	0.905 ± 0.025	4688840.633 ± 0.452
#101 (this PR)	0.326 ± 0.006	1026104.209 ± 0.027

This PR brings a ~75% decrease in the allocation rate and a ~60% decrease in time per op compared to the previous commit, and cumulatively, the changes from 1.2.0 to this PR, provide up to a 90% decrease in the allocation rate, and up to a 80% decrease in time per op.

When compared to no location metadata, there is still a performance penalty associated with populating the location metadata, but while it was once 150-300% more time per op, it is now only 10-50% more time per op.

Populating the location metadata obviously requires more memory allocation than not doing it, so including IonValue here is not necessarily fair, but it is worth noting that for all of the sample data, the time per op is now ~40% less than IonValue—even with the location metadata.

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

[popematt]

This change looks large, but ~18 of the modified files are just changing Persistent* to Immutable*.

I recommend starting with ImmutableList.kt and continuing down to ImmutableMap.kt and the associated tests for those two files. Then, take a look at IonElementLoaderImpl, and then look at the rest in whatever order you like.

[popematt]

🗺️ This is partly a stylistic change. The toMetaContainer() wraps the IonLocation in an ImmutableMap implementation, which is the main performance improvement for the location metadata. The rest of this change is stylistic to try to make it more readable and concise.

[popematt]

🗺️ Previously, this line was creating a list from an array, and then creating a persistent list by copying all of the elements from the intermediate list (an O(n) operation). Now, it just wraps the array with an ArrayBackedImmutableList (an O(1) operation) in addition to ArrayBackedImmutableList having a smaller object layout to begin with.

[popematt]

🗺️ I wanted to create only a minimal set of extension functions, so I didn't create Iterable<K, V>.toImmutableMap(). Instead, I just use the equivalent .toMap().toImmutableMapUnsafe() here.

[jobarr-amzn]

Suggested change

	* We cannot use `Map.of(...)` because those were introduced in JDK 9, and we still
	* support JDK 8.
	* We cannot use `Map.of(...)` because those were introduced in JDK 9, and we still
	* support JDK 8.
	* See: https://github.com/amazon-ion/ion-element-kotlin/issues/102

[jobarr-amzn]

Suggested change

	* We cannot use `List.of(...)` because those were introduced in JDK 9, and we still
	* support JDK 8.
	* We cannot use `List.of(...)` because those were introduced in JDK 9, and we still
	* support JDK 8.
	* See: https://github.com/amazon-ion/ion-element-kotlin/issues/102

[popematt]

I've had a few offline questions that can be roughly summarized as "Why are the persistent collections so bad for performance?" I don't think persistent collections are bad, per se, but they might not be the right choice here.

1. Persistent collections make a tradeoff. They have a larger initial overhead in order that they might amortize the O(n) space for the elements when you start performing copy-on-write operations with that collection. Larger initialization cost means more costly to load IonElements.
2. Just like any other immutable collection library, kotlinx.collections.immutable has to treat all input as untrusted input (i.e. make defensive copies of data) if it is going to correctly implement its immutability guarantees. In this PR, I am taking advantage of the fact that some of the underlying data, while mutable, cannot be leaked into a context where it would be mutated. This allows us to completely eliminate all defensive copies in IonElementLoaderImpl.

[popematt]

Well, I just looked again, and kotlinx.collections.immutables.ImmutableList, etc. are open interfaces and there are adapter classes provided (that do not perform defensive copies). I checked the performance, and it is essentially the same as what I've got, so there's a decision to be made.

Is it more valuable to minimize dependencies on other libraries or to use the interface of another library?

[jobarr-amzn]

If we can use kotlinx.collections.immutables.ImmutableList, etc. then I'd rather do that. Not because we don't have a perfectly good immutable list implementation in this PR, but because I'd rather own less code.

Do we still need or benefit from our own specialized Ion location map implementation?

[popematt]

If we can use kotlinx.collections.immutables.ImmutableList, etc. then I'd rather do that. Not because we don't have a perfectly good immutable list implementation in this PR, but because I'd rather own less code.

Do we still need or benefit from our own specialized Ion location map implementation?

We still need the specialized Ion location map. It accounts for the majority of the performance improvement in the cases when we're filling location metadata.

[jobarr-amzn]

If we can use kotlinx.collections.immutables.ImmutableList, etc. then I'd rather do that. Not because we don't have a perfectly good immutable list implementation in this PR, but because I'd rather own less code.
Do we still need or benefit from our own specialized Ion location map implementation?

We still need the specialized Ion location map. It accounts for the majority of the performance improvement in the cases when we're filling location metadata.

That's what I thought. So if we use kotlinx.collections.immutables then we're down to owning only that?

I'm finding documentation on kotlinx.collections.immutables a little thin on the ground. The kotlinx immutables library proposal Use Cases include:

• Avoiding defensive copying of collection passed as a parameter when you need an unchangeable snapshot and you're unsure whether the collection could be changed later.

While the [API details] section includes:

Immutable collections just extend the read-only collections. The immutability is enforced solely by their contract: the implementors are required to keep the collection elements unchanged after the collection is constructed.

Then the implication is that toImmutableList/Set/Map don't perform defensive copies as you said.

Checking the implementation it looks like this behavior comes from creating an empty immutable thing and then adding the other elements to it while still using the storage of source collection, as described in the README + and - operators. Is that right?

[popematt]

That's what I thought. So if we use kotlinx.collections.immutables then we're down to owning only that?

We can eliminate the interfaces and collections I added, aside from the special IonLocation map.

Then the implication is that toImmutableList/Set/Map don't perform defensive copies as you said.

Checking the implementation it looks like this behavior comes from creating an empty immutable thing and then adding the other elements to it while still using the storage of source collection, as described in the README + and - operators. Is that right?

That's what I would have hoped. However, toImmutableList() actually returns a PersistentList (which, to be fair, is an implementation of ImmutableList) by delegating to toPersistentList()... which creates the PersistentList by creating an empty persistent list and then adding all of the elements to that persistent list (persistentListOf<T>() + this). Then, we see that operator fun <E> PersistentList<E>.plus(elements: Iterable<E>) delegates to PersistentList.addAll(elements: Collection<E>). Since we're adding to an empty persistent list, the actual implementation is going to be SmallPersistentVector.addAll(elements: Collection<E>). At this point, there are two possible execution paths. If there are less than 32 elements being added, it will allocate a new array to hold the elements, copy them into the array, and then returns a new SmallPersistentVector that wraps the new array. If there are more than 32 elements, it's even worse—it instead delegates to PersistentList<T>.mutate(mutator: (MutableList<T>) -> Unit).

That function calls SmallPersistentVector.builder(), which creates a new instance of PersistentVectorBuilder (which has a decent number of auxiliary fields there). Then it calls PersistentVectorBuilder.addAll(elements: Collection<E>) which starts creating an array of arrays, I think, copying in all of the elements from the collection. Finally, it goes to PersistentVectorBuilder.build(). At this point, it creates a PersistentVector (since there was more than 32 elements).

---

That is what I was trying to avoid. However, I did end up finding ImmutableListAdapter (and its friends), which will actually do what we want them to do.