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

Reply via email to