Removes the binary reader's SymbolToken cache, which provided a minor optimization for certain cases but a major regression for others.

[tgregg]

Description of changes:

The symbolTokensById cache was added to avoid repeatedly allocating SymbolToken wrappers when symbol lookup APIs that return SymbolToken were called. This provided a minor (<10%) speedup due to reduced allocation rate / GC churn in cases where such SymbolToken APIs were called frequently. However, it should be very uncommon for this to be the case, as users tend to use the equivalent APIs that return String instead, as most do not need to deal with symbols with unknown text.

There is some usage of these APIs within the IonJava DOM, but when using the DOM, other sources of slowdown tend to dominate. There is, however, a case where this cache can significantly hurt performance within the DOM: when a small binary Ion value is loaded into the DOM from a stream that imports a large shared symbol table. In that case, the cache is allocated to fit all symbols, even the imported ones. This can cause allocation of this large list, and the resulting GC churn, to dominate.

Because the cache only has minor benefits to an uncommon case, I propose simply removing it. If in the future we find the need to optimize this uncommon case, we can consider more robust options, like:

1. Capping the number of symbols that are cached, or
2. Caching only the local symbols, or
3. Using a map so that only the symbols that are actually looked up receive space in the cache.

Note: this cache was introduced in the incremental reader implementation made opt-in in v1.9.0, and was ultimately adopted by the unified implementation introduced in 1.11.0.

Performance comparisons:

To demonstrate the regression, I ran the following data through ion-java-benchmark-cli:

$ion_symbol_table::{imports:[{name:"largeTable",version:1,max_id:1000000}]} {$11:$12,$13:$14}

In a separate PR, we will add this data and the following benchmark CLI invocation to our regression detection workflow.

v1.10.5

Benchmark                                                                        (input)                                                                                                            (options)  Mode  Cnt        Score         Error   Units
Bench.run                                        singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6        0.707 ±       0.030   us/op
Bench.run:Heap usage                             singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6      540.219 ±     493.174      MB
Bench.run:Serialized size                        singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6       ≈ 10⁻⁴                    MB
Bench.run:·gc.alloc.rate                         singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6     2260.573 ±     142.599  MB/sec
Bench.run:·gc.alloc.rate.norm                    singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6     1760.085 ±      34.754    B/op
Bench.run:·gc.churn.G1_Eden_Space                singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6     2237.120 ±     179.196  MB/sec
Bench.run:·gc.churn.G1_Eden_Space.norm           singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6     1741.696 ±      65.421    B/op
Bench.run:·gc.churn.G1_Survivor_Space            singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6        0.007 ±       0.003  MB/sec
Bench.run:·gc.churn.G1_Survivor_Space.norm       singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6        0.005 ±       0.002    B/op
Bench.run:·gc.count                              singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6      296.000                counts
Bench.run:·gc.time                               singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6      175.000                    ms

v1.11.3

Benchmark                                                                   (input)                                                                                                            (options)  Mode  Cnt        Score      Error   Units
Bench.run                                   singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6     2729.569 ±   25.922   us/op
Bench.run:Heap usage                        singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6      348.096 ±  296.065      MB
Bench.run:Serialized size                   singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6       ≈ 10⁻⁴                 MB
Bench.run:·gc.alloc.rate                    singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6     1331.417 ±   12.781  MB/sec
Bench.run:·gc.alloc.rate.norm               singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6  4003446.459 ±   57.252    B/op
Bench.run:·gc.churn.G1_Survivor_Space       singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6        0.008 ±    0.002  MB/sec
Bench.run:·gc.churn.G1_Survivor_Space.norm  singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6       22.762 ±    5.255    B/op
Bench.run:·gc.count                         singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6      393.000             counts
Bench.run:·gc.time                          singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6     6440.000                 ms

v1.11.4-SNAPSHOT (with proposed fix)

Bench.run                                   singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6      0.649 ±    0.044   us/op
Bench.run:Heap usage                        singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6    388.521 ±  290.371      MB
Bench.run:Serialized size                   singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6     ≈ 10⁻⁴                 MB
Bench.run:·gc.alloc.rate                    singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6   3975.192 ±  273.111  MB/sec
Bench.run:·gc.alloc.rate.norm               singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6   2840.134 ±    0.007    B/op
Bench.run:·gc.churn.G1_Eden_Space           singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6   3968.071 ±  259.420  MB/sec
Bench.run:·gc.churn.G1_Eden_Space.norm      singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6   2835.120 ±   18.492    B/op
Bench.run:·gc.churn.G1_Survivor_Space       singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6      0.012 ±    0.004  MB/sec
Bench.run:·gc.churn.G1_Survivor_Space.norm  singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6      0.009 ±    0.002    B/op
Bench.run:·gc.count                         singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6    526.000             counts
Bench.run:·gc.time                          singleValueThatUsesLargeSymbolTable.10n  read::{f:ION_BINARY,I:"largeSharedSymbolTable.ion",t:BUFFER,c:"largeSharedSymbolTable.ion",a:DOM,R:NON_INCREMENTAL}  avgt    6    318.000                 ms

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

[popematt]

Another solution would be to change this to a Map<Int, SymbolToken> and then lazily populate it as symbols are requested. This means that (a) the size of the cache is determined by the number of unique SymbolTokens that are requested, and (b) each SymbolToken is allocated only once. However, it does incur the cost of looking something up in a Map.

[tgregg]

Yeah, I called this out in the PR description too. I'd prefer to remove the cache until it's clear that there is a use case that demands it.

[popematt]

Wow, I don't know how I missed that.