On Mon, 26 May 2025 17:16:02 GMT, Alan Bateman <al...@openjdk.org> wrote:
> A ForkJoinPool can be created with worker threads that clear thread locals > between tasks, thus avoiding a build up of thread locals left behind by tasks > executed in the pool. The common pool does this. Erasing thread locals (by > writing null to Thread.threadLocals) grinds with thread locals that keep > native memory alive, esp. when there isn't a Cleaner or other means to free > the memory. > > For the JDK, this is currently an issue for the NIO direct buffer cache. If a > task running on a thread in the common pool does socket or network channel > I/O then it can leak when the task terminates. Prior to JDK 24 each buffer in > the cache had a cleaner, as if allocated by ByteBuffer.allocateDirect, so it > had some chance of being released. The changes in > [JDK-8344882](https://bugs.openjdk.org/browse/JDK-8344882) mean there is no > longer is cleaner for buffers in the cache. > > The options in the short term are to restore the cleaner, register a clearer > for the buffer cache, have the FJP resetThreadLocals special case > "terminating thread locals", or move the terminating thread locals to a > different TL map. Viktor Klang, Doug Lea, and I discussed this topic recently > and agreed the best short term approach is to split the map. As terminating > thread locals are for platform threads then the map can be in the > Thread.FieldHolder and avoid adding another field to Thread. Medium to long > term require further thought in this area, including seeing what might be > useful (and safe) to expose. > > Testing 1-5. Performance testing ongoing. @AlanBateman I think this approach is a good step in the right direction here. src/java.base/share/classes/java/lang/InheritableThreadLocal.java line 95: > 93: void createMap(Thread t, T firstValue) { > 94: var map = new ThreadLocalMap(this, firstValue); > 95: t.setInheritableThreadLocals(map); Stylistic suggestion: Suggestion: t.setInheritableThreadLocals(new ThreadLocalMap(this, firstValue)); src/java.base/share/classes/java/lang/Thread.java line 246: > 244: volatile boolean daemon; > 245: volatile int threadStatus; > 246: ThreadLocal.ThreadLocalMap terminatingThreadLocals; I think it's worth documenting the memory safety aspects of this field. src/java.base/share/classes/java/lang/ThreadLocal.java line 284: > 282: */ > 283: ThreadLocalMap getMap(Thread t) { > 284: if (this instanceof TerminatingThreadLocal<T>) { Would it make sense to override `getMap` in TerminatingThreadLocal instead? src/java.base/share/classes/jdk/internal/access/JavaLangAccess.java line 549: > 547: void removeCarrierThreadLocal(CarrierThreadLocal<?> local); > 548: > 549: /** Copyright update? src/java.base/share/classes/jdk/internal/misc/TerminatingThreadLocal.java line 82: > 80: */ > 81: public static void register(TerminatingThreadLocal<?> tl) { > 82: if (tl != REGISTRY) { Would this happen in any well-behaved application? If not, might be better to throw an exception? 🤔 src/java.base/share/classes/jdk/internal/misc/TerminatingThreadLocal.java line 96: > 94: > 95: /** > 96: * A per-terminating-thread registry of TerminatingThreadLocal(s) > that have been Perhaps "A per-platform-thread registry ..."? src/java.base/share/classes/sun/nio/ch/IOVecWrapper.java line 83: > 81: if (wrapper != null) { > 82: wrapper.vecArray.free(); > 83: cache[0] = null; Perhaps null it out before calling free()? (in case free() fails) test/jdk/jdk/internal/misc/TerminatingThreadLocal/TestTerminatingThreadLocal.java line 194: > 192: assertNull(tl.get()); > 193: assertEquals(ttl.get(), ttlValue); > 194: } catch (Exception ex) { Perhaps safer with catching Throwable—if there are any Errors thrown. ------------- PR Comment: https://git.openjdk.org/jdk/pull/25457#issuecomment-2916569305 PR Review Comment: https://git.openjdk.org/jdk/pull/25457#discussion_r2112057817 PR Review Comment: https://git.openjdk.org/jdk/pull/25457#discussion_r2109017596 PR Review Comment: https://git.openjdk.org/jdk/pull/25457#discussion_r2109025118 PR Review Comment: https://git.openjdk.org/jdk/pull/25457#discussion_r2112062254 PR Review Comment: https://git.openjdk.org/jdk/pull/25457#discussion_r2115588231 PR Review Comment: https://git.openjdk.org/jdk/pull/25457#discussion_r2109416867 PR Review Comment: https://git.openjdk.org/jdk/pull/25457#discussion_r2109418667 PR Review Comment: https://git.openjdk.org/jdk/pull/25457#discussion_r2112066646
