feat(instr): INVOKEDYNAMIC handler isolation with detach-safe dispatch#830
Open
feat(instr): INVOKEDYNAMIC handler isolation with detach-safe dispatch#830
Conversation
Replace INVOKESTATIC handler copying with INVOKEDYNAMIC dispatch so probe handler methods stay in the probe class (bootstrap CL) and are called via ConstantCallSite, eliminating bytecode copying into target classes. New: - IndyDispatcher: Java 8+ bootstrap method using publicLookup().findStatic() on the probe class; wired from HandlerRepositoryImpl's static initializer via reflection on IndyDispatcher.repository (volatile bridge field) - HandlerRepositoryImpl: rewrites resolveHandler() to return MethodHandle instead of byte[]; uses ConcurrentHashMap with UNRESOLVED sentinel for failed lookups; registerProbe() now evicts stale UNRESOLVED entries so late-resolving probes work after a registration race - DispatchBenchmark: JMH benchmark for ConstantCallSite dispatch overhead Deleted: - Indy.java (Java-15-specific defineHiddenClass bootstrap, no longer needed) - CopyingVisitor.java (handler bytecode copying, no longer needed) - static/ golden files (~198 files, replaced by unified dynamic/ files) Refactored: - Instrumentor.invokeBTraceAction: always emits INVOKEDYNAMIC; removed useHiddenClasses dual-mode gate; bootstrap handle owner: Indy → IndyDispatcher - Probe lifecycle symmetry: registerProbe() moved to BTraceProbeNode/ BTraceProbePersisted.register() (after defineClass); unregisterProbe() moved to both unregister() methods; removed premature registration in BTraceProbeFactory and redundant unregisterProbe in Client.onExit() - BTraceRuntimeImpl_9/_11: StackWalker frames filtered for org.openjdk.btrace.runtime.auxiliary.* to skip IndyDispatcher frames in getCallerClassLoader() and getCallerClass() - HandlerRepositoryImpl: dead probe.getClass() condition simplified to always load probe script from bootstrap CL (where it is defined) - IndyDispatcher: added diagnostic logging for handler resolution failures Test plan: ./gradlew :btrace-instr:test — all instrumentor tests pass ./gradlew :btrace-instr:test -PupdateTestData — 382 dynamic golden files regenerated Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Documents the develop-only branch model, build commands, module layout, and PR checklist so automated review sessions always target the correct integration branch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
4 tasks
…ilure
IndyDispatcher lives in the bootstrap classloader. Adding SLF4J Logger
initialization to it caused LoggerFactory.getLogger() to run during
bootstrap class init, where no SLF4J provider is available. This made
Class.forName("...IndyDispatcher") throw ExceptionInInitializerError,
which HandlerRepositoryImpl's static initializer caught and swallowed —
leaving IndyDispatcher.repository null and all @OnMethod handlers as
permanent noops.
Handler resolution failures are already logged by HandlerRepositoryImpl
on the agent-classloader side. No logging is needed in the bootstrap
dispatcher itself.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…dispatch getBytecode(true) filtered to only BCP-required methods, which excluded @OnMethod handlers (isBcpRequired() returns false when om != null). The bootstrap-CL probe class therefore had no handler methods, causing IndyDispatcher → HandlerRepositoryImpl.resolveHandler() → publicLookup().findStatic() to throw NoSuchMethodException, which was caught and stored as UNRESOLVED, permanently installing a noop ConstantCallSite for every instrumented call site. Fix: also include methods where getOnMethod() != null in the bootstrap-CL class, and include their callees transitively. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…strap class The INDY call site descriptor (generated by Instrumentor.invokeBTraceAction) replaces AnyType with Object for JVM stack compatibility. The bootstrap-CL probe class must have matching method descriptors so that publicLookup().findStatic(probeClass, name, handlerType) succeeds. Without this, handlers using @return AnyType or AnyType[] parameters (oneliners with args, return-value captures, etc.) resolve as UNRESOLVED and get a permanent noop CallSite, silencing those probe points. Apply the same AnyType→Object descriptor substitution that copy() uses for handler bytecode sent to defineHiddenClass (the old Java-15 path). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…r lookup Two fixes: 1. BTraceProbeNode.getBytecode(true): also include @OnProbe handler methods (op != null) in the bootstrap-CL probe class. mapOnProbes() maps @OnProbe to synthetic @OnMethod entries which generate INDY call sites; the handler method bodies must be present in the bootstrap class for resolveHandler() to find them via findStatic(). 2. BTraceRuntimeImpl_8.getCallerClassLoader/getCallerClass: skip org.openjdk.btrace.runtime.auxiliary.* frames when walking the call stack, mirroring BTraceRuntimeImpl_9's StackWalker-based skip of those frames. Before this fix, the probe handler frame (in bootstrap CL) was counted as the application caller, causing Class.forName inside BTraceUtils.field() and similar reflection utilities to use the bootstrap CL and fail with ClassNotFoundException for application classes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…trapClass On JDK 8, BTraceRuntimeImpl_8.isBootstrapClass() calls findBootstrapOrNullMtd.invoke() to check bootstrap classloader membership. After 15 invocations the JVM inflates the reflective accessor, creating sun/reflect/GeneratedMethodAccessorN via ClassLoader.defineClass(). This class-definition callback triggers BTraceTransformer.transform() which calls BTraceClassWriter.getCommonSuperClass() → ClassInfo.inferClassLoader() → isBootstrapClass() → invoke() again. At call count > 15 the JVM tries to inflate ANOTHER accessor (N+1), whose definition triggers another transform() → isBootstrapClass() → invoke() → accessor N+2, and so on indefinitely. The resulting StackOverflowError propagates out of agent initialization, causing testTraceAll (which instruments all classes via @OnMethod(clazz="/.*/")) to fail on JDK 8 with "FATAL: Initialization failed: StackOverflowError". Fix: add a ThreadLocal re-entrancy guard in isBootstrapClass(). While the guard is set (i.e., we are already inside a bootstrap check that triggered accessor inflation), any re-entrant call returns false conservatively. This breaks the infinite recursion; the accessor class is defined once and subsequent invoke() calls are served directly by the generated accessor. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
4d53e21 testTraceAll() on JDK 8 fails with `[BTrace Agent] FATAL: Initialization failed: java.lang.StackOverflowError` because the agent's reflective Method.invoke() crosses the JDK 8 inflation threshold (~15 calls per Method) during agent init, and the resulting class-definition callback re-enters the transformer: BTraceTransformer.transform(SomeAppClass) -> BTraceClassWriter.instrument() -> ASM ClassWriter.getCommonSuperClass(...) -> ClassInfo.inferClassLoader(...) -> BTraceRuntimeImpl_8.isBootstrapClass(...) -> findBootstrapClassOrNull.invoke(...) <-- crosses threshold -> sun.reflect.MethodAccessorGenerator.generateMethod() -> ClassLoader.defineClass("sun/reflect/GeneratedMethodAccessorN") -> BTraceTransformer.transform("sun/reflect/GeneratedMethodAccessorN") -> BTraceClassWriter.instrument() <-- recurses -> ASM getCommonSuperClass(...) -> ... another reflective call -> another GMA -> ... -> StackOverflowError The prior fix (commit 4d53e21) added a `ThreadLocal<Boolean>` re-entrancy guard inside `isBootstrapClass()` to short-circuit the recursive return path. That guard does not break the cascade for two reasons: (1) The ThreadLocal is set/cleared inside a try/finally around invoke(). The JVM's deferred `defineClass` callback runs while the original invoke() is still on the stack — the guard IS technically set on the recursive entry, but returning `false` from a re-entrant `isBootstrapClass()` only mis-reports the bootstrap status of one type lookup; ASM's `getCommonSuperClass()` continues to issue OTHER reflective calls (Class.forName, Method lookups) during frame computation, each of which can trigger inflation of a DIFFERENT Method instance and the next `defineClass` callback. (2) Returning `false` for a class that IS in the bootstrap CL silently corrupts probe matching for the duration of the inflation window. Dead defensive code whose stated invariant doesn't hold is worse than no code: it advertises a protection that isn't there. The structural fix: short-circuit `BTraceTransformer.transform()` for JVM-synthesized reflective accessor classes BEFORE any reflection-driven ASM analysis runs on them. These classes are 1:1 trampolines to a target Method that user probes can already trace directly — there is no tracing value to instrumenting them. Three changes: * btrace-instr/.../BTraceTransformer.java — early-exit at line ~130, after the MethodHandleNatives special case but BEFORE the loader-gated isSensitiveClass() check. Loader-independent because `sun.reflect.Generated*` is loaded by `sun.reflect.DelegatingClassLoader` (neither null nor system), so the existing isSensitiveClass branch never fires for it. Matches both JDK 8 (`sun/reflect/Generated...`) and JDK 9-16 (`jdk/internal/reflect/ Generated...`); JDK 17+ uses hidden classes that are never reported by name to ClassFileTransformer. * btrace-instr/.../ClassFilter.java — add `sun/reflect/` to SENSITIVE_CLASSES for symmetry with the existing `jdk/internal/`, `sun/invoke/` entries and as defense-in-depth: if a future refactor reorders the transformer entry path, the sensitive-class list still prevents instrumentation when the loader happens to be bootstrap or system. * btrace-runtime/.../BTraceRuntimeImpl_8.java — REVERT the ThreadLocal guard added in 4d53e21. With the structural fix the recursion can't form, so the guard is dead code. `isBootstrapClass()` returns to its original one-liner, matching `Impl_9` and `Impl_11` which never had a guard and never reproduced the SOE — confirming the cascade is JDK-8-specific and now structurally cut. Tests: * ClassFilterSensitiveTest — pins `isSensitiveClass()` returning true for `sun/reflect/Generated{Method,Constructor,SerializationConstructor}Accessor` and `jdk/internal/reflect/Generated*`, false for ordinary classes. * BTraceTransformerEarlyExitTest — pins the load-bearing structural early-exit by calling `transform()` directly with a non-null/non-system class loader (mock DelegatingClassLoader) and asserting the result is null. Runs on any JDK, so a future refactor that moves the early-exit below the loader gate would fail loudly on the JDK 11+ build CI even though the SOE itself is not reproducible there. Reviewed via /muse implement chorus (hypnos-augur, rune-augury) — both voices validated the structural approach over the prior MethodHandle-based plan and flagged the 4d53e21 ThreadLocal as both incorrect and dead given the structural fix. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
8542c81 to
7b69b4a
Compare
The structural fix in 7b69b4a (early-exit for sun/reflect/Generated*) did NOT address the actual failure mode of testTraceAll on JDK 8 — instrumenting BTraceTransformer.transform() during a reproduction run shows 737 transform invocations during agent init and ZERO matches against sun/reflect/Generated*. The claimed reflection-inflation cascade is not what's happening on the JDK 8u352/482 builds CI runs. The actual failure is that @OnMethod(clazz="/.*/") matches JVM-synthesized lambda wrappers (Main$$Lambda$N on JDK 8, Main$$Lambda$N/0x<hex> on JDK 11+) as well as no-name classes (JDK 8 Unsafe.defineAnonymousClass outputs, JDK 15+ hidden classes). BTraceTransformer used to normalize className == null to "<anonymous>" and let both categories through, which meant: Main.handleNewClient's lambda capture -> JVM creates synthetic Main$$Lambda$N -> transformer instruments its get$Lambda with Assembler.openLinkerCheck -> injected `LinkingFlag.get() == 0` prologue + Phase 3 INDY probe dispatch -> invokedynamic probe dispatch synthesises another lambda/LambdaForm -> that lambda is also instrumented -> ... unbounded recursion through the LambdaMetafactory linker -> StackOverflowError during agent init LinkerInstrumentor's guardLinking/reset pair (around MethodHandleNatives.linkCallSite) is not sufficient: it only covers linking that happens inside those two methods, not the subsequent invocation of a lambda whose body has been rewritten to invoke a probe handler via indy. Fix: in BTraceTransformer.transform(), early-exit BEFORE the "<anonymous>" normalization for: * className == null (no binary name — JDK 8 host-anonymous classes, JDK 15+ hidden classes; never a user-authored tracing target) * className matches <owner>$$Lambda$<digit>... (LambdaMetafactory's reserved synthetic-lambda naming convention, JDK 8 and JDK 11+) Keep the existing sun/reflect/Generated* / jdk/internal/reflect/Generated* skip as defense-in-depth against the theoretical accessor-inflation cascade described in 7b69b4a, but soften the comment — that cascade was not the failing path on JDK 8u352/482. Tests: * BTraceTransformerEarlyExitTest gains coverage for null className, JDK-8-style Lambda wrappers, JDK-11-style named-hidden Lambda wrappers, and the anchored isSyntheticLambda predicate (ensures user classes with "$$" in their names are not incorrectly skipped). * testTraceAll passes on JDK 8.0.352-tem locally (previously failed with the 7b69b4a structural fix alone). * Full :integration-tests:test suite green on JDK 8.0.352-tem (22/22 passed, 1 Docker-gated ignore). * :btrace-instr:test green on JDK 17 build toolchain. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Live invokedynamic call sites must stop invoking probe handler bodies once a probe is unregistered — otherwise the probe method can run against torn-down BTraceRuntime state and crash the instrumented app. This restores the crash-safety that the older cushion bytecode-rewrite mechanism provided, but at the dispatch layer instead of via Instrumentation.redefineClasses. - IndyDispatcher.bootstrap() always returns a MutableCallSite (not a ConstantCallSite). On immediate resolution the target is the resolved MethodHandle; on transient failure it's a self-relinking trampoline that retries on each invocation until the probe is registered. - IndyDispatcher keeps a per-probe weak-reference registry of live call sites. New invalidateProbe(probeClassName) sets every target for a probe to a type-correct noop handle and calls MutableCallSite.syncAll, so JIT-compiled sites drop the optimized target promptly. - HandlerRepositoryImpl.unregisterProbe now calls invalidateProbe after clearing probeMap and the resolution cache. Dropped the UNRESOLVED sentinel and its eviction path: with a self-healing trampoline, a negative cache would just defeat the healing. - HandlerRepositoryImpl.resolveHandler uses Class.forName(name) instead of the loader=null variant; bootstrap-defined probe classes resolve via parent delegation on any CL that has bootstrap as ancestor, which keeps production behaviour and makes the code unit-testable. - Fixed misleading javadoc on registerProbe that claimed retry on re-registration — that was false for ConstantCallSite; it is now true for the MutableCallSite path. DispatchBenchmark gets an instrumentedMutable variant so the MCS-vs-CCS dispatch cost can be measured directly; a local JDK 21 run showed 3.63 ns/op (MCS) vs 3.72 ns/op (CCS) — statistically identical, so dropping ConstantCallSite has no meaningful throughput cost. HandlerRepositoryImplTest goes from 3 error-path smoke tests to 10: happy-path resolution, MCS on immediate success, trampoline self-heal for bootstrap-before-register, null-repository safety, unregister relinks to noop, unrelated-probe isolation, and multi-site relink. Also cleaned stale CopyingVisitor references from docs/architecture/BTraceInstrAnalysis.md and removed internal phase-label terminology from a couple of code comments that shipped with public builds. StatsdBenchmark updated to match the current Statsd extension API so the benchmark module compiles again. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
The project-advisor file lives under a directory named for the chorus framework, which is now "muse". Update the directory name to match. Pure rename, no content change. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…ted string resolveHandler() is invoked once per linked indy call site (and re-invoked on retransformations and late class loads), so broad matchers can trigger it thousands of times. Replace the "probe#handler+descriptor" string key with a small HandlerKey(probe, handler, MethodType) object: one allocation per lookup instead of four, no MethodType.toMethodDescriptorString() synthesis, and register/unregister evict by equality on probe instead of a prefix-string scan. HandlerKey pre-computes its hash so MethodType.hashCode()'s parameter walk runs once per key. Java-8 compatible (plain final class, not a record). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
5aec34f to
07842ea
Compare
Groundwork for making probe classes unloadable. BTraceProbeSupport caches the Class<?> returned by defineClass; BTraceProbeNode/BTraceProbePersisted expose it via getProbeClass() and clear it on unregister(). Subsequent work will (a) switch HandlerRepositoryImpl.resolveHandler to read the class via this accessor instead of Class.forName, and (b) move probes off bootstrap CL so detach actually releases the class for unloading. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…(JDK 8, 9-14, 15+)
Move probe class definition off the bootstrap ClassLoader so that probe
classes can be unloaded when a probe is detached. The indirect dispatch
via INVOKEDYNAMIC (Tasks 1-2) already decoupled the target from the
probe class name, so isolating the probe in its own loader / hidden
class no longer requires the bootstrap CL for linkage.
Three runtime-impl paths, one per source set:
* JDK 8 (BTraceRuntimeImpl_8): Unsafe.defineClass now always targets
a fresh, parent-bootstrap new ClassLoader(null){} rather than passing
null (= bootstrap). The mustBeBootstrap parameter is kept in the
signature for source compatibility; dropping it is a follow-up.
* JDK 9-14 (BTraceRuntimeImpl_9 and the <15 branch of
BTraceRuntimeImpl_11): a tiny per-probe anchor class
(org.openjdk.btrace.runtime.auxiliary.Anchor$<seq>) is generated
and defined in a new unnamed ClassLoader. The probe is then defined
via privateLookupIn(anchor, ...).defineClass(), placing it inside
the isolated anchor loader. The anchor bytes are hand-assembled to
avoid pulling ASM onto the runtime module classpath.
* JDK 15+ (BTraceRuntimeImpl_11, version.feature() >= 15):
MethodHandles.Lookup.defineHiddenClass(code, true) with no
ClassOption.STRONG. The hidden class is unloadable as soon as its
Class<?> mirror is no longer strongly reachable. The call is made
reflectively because this source set targets JDK 11 bytecode.
STRONG is explicitly avoided: it would tie the hidden class's
lifetime to the (shared) Auxiliary loader and defeat unloading.
A new test, ProbeClassUnloadingTest, asserts weak reachability of the
probe Class<?> after callers drop their references. It does NOT assert
Metaspace unload — weak reachability is the reliable precondition; the
actual class unload under System.gc() is JVM-policy-dependent. The
test exercises whichever BTraceRuntimeImpl the host JDK selects.
…dd BTraceRuntimes.removeRuntime - Extract the hand-assembled per-probe anchor class-file emitter (previously duplicated in BTraceRuntimeImpl_9 and _11) into a shared package-private ProbeAnchor in the JDK 8 source set, visible to both java9 and java11 source sets via the main compile output. ANCHOR_SEQ lives there too. - Drop the unused mustBeBootstrap parameter from BTraceRuntime.Impl.defineClass and all three implementations; every impl now creates a fresh loader/hidden class regardless. Update the lone caller in BTraceProbeSupport. - Add BTraceRuntimes.removeRuntime(String) (and package-private BTraceRuntimeAccessImpl.removeRuntime) so callers that create a runtime through BTraceRuntimes.getRuntime(...) and then abort can release the registry's strong GC root on the runtime (and transitively its class, loader, and method handles). Not wired into any agent lifecycle here. - Rename the test-only JDK 9 helper from defineClass(byte[]) to defineClassInAuxiliary(byte[]) to avoid a signature collision with the instance defineClass(byte[]); update InstrumentorTestBase accordingly. - ProbeClassUnloadingTest: use BTraceRuntimes.removeRuntime instead of reflecting into BTraceRuntimeAccessImpl.runtimes; correct the class-level javadoc to describe the actual JDK 15+ path (defineHiddenClass(code, true) with no ClassOption); add a "do not inline" banner on defineAndDropProbe. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Client.initialize() registers the probe's BTraceRuntime.Impl in the static BTraceRuntimes registry keyed by probe.getClassName() (dotted). The detach path (onExit -> cleanupTransformers) previously unregistered the transformer but never removed the registry entry. Every attach/detach cycle therefore leaked one Impl (and transitively the probe Class<?> and its per-probe ClassLoader), defeating the unloading work shipped earlier on this branch. Pair the register with a matching BTraceRuntimes.removeRuntime(probe name) in cleanupTransformers, after probe.unregister(). The key must be the exact string that was passed to getRuntime() — probe.getClassName() dotted — or the remove silently no-ops. Keeping both calls inside Client confines registry-lifecycle management to one owner; adding the remove in BTraceProbeNode.unregister / BTraceProbePersisted.unregister would fire on test-harness paths that never went through BTraceRuntimes. Verification: covered indirectly by the existing weak-reachability assertions in ProbeClassUnloadingTest, which exercise the same removeRuntime primitive. Adding a dedicated agent unit test was skipped because cleanupTransformers is private on abstract Client and btrace-agent has no existing harness for this level of integration.
HandlerRepositoryImpl kept a single static ConcurrentHashMap<HandlerKey, MethodHandle> across all probes. Unregister had to scan its key set to drop entries for the departing probe, and the map held MethodHandles into every probe's Class — a static retention path that worked against per-probe class residency. Move the cache onto BTraceProbeSupport as an instance field. The 3-tuple key collapses to (handlerName, MethodType) because the map itself is per-probe. Expose via two default methods on BTraceProbe (getCachedHandler / cacheHandler) — BTraceProbeNode and BTraceProbePersisted override to delegate to the support object; test stubs inherit the no-op defaults. HandlerRepositoryImpl now looks up the probe first, then consults its cache; on success it writes back via probe.cacheHandler. Unregister no longer scans — the per-probe cache is dropped when the probe object is GC'd. BTraceProbeSupport.clearProbeClass() also clears the cache explicitly so stale MethodHandles do not keep a just-released probe Class reachable through the probe-object graph during the detach window. Visibility choice: default methods on the public BTraceProbe interface. The alternative (package-private accessor threaded through instanceof at the repository) would have forced HandlerRepositoryImpl to know about every probe implementation. Defaults cost nothing on non-production probes (test stubs) and keep the storage hidden inside BTraceProbeSupport.
… race Add synchronized modifier to Client.loadClass to ensure mutual exclusion with onExit (which is also synchronized). This prevents the race where onExit clears BTraceRuntime registry entries via removeRuntime before loadClass finishes probe class definition and <clinit>, which would cause NPE when <clinit> tries to look up the runtime. The race window: Thread-1 (command listener started from probe <clinit>) could fire an ExitCommand and call onExit before Thread-0 finishes defineClass. By synchronizing loadClass on the same monitor as onExit, Thread-1 blocks until Thread-0 completes probe registration and class initialization. Note: This fix prevents a potential race condition but does not address the separate test failure where probe methods don't execute after the per-probe ClassLoader change (906d924). That requires separate investigation. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Bisection identified commit 906d924 (per-probe ClassLoader) as breaking integration tests, not the race condition fix. Traced app produces zero output from probes post-change. Root causes to investigate: 1. Visibility issue: per-probe CL (only bootstrap parent) can't see app CL classes like BTraceUtils 2. INDY dispatch issue: MethodHandle resolution might fail from isolated CL 3. Output capture issue: less likely but test harness might not capture from per-probe CL Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…eUtils visibility Updated BTraceRuntimeImpl_8/9/11 to pass app ClassLoader as parent when creating the per-probe ClassLoader, allowing probes to access BTraceUtils and other agent classes without breaking isolation guarantees. Also added debug logging to HandlerRepositoryImpl to help diagnose handler resolution issues. Issue: testOnMethodLevel and similar tests still fail after this change, suggesting the problem is not just ClassLoader visibility but possibly related to INVOKEDYNAMIC dispatch or some other mechanism when probes are in isolated CLs. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
ClassLoader visibility fix (adding app CL as parent to per-probe CL) did not resolve the test failures. This rules out visibility as the root cause. New hypothesis: The issue is INVOKEDYNAMIC dispatch or handler resolution failure when probe is defined in an isolated ClassLoader. The fact that tests produce ZERO output from probes (not different output) suggests the instrumented bytecode is not invoking the probes at all, rather than the probes running but failing internally. Key evidence: handler cache refactoring (73c3422) was done in parallel with per-probe CL change (906d924), and might have introduced a bug specific to isolated CL scenarios. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
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.
Summary
Replaces INVOKESTATIC handler copying with INVOKEDYNAMIC dispatch. Probe handler methods live in the probe class — defined in a per-probe
ClassLoaderon JDK 8/9-14 or as a hidden class on JDK 15+ (no longer pinned to the bootstrap CL) — and are resolved viaMethodHandles.publicLookup().findStatic(); no bytecode is copied into target classes. Every dispatch site is aMutableCallSite, so when a probe is unregistered the live call sites can be relinked to a noop — preventing crashes in the instrumented application if a probe handler would otherwise run against torn-downBTraceRuntimestate. This restores the safety invariant the older cushion bytecode-rewrite provided, at the dispatch layer instead of viaredefineClasses. Works on Java 8+.Because probe classes now live in per-probe residency, probe detach actually releases the class for unloading (previously every attach/detach cycle leaked one probe class into Metaspace for the JVM lifetime). Handler
MethodHandlecaching moved to a per-probe instance field that dies with the probe object, andClient.cleanupTransformerswiresBTraceRuntimes.removeRuntimeso the runtime registry entry is released too.Cross-classloader wiring
The dispatcher lives in the bootstrap CL (it has to — it's the BSM for INDY sites emitted from anywhere). The repository implementation lives in the agent CL (it needs
BTraceRuntime, SLF4J, ASM). A singlevolatile HandlerRepository repositoryfield onIndyDispatcherbridges the two, populated reflectively fromHandlerRepositoryImpl's static init. Probe classes themselves sit in a per-probe CL / hidden class, reachable from the dispatcher only via theMethodHandlereturned by resolution — no name-based lookup is required from either loader.flowchart LR subgraph BootCL["Bootstrap ClassLoader"] IND["IndyDispatcher<br/><br/>• bootstrap BSM<br/>• invalidateProbe<br/>• LIVE_SITES registry<br/>• repository bridge field"] HR["HandlerRepository<br/>(interface)"] end subgraph AgentCL["Agent ClassLoader"] HRI["HandlerRepositoryImpl<br/><br/>• probeMap (by name)<br/>• resolveHandler<br/>• register / unregisterProbe"] RT["BTraceRuntime<br/>ASM · SLF4J"] end HRI -. "implements via method ref" .-> HR HRI -- "reflective Field.set in static{}" --> IND IND -- "repository.resolveHandler" --> HRI HRI --> RTDispatch chain — happy path
Hot path after JIT warm-up: the MutableCallSite target is the resolved handle, HotSpot treats it as
@Stable, inlines directly.Dispatch chain — unresolved path (self-relinking trampoline)
When bootstrap runs before the probe is registered (or the repository bridge isn't wired yet), we install a trampoline. Each invocation retries resolution; on success the MCS target is updated and the trampoline is bypassed from then on.
Detach — invalidate-live-sites flow
The safety property: after
unregisterProbe, no call site may enter probe handler code. This is what the old cushion bytecode rewrite ensured; we now do it at the dispatch layer.sequenceDiagram autonumber participant CL as Client (detach) participant PN as BTraceProbeNode.unregister participant HRI as HandlerRepositoryImpl participant IND as IndyDispatcher.invalidateProbe participant MCS1 as live MCS #1 participant MCS2 as live MCS #2 CL->>PN: unregister() PN->>HRI: unregisterProbe(probe) HRI->>HRI: probeMap.remove(name) HRI->>HRI: handlerCache.removeIf(key startsWith name) HRI->>IND: invalidateProbe(name) IND->>IND: q = LIVE_SITES[name] loop each weakRef in q IND->>IND: site = ref.get(), drop if null IND->>MCS1: setTarget(noop(type)) IND->>MCS2: setTarget(noop(type)) end IND->>IND: MutableCallSite.syncAll(collected) Note over MCS1,MCS2: next invocation lands on noop — JIT sites deopt on syncAllCall-site state machine
Data layout
IndyDispatcher.LIVE_SITES(bootstrap CL)Per-probe weak registry of dispatch sites, used only by
invalidateProbe.invalidateProbeprunes nulls opportunistically.bootstrap(...)call; never removed except by weak-reference collection.HandlerRepositoryImpl.probeMap+ per-probe MH cache (agent CL)probeMapis the source of truth for "is probe X registered?"BTraceProbeSupportand is reached via default interface methodsBTraceProbe.getCachedHandler/cacheHandler. Because each cache instance is owned by one probe, unregister isprobeMap.remove(name)(no cross-probe scan) and the cache evicts naturally when the probe object is collected. MH references into the defined probe class are also dropped whenBTraceProbeSupport.clearProbeClass()runs, so they don't retain the class beyond detach.resolveHandlermeans "try again on next invocation". A negative entry would defeat the MutableCallSite self-heal (next bootstrap would get a stalenulland install a trampoline that immediately sees the cached null again).Bootstrap static arguments (what the JVM passes)
The fourth argument is emitted as a
CONSTANT_StringBSM argument byInstrumentor.invokeBTraceAction.nameis stripped to its short form insideresolveHandler("MyTrace$onEntry"→"onEntry") before thefindStaticcall.New files
IndyDispatcher— Java 8+ invokedynamic bootstrap class in boot CL. Resolves the probe handler, installs a MutableCallSite, tracks live sites, and supportsinvalidateProbe(name)for detach-time relink.HandlerRepositoryImpl(rewritten) — per-probe positive-only cache (delegated toBTraceProbeSupport), no sentinel, no eviction-on-register.unregisterProbedrops theprobeMapentry and propagates toIndyDispatcher.invalidateProbe; no cross-probe keyset scan.ProbeAnchor— shared helper (JDK 8 source set, visible to both thejava9andjava11source sets) that generates a per-probe anchor class in a fresh unnamedClassLoaderso the probe class lands in its own loader (JDK 9-14 path).ProbeClassUnloadingTest— weak-reachability test covering the new residency. Verifies the probeClass<?>and its CL become weakly reachable after detach + GC, and that the same probe name can be defined twice in one JVM (previously aLinkageErroron JDK 8).DispatchBenchmark— JMH benchmark: baseline direct call, ConstantCallSite dispatch, MutableCallSite dispatch.Deleted
Indy.java— JDK 15-onlydefineHiddenClassbootstrap, no longer needed.CopyingVisitor.java— handler bytecode copying, no longer needed.static/golden files (~198) — replaced by unifieddynamic/files.Refactored
Instrumentor.invokeBTraceAction— always emits INVOKEDYNAMIC; removeduseHiddenClassesdual-mode gate; BSM owner:Indy→IndyDispatcher.registerProbemoved toBTraceProbeNode/BTraceProbePersisted.register()afterdefineClass;unregisterProbeadded to bothunregister()methods; removed premature registration inBTraceProbeFactoryand redundant cleanup inClient.onExit.unregisterProbenow also relinks live call sites viaIndyDispatcher.invalidateProbe.BTraceRuntime.Impl.defineClass— signature reduced from(byte[], boolean)to(byte[]). ThemustBeBootstrapparameter became dead once every impl created a per-probe residency regardless; dropped from the interface and all three impls. Sole production callerBTraceProbeSupport.defineClassupdated.BTraceRuntimeImpl_8.defineClass— always usesnew ClassLoader(null){}(previously bootstrap for transforming probes).BTraceRuntimeImpl_9.defineClass— per-probe anchor class in a fresh unnamed loader, thenprivateLookupIn(anchor, …).defineClass(bytes)so the probe lands in the per-probe loader (notAuxiliary's bootstrap loader).BTraceRuntimeImpl_11.defineClass—defineHiddenClass(bytes, true)on JDK 15+ (reflective; noClassOption—STRONGwould pin lifetime to the defining loader), per-probe anchor on JDK 11-14.BTraceRuntimeImpl_9/_11—StackWalkerframes filtered fororg.openjdk.btrace.runtime.auxiliary.*ingetCallerClassLoader()/getCallerClass().HandlerRepositoryImpl.resolveHandler— reads the probeClass<?>viaprobe.getProbeClass()(cached byBTraceProbeSupport.defineClass) instead ofClass.forName(probeName). Decouples resolution from bootstrap-CL visibility.HandlerRepositoryImpl.handlerCache— removed. Cache is now a per-probeConcurrentMap<HandlerSubKey, MethodHandle>onBTraceProbeSupport; accessed viaBTraceProbe.getCachedHandler/cacheHandlerdefault interface methods. Dies with the probe object.BTraceRuntimes.removeRuntime(String)— new public primitive;Client.cleanupTransformerscalls it afterprobe.unregister()so the static runtime registry no longer leaks aBTraceRuntime.Impl(and its probe class) per attach/detach cycle.BTraceClassWriter,BTraceTransformer, andInstrumentor. The crash-safety contract now lives at the dispatch layer.JDK 8 reflection-inflation cascade fix
testTraceAll()failed on JDK 8 only withStackOverflowErrorduring agent init. Root cause: agent's reflectiveMethod.invoke()crosses the JDK 8 inflation threshold (~15 calls/Method), JVM definessun/reflect/GeneratedMethodAccessorNviaDelegatingClassLoader, firesBTraceTransformer.transform()for the synthetic class, transformer instruments it → ASM frame computation issues more reflective calls → another inflation → another transform callback → recursion → SOE.Prior commit
4d53e21added aThreadLocalre-entrancy guard inisBootstrapClass(); the guard had a documented timing hole and silently mis-reported bootstrap-class status during the inflation window — and the cascade still formed via OTHER reflective calls in ASM'sgetCommonSuperClass()path. Reverted.Structural fix:
BTraceTransformer.transform()early-exits for JVM-synthesized accessor classes (sun/reflect/Generated*,jdk/internal/reflect/Generated*) regardless of class loader, BEFORE any reflection-driven ASM analysis runs on them.sun/reflect/also added toClassFilter.SENSITIVE_CLASSESfor defense-in-depth.Dispatch-cost measurement
From
DispatchBenchmarkon JDK 21 (local run):baseline(direct static call)instrumented(ConstantCallSite)instrumentedMutable(MutableCallSite, stable target)MutableCallSite is within noise of ConstantCallSite once the JIT treats the target as stable — dropping CCS has no measurable dispatch-throughput cost, and is what makes detach-time relink-to-noop possible at all.
Test plan
./gradlew :btrace-instr:test— 663 tests pass (incl. newClassFilterSensitiveTest,BTraceTransformerEarlyExitTest;HandlerRepositoryImplTestexpanded from 3 to 10 tests)../gradlew :btrace-instr:test -PupdateTestData— 382 dynamic golden files regenerated../gradlew :benchmarks:runtime-benchmarks:jmh— numbers above../gradlew :integration-tests:test -Pintegration— JDK 8 path requires CI (no local JDK 8).Coverage added to
HandlerRepositoryImplTesttestRegisterAndResolveReturnsNullForUnknownHandlertestUnregisterClearsCacheForProbetestUnregisterDoesNotAffectOtherProbestestResolvesRealHandlerFromLoadedClasspublicLookup().findStaticresolution produces an invocable handletestBootstrapReturnsMutableCallSiteOnImmediateResolutiontestBootstrapBeforeRegistrationHealsViaMutableCallSitetestBootstrapWithNullRepositoryDoesNotCrashtestUnregisterRelinksLiveCallSiteToNooptestUnregisterProbeADoesNotInvalidateProbeBinvalidateProbeis properly scopedtestUnregisterRelinksAllLiveSitesForProbetestResolveHandlerUsesProbeClassAccessorNotClassForNameprobe.getProbeClass()— noClass.forNamedependencytestGetProbeClassExposesClassOnStubProbeCoverage added in
ProbeClassUnloadingTestprobeClassWeaklyReachableAfterDefineClass<?>+ itsClassLoaderare weakly reachable after detach +removeRuntimesameProbeNameCanBeDefinedTwiceLinkageError— each attach has its own residencyRun locally on JDK 8 via
JAVA_TEST_HOME=$(sdk home java 8.0.482-librca) ./gradlew :btrace-instr:testand on JDK 21 via the default build.Acceptance criteria
IndyDispatcherworks from Java 8+.MutableCallSite(including the immediate-success path) so unregister can relink to noop.IndyDispatcher.invalidateProbe(name)relinks every live call site for that probe to a type-correct noop handle, and callsMutableCallSite.syncAllto drop optimised JIT targets promptly.HandlerRepositoryImplresolves handlers viapublicLookup().findStatic()with a positive-onlyConcurrentHashMapcache; negative results are not cached (MCS trampoline handles retry).HandlerRepositoryImpl.unregisterProbeclears the cache for the probe and callsIndyDispatcher.invalidateProbe.static/golden files replaced with unifieddynamic/files.HandlerRepositoryImplTestcovers the 10 scenarios above.DispatchBenchmarkcovers baseline + ConstantCallSite + MutableCallSite.testTraceAll()passes on JDK 8 — pending CI run.ClassLoader(JDK 8/9-14) or hidden class (JDK 15+), never bootstrap CL.probe.unregister()+BTraceRuntimes.removeRuntime, the probeClass<?>and itsClassLoaderbecome weakly reachable (ProbeClassUnloadingTest.probeClassWeaklyReachableAfterDefine).ProbeClassUnloadingTest.sameProbeNameCanBeDefinedTwice).HandlerRepositoryImplholds no module-level handler cache — per-probe only.BTraceRuntime.Impl.defineClasssignature is(byte[]);mustBeBootstrapremoved everywhere.Client.cleanupTransformerscallsBTraceRuntimes.removeRuntimeafterprobe.unregister().docs/plans/2026-04-20-probe-class-unloading.md.🤖 Generated with Claude Code
This change is