You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Do not ask the VM for classes by signature on each class unload - #1057
Do not ask the VM for ClassesBySignature on every class unload event
VirtualMachineImpl.removeKnownRefType() sent a ClassesBySignature command
for each ClassUnload event. The target VM walks all of its loaded classes
to answer, so unloading thousands of classes kept its JDWP agent busy for
minutes and delayed further class unloading. The answer could not contain
the unloaded class anyway, so its mirror was never removed from the cache.
Look the class up in the cached mirrors instead, by signature.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
Unload handling can incorrectly evict a still-loaded same-signature mirror, and the regression test can mask this bug by repopulating the cache before checking preservation.
Addresses #1056 by using cached reference-type mirrors during class-unload handling, avoiding expensive target-VM class scans that slow application reloads.
Changes:
Adds a signature-to-mirror-ID index and updates it when signatures become known.
Uses cached candidates to locate and remove unloaded mirrors.
Adds unload-event tests, including same-name classes loaded by different class loaders.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Unload handling still misses mirrors obtained through name/signature lookups and scans the entire cache per unloaded class, risking stale cached data and event-processing stalls during large reloads.
No open issues 🎉
2 previously missed issues
In code that hasn't changed since last review
Set signatures on mirrors returned by class lookup
Mirrors first obtained through classesByName()/classesBySignature() are missed by this index: readWithTypeTag() caches a new mirror without setting its signature. Unless the client subsequently calls name() or signature(), unload handling finds no candidate and leaves that mirror and any cached fields/methods unflushed. In classesBySignature(), call elt.setSignature(signature) after the null check and before elements.add(elt) (line 734), and cover this lookup path without a class-prepare request.
This still performs a full debugger-side cache scan per unloaded class: ClassTypeImpl.flushStoredJdwpResults() and InterfaceTypeImpl.flushStoredJdwpResults() call allCachedRefTypes(), whose ValueCache.values() scans and copies the entire cache. Now that unloaded mirrors are actually found, unloading U cached types with N cached types still costs O(U × N), with a full snapshot allocation for each flush, which can stall event processing during large reloads. Please use unload-specific local invalidation or batch dependency invalidation rather than running the full dependency scan for each unloaded mirror.
Thanks, the 5 warnings are fixed (restriction warnings suppressed in the test like in the other JDI tests, unused lambda parameter removed). I also addressed the two additional Copilot findings: mirrors returned by classesBySignature() are now indexed (with a test), and unload no longer scans the whole mirror cache per unloaded class.
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
The unload-only index includes array types, but JDWP emits no unload events for them, allowing stale array IDs to accumulate across class-loader reloads.
…se-jdt#1056
For every ClassUnload event, VirtualMachineImpl.removeKnownRefType()
asked the target VM for the classes with the signature of the unloaded
class. To answer, the VM walks all of its loaded classes, so unloading
thousands of classes at once (e.g. a web application reload) kept the
JDWP agent of the target busy for minutes, and classes were not
unloaded meanwhile. The answer was of no use either: it never contains
the unloaded class, so its mirror stayed in the cache, while the mirror
of a still loaded class with the same name was removed.
Now the cached mirrors are indexed by signature and the unloaded class
is looked up in the cache only. A cached class with that signature is
removed only if the VM reports that it is no longer prepared, which is
a cheap request by reference type ID. This also covers the same unload
being reported more than once and classes with the same name loaded by
other class loaders. Mirrors returned by classesBySignature() are
indexed as well, and only the data of the unloaded mirror itself is
flushed, instead of scanning the whole cache for dependent types.
ClassUnloadEventTest covers the unload of a class known from a class
prepare event, of a class with a same-named class still loaded, and of
a class found by classesByName().
Fixeseclipse-jdt#1056
Assisted-by: Anthropic Claude (Claude Fable 5.2)
We are too late for M3 build, so I fear this change has to wait till 4.43 release, except the nightly build today will have troubles to be promoted as M3.
I will try to review tomorrow or next week, it is on my list.
Btw, did you made any performance measurements for this patch?
I am running Eclipse with this fix, and from a user perspective (mine), it was an enormous difference: after about 3 Tomcat's reloads the application froze for more than a minute. Now it is instantaneous.
(only after reloads that it got slow).
About some concrete measurements, Claude has some:
I measured with the reproducer from #1056 (30,000 classes kept loaded, 4 rounds unloading 3,000 classes each), debugged by a minimal debugger on top of the Eclipse JDI (jdimodel.jar of 4.40 vs. the same jar with this PR), JDK 21 on Windows 10:
Scenario
JDI
Rounds
Target busy afterwards
Target CPU
Debugger CPU
Unload only
4.40
94.1 s
36.3 s
127.5 s
1.6 s
Unload only
this PR
1.1 s
0.0 s
0.4 s
0.4 s
All classes cached
4.40
53.8 s
72.5 s
124.3 s
2.1 s
All classes cached
this PR
1.4 s
0.0 s
1.2 s
2.4 s
"All classes cached" has a class prepare request for all classes, so every unloaded class has a cached mirror. That is the worst case for the new code (one status request per unloaded class), and the debugger side costs about the same as before.
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
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.
SougandhS
changed the title
What it does
Do not ask the VM for ClassesBySignature on every class unload event
VirtualMachineImpl.removeKnownRefType() sent a ClassesBySignature command
for each ClassUnload event. The target VM walks all of its loaded classes
to answer, so unloading thousands of classes kept its JDWP agent busy for
minutes and delayed further class unloading. The answer could not contain
the unloaded class anyway, so its mirror was never removed from the cache.
Look the class up in the cached mirrors instead, by signature.
Fixes #1056
🤖 Generated with Claude
How to test
There is a reproducer in the issue, and this PR has tests
Author checklist