On Thu, 4 Aug 2022 10:57:53 GMT, Peter Levart <[email protected]> wrote:
> This is an attempt to fix inconsistent behavior of TerminatingThreadLocal(s)
> when used internally in JDK for per-carrier-thread caches of native
> ByteBuffer(s) and NativeBuffer(s). If used from virtual thread, such
> TerminatingThreadLocal(s) are not registered with the carrier thread for the
> cleanup.
> The fix introduces an internal CarrierThreadLocal subclass of ThreadLocal
> which rewires its API methods to use current thread's carrier thread as the
> source of ThreadLocalMap instead of current thread itself (if it is a
> VirtualThread for example). TerminatingThreadLocal is now a subclass of
> CarrierThreadLocal. It seems only per-carrier-thread caching is a usecase for
> it. The uses of JavaLangAccess in various places to access a carrier-thread
> value of given ThreadLocal has been replaced by public API calls and the use
> of CarrierThreadLocal instead of plain ThreadLocal. JavaLangAccess is still
> used to dispatch the CarrierThreadLocal API methods, but only for that. Would
> someone be tempted to use JavaLangAccess methods directly, they now require
> CarrierThreadLocal argument to guard against missuses.
> The REGISTRY of TerminatingThreadLocal(s) that tracks which of them have
> values bound to a particular carrier thread is now implemented conveniently
> with a CarrierThreadLocal.
> The test is expanded with a case that demonstrates a situation where a
> carrier thread is terminated. Since it must wait for 30 seconds for that to
> happen, only one of the test cases is performed in this mode. The correct
> logic of TerminatingThreadLocal is still verified with all test-cases using
> platform threads that can be terminated more rapidly.
I did a quick pass over this and it looks good. Adding CarrierThreadLocal is a
good idea and as it allows the use-sites to be revert/cleaned up. Just a few
minor comments so far.
src/java.base/share/classes/sun/nio/ch/Util.java line 233:
> 231: }
> 232:
> 233: BufferCache cache = bufferCache.get();
I assume JLA is no longer needed.
test/jdk/jdk/internal/misc/TerminatingThreadLocal/TestTerminatingThreadLocal.java
line 40:
> 38: * @compile --enable-preview -source ${jdk.version}
> TestTerminatingThreadLocal.java
> 39: * @run main/othervm --enable-preview
> -Djdk.virtualThreadScheduler.parallelism=1
> -Djdk.virtualThreadScheduler.maxPoolSize=2 TestTerminatingThreadLocal
> 40: */
If you add `@enablePreview` then it will allow you to drop the `@compile` line
and drop `--enable-preview from the `@run` tag.
You might want to add the bugId to the `@bug` list too.
-------------
PR: https://git.openjdk.org/jdk/pull/9743
