Skip to content

Commit f2d8978

Browse files
jbachorikclaude
jbachorik
and
claude
committed
Fix LivenessTracker forced-cleanup gate silently dropping population samples
cleanup_table()'s per-klass population accounting was gated on !forced, skipping it entirely whenever track()'s table-overflow branch triggered a forced sweep. Gate on a new is_epoch_owner check instead (true once per genuinely new GC epoch, forced or not), so a forced sweep still folds a sample using an already-cached klass id. resolveKlassId() (a real Class.getName() Java upcall) still only runs on the organic path - that part was a legitimate hot-path cost concern, just misdescribed as a JVMTI safety requirement. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent a636398 commit f2d8978

2 files changed

Lines changed: 66 additions & 35 deletions

File tree

‎ddprof-lib/src/main/cpp/livenessTracker.cpp‎

Lines changed: 57 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -37,10 +37,18 @@ void LivenessTracker::cleanup_table(bool forced) {
3737
forced, _gc_generations, (unsigned long long)current,
3838
(unsigned long long)target_gc_epoch, _table_size);
3939

40-
if ((target_gc_epoch == _last_gc_epoch ||
41-
!__atomic_compare_exchange_n(&_last_gc_epoch, &current,
42-
target_gc_epoch, false, __ATOMIC_RELAXED, __ATOMIC_RELAXED)) &&
43-
!forced) {
40+
// is_epoch_owner is true iff this call is the one that moves _last_gc_epoch
41+
// to target_gc_epoch - i.e. the first cleanup_table() call (forced or not)
42+
// to observe this particular GC epoch transition. Population accounting
43+
// below is gated on this rather than on !forced, so a forced (table-
44+
// overflow) sweep still folds one sample per genuinely new epoch instead
45+
// of either skipping it entirely or double-counting the same epoch across
46+
// repeated forced sweeps.
47+
bool is_epoch_owner = target_gc_epoch != current &&
48+
__atomic_compare_exchange_n(&_last_gc_epoch, &current, target_gc_epoch,
49+
false, __ATOMIC_RELAXED, __ATOMIC_RELAXED);
50+
51+
if (!is_epoch_owner && !forced) {
4452
// if the last processed GC epoch hasn't changed, or if we failed to update
4553
// it, there's nothing to do
4654
TEST_LOG("LivenessTracker::cleanup_table early-exit: epoch unchanged and not forced");
@@ -69,35 +77,52 @@ void LivenessTracker::cleanup_table(bool forced) {
6977
}
7078
_table[target].age += epoch_diff;
7179

72-
if (_gc_generations && !forced) {
80+
if (_gc_generations && is_epoch_owner) {
7381
// Per-klass population tracking (design doc's Open Question 3) -
74-
// gated on _gc_generations so this new cost (a GetObjectClass +
75-
// Class.getName() + StringDictionary lookup per surviving entry,
76-
// previously paid only at JFR-flush time, see flush_table() below)
77-
// is paid only when the caller actually asked for generation/
78-
// survival-shaped data (arguments.cpp:223-227,244), not for every
79-
// liveness-tracking session. Also gated on !forced: track()'s
80-
// table-overflow branch calls cleanup_table(true) synchronously
81-
// from the allocation-sampling call stack (JVMTI SampledObjectAlloc
82-
// callback), and this class resolution must stay off that path -
83-
// see this file's header comment on _klass_population ("never from
84-
// track()"). The non-forced callers (flush_table(), stop()) only
85-
// run on the JFR-flush/profiler-stop cadence, never from track().
86-
jobject ref = env->NewLocalRef(_table[target].ref);
87-
if (ref != nullptr) {
88-
u32 klass_id = resolveKlassId(env, ref);
89-
if (klass_id != 0) {
90-
accumulateKlassCount(klass_id, _table[target].ref);
91-
// Cache the resolution: flush_table() runs its own
92-
// GetObjectClass+Class.getName()+lookupClass() sequence for
93-
// every surviving entry immediately after cleanup_table()
94-
// returns (flush_table() always calls cleanup_table() first),
95-
// which would otherwise repeat this exact JNI round-trip for
96-
// the same object. An object's class is immutable, so this
97-
// value stays valid for flush_table()'s read below.
98-
_table[target].cached_klass_id = klass_id;
82+
// gated on _gc_generations so this new cost is paid only when the
83+
// caller actually asked for generation/survival-shaped data
84+
// (arguments.cpp:223-227,244), not for every liveness-tracking
85+
// session. Gated on is_epoch_owner (not !forced) so a forced
86+
// (table-overflow) sweep still contributes one population sample
87+
// per genuinely new GC epoch instead of silently dropping it.
88+
u32 klass_id = 0;
89+
if (!forced) {
90+
// GetObjectClass + Class.getName() + StringDictionary lookup per
91+
// surviving entry, previously paid only at JFR-flush time (see
92+
// flush_table() below). Only affordable on this, the organic
93+
// GC-driven cleanup path (flush_table()/stop()'s cadence).
94+
jobject ref = env->NewLocalRef(_table[target].ref);
95+
if (ref != nullptr) {
96+
klass_id = resolveKlassId(env, ref);
97+
if (klass_id != 0) {
98+
// Cache the resolution: flush_table() runs its own
99+
// GetObjectClass+Class.getName()+lookupClass() sequence for
100+
// every surviving entry immediately after cleanup_table()
101+
// returns (flush_table() always calls cleanup_table() first),
102+
// which would otherwise repeat this exact JNI round-trip for
103+
// the same object. An object's class is immutable, so this
104+
// value stays valid for flush_table()'s read below, and for
105+
// a later forced sweep's read right below.
106+
_table[target].cached_klass_id = klass_id;
107+
}
108+
env->DeleteLocalRef(ref);
99109
}
100-
env->DeleteLocalRef(ref);
110+
} else {
111+
// track()'s table-overflow branch calls cleanup_table(true)
112+
// synchronously from the allocation-sampling call stack (JVMTI
113+
// SampledObjectAlloc callback). resolveKlassId() calls
114+
// Class.getName(), a genuine Java-bytecode upcall (unlike the
115+
// plain native jvmti->GetClassSignature() call
116+
// ObjectSampler::recordAllocation already makes on this same
117+
// callback stack) - too costly, and too re-entrancy-prone via
118+
// the String allocation it can trigger, to run from there. Reuse
119+
// whatever class id an earlier organic epoch already resolved
120+
// for this entry instead; if it was never resolved, this entry's
121+
// sample for this epoch is dropped rather than resolving now.
122+
klass_id = _table[target].cached_klass_id;
123+
}
124+
if (klass_id != 0) {
125+
accumulateKlassCount(klass_id, _table[target].ref);
101126
}
102127
}
103128
} else {
@@ -112,7 +137,7 @@ void LivenessTracker::cleanup_table(bool forced) {
112137

113138
TEST_LOG("LivenessTracker::cleanup_table survivors=%u klass_count_scratch_size=%d",
114139
newsz, _klass_count_scratch_size);
115-
if (_gc_generations && !forced && _klass_count_scratch_size > 0) {
140+
if (_gc_generations && is_epoch_owner && _klass_count_scratch_size > 0) {
116141
foldKlassCountsLocked(env, target_gc_epoch);
117142
}
118143

‎ddprof-lib/src/main/cpp/livenessTracker.h‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -174,9 +174,15 @@ class alignas(alignof(SpinLock)) LivenessTracker {
174174
// force=true is used by track()'s table-overflow branch to run a cleanup
175175
// synchronously from the allocation-sampling call stack, bypassing the
176176
// GC-epoch-changed check below. The per-klass population tracking below
177-
// (_gc_generations) only runs when force is false, i.e. from flush_table()/
178-
// stop()'s cadence, never from that forced/hot-path call - see this
179-
// method's own "!forced" checks in livenessTracker.cpp.
177+
// (_gc_generations) runs on both paths, once per genuinely new GC epoch
178+
// (see "is_epoch_owner" in livenessTracker.cpp), but resolveKlassId() - a
179+
// real Class.getName() Java-bytecode upcall, unlike the plain native JVMTI
180+
// calls already made elsewhere on this same callback stack - is only ever
181+
// invoked when force is false, i.e. from flush_table()/stop()'s cadence;
182+
// too costly/re-entrancy-prone to pay on the hot path. A forced sweep
183+
// instead reuses whatever cached_klass_id an entry already picked up from
184+
// an earlier organic epoch, or skips accounting for that entry this epoch
185+
// if it was never resolved.
180186
void cleanup_table(bool force = false);
181187

182188
void flush_table(std::set<int> *tracked_thread_ids);

0 commit comments

Comments
 (0)