Caffeine: Creating a cache leaks a small amount of memory until Java 10 as of 2.6.1

[jschlather]

This is probably not an issue for the majority of users of caffeine. There's currently a memory leak in method handles that is fixed in java 10 https://bugs.java.com/view_bug.do?bug_id=JDK-8174749 . So, this commit switching things over to method handles has the unfortunate side effect that creating a cache will now leak a small amount of memory. We saw this in our apps because we have some tooling that swaps out caches repeatedly.

[ben-manes]

Thank you. Do you have any recommendations for how we should approach this? We could either wait until JDK10 (March) or rely on reflection instead.

[jschlather]

I'm not very familiar with the benefits of method handles vs reflection. It seems like it might be safer to rely on reflection until java 10 has been released for awhile.

[ben-manes]

In our case of instantiating the cache, the benefits are probably moot. For individual entries we create them directly. Since method handles were advocated by the JDK team it seemed equally valid for us to use, and below you'll see them being ~3x faster. I'm okay reverting back since the usage is pretty minor.

Benchmark                                   Mode  Cnt         Score         Error  Units
FactoryBenchmark.direct                    thrpt   10  80030436.680 ± 1273539.573  ops/s
FactoryBenchmark.methodHandle_invoke       thrpt   10  62577082.484 ± 2720153.408  ops/s
FactoryBenchmark.methodHandle_invokeExact  thrpt   10  62964398.800 ± 2959115.745  ops/s
FactoryBenchmark.reflectionFactory         thrpt   10  24098580.087 ±  428434.865  ops/s

[johnou]

@ben-manes couldn't we ask them to back-port the fix to JDK8 before it's EOL?

[slandelle]

@johnou @ben-manes JDK 8's EOL is January 2019 (it's the LTS until Java 11) so I'd be very disappointed if the fix wasn't back-ported.

[ben-manes]

I suspect only P1 bugs are put into LTS releases. These bugs in the Collections Framework seemed serious enough to backport and isolated code to be done without much effort, yet they decided not to.
JDK-8071667: HashMap corrupts itself with recursive computations
JDK-8142175: ConcurrentHashMap live-locks with recursive computations
JDK-8186171: HashMap's breaks the Iterator contract for Entry.setValue

[slandelle]

@ben-manes thanks for the (sad) info!

[johnou]

@ben-manes are you sure?

https://bugs.java.com/view_bug.do?bug_id=8162795 states

Redoing MemberNameTable changes as per 8152271.
Intended for jdk9 and jdk8, while JDK-8174749 will introduce a better solution for jdk10.

I would stick with method handles as reflection can produce scary warnings with jdk9..

[jschlather]

The phrasing on that ticket is not clear, but the fix in jdk8 and jdk9 still leaks memory. I posted on the hotspot-dev mailing list while tracking down this issue in our stack http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-February/030068.html and one of the hotspot devs clarified the issue

The MemberNameTable in jdk8 used jweak as pointers to MemberNames so the
below increases in JNI block memory would also be accountable to the
MemberNameTable. We fixed this temporarily in jdk8 and 9 to leak less
memory but it still leaked. The long term non-leaking fix is in jdk10,
which is pointed to by the bugs you found.

[ben-manes]

@johnou I think that I have the next release ready, so let me know if you have remaining concerns. If so, any suggestions on what you'd like to see done? The only option I can think of is to inspect the Java version.

[johnou]

@ben-manes we can always go back to method handles if they backport the fix, don't let my concerns on the issue hold you from releasing :+1:

[ben-manes]

Released. Thanks!

[DavyLandman]

just an update, as @johnou marked, the bugfix was backported to jdk8-121 and jdk9-b152, so maybe you could reintroduce the method handles?

https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8162795