[mcculls]

What Does This Do

Introduce ObjectStore API + initial implementation backed by a single global weak map.

Motivation

This the first of a series of PRs to implement field-injection / object-storage outside of tracing. The idea here is to use a single global weak map when field-injection is not available / possible.

Store ids are encoded by allocating each type a unique code and combining the key and value type to get a unique store id. This store id is added to the weak-reference used to track the 'holding' object in the global map. This lets us have a single global map, rather than maintain a separate map per-store - at the cost of adding the store-id to each key. (Note the global map approach used here doesn't require that key-value combinations are assigned sequential ids, just that they are unique.)

Lookups use a non-reference type which has a symmetric hashCode / equals to the reference type. This avoids the cost of reference tracking for purely read-only requests. Lookup keys are reusable and cached per-thread to avoid allocations.

The global map has a hard limit of 100_000 entries and a soft limit of 50_000. The soft limit is used as a sign to start proactively removing old entries, which are likely unused but may be still in the map because of a bug in external code using the object-store. This mitigates against a rogue caller using up all the entries. (Also note that the weak store is itself a fall-back for when field-injection was not possible, and is not expected to be heavily used.)

To limit overhead, old entries are identified by a periodic sampling approach whenever removeStaleEntries is called. We therefore don't guarantee that it's always the oldest content that is removed when over the soft limit, but it won't be any of the keys added since the last call to removeStaleEntries.

Once the global store has grown over a certain size we also attempt inline removal of stale entries, using a one-out one-in approach. This is meant to handle situations where the periodic call to removeStaleEntries is not enough to handle churn in the map.

Lastly the GlobalObjectStore type is deliberately public to help migrate the existing field-injection approach in the tracer while the local field-injection feature is completed.

Additional Notes

The unit tests were developed with the help of Claude

Benchmark Results

The single global map approach performs the same as having a map per-store for most occupancy levels. It handles higher occupancy levels better.

Benchmark                            (mapPerStore)  (targetGlobalOccupancy)  Mode  Cnt    Score    Error  Units
ObjectStoreBenchmark.multiThreaded            true                     1000  avgt    5   13.611 ±  0.884  us/op
ObjectStoreBenchmark.multiThreaded            true                     5000  avgt    5   20.110 ±  2.394  us/op
ObjectStoreBenchmark.multiThreaded            true                    10000  avgt    5   22.185 ±  1.700  us/op
ObjectStoreBenchmark.multiThreaded            true                    50000  avgt    5   43.388 ± 23.627  us/op
ObjectStoreBenchmark.multiThreaded            true                   100000  avgt    5   99.163 ± 29.521  us/op
ObjectStoreBenchmark.multiThreaded            true                   150000  avgt    5  463.634 ± 24.673  us/op
ObjectStoreBenchmark.multiThreaded           false                     1000  avgt    5   12.696 ±  2.571  us/op
ObjectStoreBenchmark.multiThreaded           false                     5000  avgt    5   19.393 ±  2.190  us/op
ObjectStoreBenchmark.multiThreaded           false                    10000  avgt    5   23.948 ±  1.946  us/op
ObjectStoreBenchmark.multiThreaded           false                    50000  avgt    5   38.192 ± 17.200  us/op
ObjectStoreBenchmark.multiThreaded           false                   100000  avgt    5   95.333 ± 21.563  us/op
ObjectStoreBenchmark.multiThreaded           false                   150000  avgt    5  157.312 ± 39.411  us/op
ObjectStoreBenchmark.singleThreaded           true                     1000  avgt    5   10.452 ±  0.810  us/op
ObjectStoreBenchmark.singleThreaded           true                     5000  avgt    5   14.262 ±  2.894  us/op
ObjectStoreBenchmark.singleThreaded           true                    10000  avgt    5   15.636 ±  1.375  us/op
ObjectStoreBenchmark.singleThreaded           true                    50000  avgt    5   30.095 ±  1.779  us/op
ObjectStoreBenchmark.singleThreaded           true                   100000  avgt    5   58.462 ±  1.502  us/op
ObjectStoreBenchmark.singleThreaded           true                   150000  avgt    5  106.976 ±  3.885  us/op
ObjectStoreBenchmark.singleThreaded          false                     1000  avgt    5    9.596 ±  0.312  us/op
ObjectStoreBenchmark.singleThreaded          false                     5000  avgt    5   14.098 ±  1.130  us/op
ObjectStoreBenchmark.singleThreaded          false                    10000  avgt    5   16.159 ±  0.739  us/op
ObjectStoreBenchmark.singleThreaded          false                    50000  avgt    5   30.951 ±  2.350  us/op
ObjectStoreBenchmark.singleThreaded          false                   100000  avgt    5   61.481 ±  7.446  us/op
ObjectStoreBenchmark.singleThreaded          false                   150000  avgt    5   92.547 ±  2.461  us/op

[amarziali]

looks good. I left a couple of minor/question

[mcculls]

To test old key eviction I wrote a sample app that continually adds numeric keys in increasing order to the object-store, and tweaked the global map code to dump out which keys were evicted via the old key process. The results look acceptable with lower keys generally evicted over higher keys.

The upwards trend holds even when removeStaleEntries is called at a really low rate, such as once every 10_000 keys:

[PerfectSlayer]

I left a bunch of comments, mostly about API, but it looks good to me 👍

🎯 suggestion: ‏Quick feedback about the store API: I found the getOrPut() and getOrCompute() a bit surprising -- I hard to read through the Javadoc twice to make sure I get the expected behavior.
What about using the API similar the Java map instead: computeIfAbsent()? And the get counterpart can become putIfAbstent(). Do you think it can make it more explicit?

💭 thought: ‏I left few comments about null checks, do you think adding a package-info.java with @ParametersAreNonnullByDefault would API users and their IDE?

[PerfectSlayer]

🔍 nitpick: Java naming conventions -- help with code reviews 🙏 ‏

Suggested change

	private static final Map<String, Short> typeIds = new ConcurrentHashMap<>();
	private static final Map<String, Short> TYPE_IDS = new ConcurrentHashMap<>();

[PerfectSlayer]

🔍 nitpick: Java naming conventions -- help with code reviews 🙏

Suggested change

	private static final AtomicInteger nextTypeId = new AtomicInteger(1);
	private static final AtomicInteger NEXT_TYPE_ID = new AtomicInteger(1);

[mcculls]

Do we need to distinguish between a constant, i.e. a static immutable value, and a field that happens to be static but actually holds mutable state? For the latter camel-case is probably more appropriate to hint that the contained value is mutable. I'm leaning towards that direction, but will make sure this code-base is consistent one way or another.

[mcculls]

FWIW the old Java style guide stated:

Names of fields being used as constants should be all upper-case, with underscores separating words.
The following are considered to be constants:

1. All static final primitive types (Remember that all interface fields are inherently static final).
2. All static final object reference types that are never followed by "." (dot).
3. All static final arrays that are never followed by "[" (opening square bracket)

Currently this code-base is inconsistent - I'll make it consistent and document the naming convention used in this project

[PerfectSlayer]

❔ question: ‏Should we handle the null values here? It might not be too expensive to do it here 🤷 ‏

[mcculls]

I'll make sure that the exception is some form of IllegalArgumentException

[mcculls]

What about using the API similar the Java map instead: computeIfAbsent()? And the get counterpart can become putIfAbstent(). Do you think it can make it more explicit?

Note this deliberately uses getOrPut and getOrCompute specifically because Map.putIfAbsent does not behave the same way as Map.computeIfAbsent.

Map.computeIfAbsent - either returns the existing value... or it computes a new value, adds it, and returns that

Map.putIfAbsent - either returns the existing value... or it adds the given value to the map, and returns null

This difference in behaviour means you need additional logic to adapt the true behaviour of Map.putIfAbsent to what you want 99% of the time when dealing with a more store-like API, i.e. getOrPut. Previously with the old context store we just changed the behaviour and kept the name - but this led to confusion in the code and having to remember to read the javadoc to find out which behaviour a method provides.

Note that getOrPut and getOrCompute come from Kotlin's API - IMHO they more accurately describe what we want from an object store, whereas putIfAbsent and computeIfAbsent will make people think they're dealing with a map which is not the case when we add field-injection...

[PerfectSlayer]

Note that getOrPut and getOrCompute come from Kotlin's API - IMHO they more accurately describe what we want from an object store, whereas putIfAbsent and computeIfAbsent will make people think they're dealing with a map which is not the case when we add field-injection...

Make sense! Thanks for the clarification 🙏

[PerfectSlayer]

Re-approving