Use of (boxed) Integer in ArrayCache.java doesn't work with interning

[phirrip]

ArrayCache.java uses multiple ConcurrentMap<String, WeakHashMap<Integer, ARRAY>> to cache and potentially re-use them.

e.g.

private static final ConcurrentHashMap<String, WeakHashMap<Integer, String[]>> stringArrayCache = new ConcurrentHashMap();

The Integer keys within the inner WeakHashMap are created by auto-boxing of primitive int arguments passed in, either explicitly for the get* methods, or from the array length value for the release methods.

Integer has an internal caching/interning mechanism to avoid boxing costs. This interning can be controlled via. a system property (java.lang.Integer.IntegerCache.high) to increase the size of the cache. When using ChartFX increasing this cache size can reduce various boxing costs - e.g. style & label maps which use Integer keys.

However increasing this cache size can then cause a build-up of arrays (and their contents - e.g. the String[]s can then contribute significant memory pressure) within ArrayCache.java - since the boxed Integers are cached with strong references the ArrayCache WeakHashMaps then never evict.

ArrayCache.java should not use Integer in the WeakHashMaps.

Assuming you want to keep the same pattern but with fewer gotchas ArrayCache.java should define a class equivalent to Integer (wrapping a primitive int) but with a controlled life-cycle (presumably no interning) to use as keys in the WeakHashMaps.

[wirew0rm]

Hey,
thanks for taking the time to report this. In our tests this never produced significant problems but we also didn't change the jvm defaults.

On your proposed fix, wouldn't it be sufficient to explicitly use new Integer(array.size) in the release(array) functions? IIRC, the interning only happens for auto-boxing and Integer.valueOf() but not for explicit constructor calls with new. Of course with a comment explaining the necessity of the new so it doesn't get removed on the next cleanup.

I'll have to investigate this and I also have some other problems I need to take care of first, but if you want to provide a PR addressing this I would gladly merge it.

[phirrip]

You're right that you could use the Integer constructor to get around the interning, however I think this would be less clear / self describing. Also the Integer constructor is deprecated from Java 9.

[dedeibel]

I would also like to be able to replace the DoubleArrayCache in ErrorDataSetRenderer for other reasons and created a prototype that worked well for me 1a5ca72

Ralph pointed me to this thread and noted that @ennerf might already have thoughts on this. Can you maybe comment?

DoubleArrayCache seems relatively unrelated to ArrayCache but I guess it seems like you would want to be able to control and replace none or all of the caching.

I guess an interface that can provide a cache for all types is needed then – seems like ArrayCache is already halfway there. How about a way to specify a ArrayCache instance as an implementation.

[wirew0rm]

There is some more discussion and another proof of concept implementation in #370.

[phirrip]

I do agree DoubleArrayCache has other properties I would like to change unrelated to this issue.

Using soft references and a linear lookup time for arrays is very undesirable in my usage (points appended live over time) as the cache can build up to very large and expensive to access. I already have to periodically clear the DoubleArrayCache to avoid significant performance regressions. If this were behind an interface and could optionally be specified to the renderer I could provide an implementation which better suits my needs.