fix(java): run descriptor warm-up on Fory's compiler pool to fix thread context classloader - #4112
Draft
stevenschlansker wants to merge 2 commits into
Draft
stevenschlansker wants to merge 2 commits into
stevenschlansker wants to merge 2 commits into
Conversation
…s shutdown hook Descriptor warm-up used ForkJoinPool.commonPool(), whose threads serve the whole JVM. When Fory is loaded by a classloader other than the system loader, such as a plugin or per-application loader, warm-up tasks do not run with that loader as their context classloader (TCCL). On JDK 9+, common-pool threads use the system loader. On JDK 8, a thread inherits the TCCL of whichever thread started it, which can be another application's loader, and the shared thread keeps that loader reachable. Warm-up now uses CodeGenerator.getCompilationService(), Fory's own pool. Before, only async compilation created that pool. Now the first descriptor build that submits warm-up work also creates it, including in the default configuration. ForyJitCompilerThreadFactory sets each pool thread's TCCL to the loader that defines Fory, instead of inheriting the TCCL of the submitting thread. Fory already falls back to that loader when the TCCL is null. The pool's JVM shutdown hook (apache#3138) is removed. The JVM keeps a registered hook until exit, so the hook kept Fory's classloader reachable, and its thread kept the TCCL of the thread that created the pool. With the pool now created in the default configuration, that hold would apply to every application. The pool threads are daemons and idle threads time out after 5 seconds, so the pool does not delay JVM exit. Without the hook, in-flight tasks no longer get up to 1 second at exit, and tasks submitted during shutdown are accepted instead of rejected with a warning. This removes only the compiler pool's hold on the loader: other Fory state, such as the MultiKeyWeakMap cleaner thread, still keeps the loader reachable after a Fory instance is created. seMaxCompilationThreadPoolSize now rejects values below 1 when it is called, instead of failing later at pool creation. A call after the pool exists logs a warning and has no effect. The default size is computed when the pool is created, because CodeGenerator is initialized at native-image build time and its static initializer sized the pool for the build machine. Native-image build time keeps the common pool for warm-up, because using the compiler service there would store a direct executor in the image.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why?
The
Descriptorclass usesForkJoinPool.commonPool()which is global to the whole JVM.When Fory is loaded by an isolated classloader other than the system loader, like a WAR, warmup tasks run with the wrong context classloader (TCCL) - the system loader or inherited, depending on JDK version.
What does this PR do?
Warm-up now uses
CodeGenerator.getCompilationService(), Fory's own pool.ForyJitCompilerThreadFactorysets each pool thread's TCCL to the loader that defines Fory, instead of inheriting the TCCL of the submitting thread. Fory already falls back to that loader when the TCCL is null.Remove shutdown hook for CodeGenerator thread pool: it retains Fory's classloader until exit, and the pool is daemon anyway.
seMaxCompilationThreadPoolSizenow rejects requested thread count < 1Native-image build time keeps the common pool, because the compiler service is a direct executor there. Pool size computed at native-image runtime instead of build time.
TODO in the code is removed
Related issues
#3138 introduces the shutdown hook without much explanation. I do not want to delete a necessary feature, but shutdown hooks are JVM-global and should be used sparingly.
AI Contribution Checklist
Review artifacts inline:
I worked through the low and nit findings and declined them: large increase of scope to fix small problems:
CodegenContextcacheDoes this PR introduce any user-facing change?
seMaxCompilationThreadPoolSizenow rejects requested thread count < 1seMaxCompilationThreadPoolSizemust now be set before Fory descriptors are read, otherwise it warnsNo negative production impact anticipated.
Benchmark
I compared this branch with apache/main (33abafa) on JDK 25, using a generated graph of 400 classes with nested object, List, and Map fields. Each measurement ran in a fresh JVM, the two versions alternated in adjacent pairs, and I report the median of 20 rounds at -XX:ActiveProcessorCount=2 and 8. The first serialize with synchronous JIT changed by +0.2% at 2 CPUs and -0.1% at 8 CPUs, which is within noise.
The first serialize with async compilation, timed until both the common pool and the compiler pool were idle, was 1.7% (36 ms) slower at 2 CPUs and 1.1% (22 ms) slower at 8 CPUs. A cold descriptor build with no Fory instance was 5.7 ms (8%) slower at 2 CPUs and 10 ms (16%) slower at 8 CPUs, mostly because it now loads about 94 more classes for CodeGenerator and its executor, which a Fory instance that compiles loads anyway. With the compiler pool already created, that difference falls to 1.4 ms (2.6%, measured at 4 CPUs), and the change does not touch the steady-state serialization path.
Since this is a bugfix, I think a small startup cost is worth it.