Bound class-map growth to fix reliability-chaos OOMs#654
Merged
Conversation
Profiler::_class_map grows unbounded under sustained distinct-class churn since rotate() carries the full accumulated set forward each JFR chunk. Cap it via bounded_lookup(), drop liveness samples instead of wrapping -1 into the u32 event id, and add a counter for dropped samples.
![dd-octo-sts[bot]](https://avatars.githubusercontent.com/in/1157446?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff0e37aaef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Pull request overview
Bounds the profiler’s class-name dictionary growth to prevent container OOMs under workloads with unbounded dynamic class churn, and fixes a correctness issue where a failed class lookup (-1) could wrap into the unsigned JFR event id.
Changes:
- Cap
Profiler::_class_mapgrowth by switchinglookupClass()toStringDictionary::bounded_lookup(..., MAX_CLASS_MAP_SIZE)and returning-1once the cap is reached (with a newCLASS_MAP_AT_CAPACITYcounter). - Guard
LivenessTracker::flush_table()to drop samples whenlookupClass()returns-1to avoidu32wraparound corruption. - Add/extend C++ unit tests for the bounded dictionary behavior and the negative-class-id guard scenario.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ddprof-lib/src/main/cpp/profiler.h | Introduces MAX_CLASS_MAP_SIZE cap for the class-name dictionary. |
| ddprof-lib/src/main/cpp/profiler.cpp | Uses bounded lookup for class names, increments a new counter, and returns -1 on failure. |
| ddprof-lib/src/main/cpp/livenessTracker.cpp | Drops liveness samples when class id is negative to prevent u32 wraparound. |
| ddprof-lib/src/main/cpp/counters.h | Adds CLASS_MAP_AT_CAPACITY counter definition. |
| ddprof-lib/src/test/cpp/stringDictionary_ut.cpp | Adds a unit test validating size-limit behavior of bounded_lookup. |
| ddprof-lib/src/test/cpp/livenessTracker_ut.cpp | Adds tests documenting guarded vs. unguarded negative class-id narrowing behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
CI Test ResultsRun: #29270148867 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-07-13 17:39:53 UTC |
Contributor
Benchmark Results (commit ff0e37a)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124284718 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10172 ms (21 iters) | ✅ 10239 ms (21 iters) | ≈ +0.7% (±10.9%) | — / — |
| akka-uct | 25 | ✅ 8861 ms (24 iters) | ✅ 8906 ms (24 iters) | ≈ +0.5% (±9.8%) | — / — |
| finagle-chirper | 21 | ✅ 5991 ms (33 iters) | ✅ 5981 ms (33 iters) | ≈ -0.2% (±24.5%) | |
| finagle-chirper | 25 | ✅ 5453 ms (36 iters) | ✅ 5473 ms (36 iters) | ≈ +0.4% (±24.3%) | |
| fj-kmeans | 21 | ✅ 2709 ms (69 iters) | ✅ 2820 ms (66 iters) | 🔴 +4.1% | — / — |
| fj-kmeans | 25 | ✅ 2824 ms (66 iters) | ✅ 2811 ms (67 iters) | ≈ -0.5% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2119 ms (88 iters) | ✅ 2065 ms (91 iters) | 🟢 -2.5% | — / — |
| future-genetic | 25 | ✅ 2069 ms (90 iters) | ✅ 2067 ms (90 iters) | ≈ -0.1% (±2.7%) | — / — |
| naive-bayes | 21 | ✅ 1255 ms (136 iters) | ✅ 1252 ms (136 iters) | ≈ -0.2% (±32.4%) | — / — |
| naive-bayes | 25 | ✅ 987 ms (172 iters) | ✅ 1015 ms (168 iters) | ≈ +2.8% (±32%) | — / — |
| reactors | 21 | ✅ 16222 ms (15 iters) | ✅ 16143 ms (15 iters) | ≈ -0.5% (±6%) | — / — |
| reactors | 25 | ✅ 18392 ms (15 iters) | ✅ 18098 ms (15 iters) | ≈ -1.6% (±4.3%) | — / — |
Internal counter details (ddprof)
ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
| Benchmark | JDK | Dropped rec | Dropped jvmti | Dropped trace | Skipped WC | AGCT fail | Unwind fail |
|---|---|---|---|---|---|---|---|
| akka-uct | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 1962 / 2002 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 2398 / 2296 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 4 / 2 | 8858 / 8495 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 1 | 8731 / 8269 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 1265 / 1253 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 3 / ✅ | 1276 / 1268 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 2977 / 3050 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 2888 / 2850 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 2 | 3488 / 3473 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 3456 / 3457 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 1584 / 1669 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1853 / 1794 | ✅ / ✅ | ✅ / ✅ |
Contributor
Benchmark Results (commit a8733ae)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124292141 Commit: ✅ Within expected boundariesNo significant runtime deltas (all within run-to-run noise) and no internal-counter outliers. Runtime details (per benchmark × JDK)
Internal counter details (ddprof)ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
|
This comment has been minimized.
This comment has been minimized.
lookupDuringDump() had no size check, so the JFR dump path could still grow _class_map past the cap Profiler::lookupClass() enforces at sample time.
Contributor
Benchmark Results (commit e3224fd)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124307330 Commit: ✅ Within expected boundariesNo significant runtime deltas (all within run-to-run noise) and no internal-counter outliers. Runtime details (per benchmark × JDK)
Internal counter details (ddprof)ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
|
Contributor
Reliability & Chaos Results✅ All reliability & chaos checks passed Pipeline: https://gitlab.ddbuild.io/DataDog/java-profiler/-/pipelines/124366851 |
thread-churn/dump-storm antagonists cycle many short-lived threads; tcmalloc's defaults are slow to return per-thread caches to the OS, inflating container RSS past the OOM limit on aarch64. Reproduced under a 3GB memory.max cgroup on aarch64 (tcmalloc, profiler+tracer, JDK 25.0.3, 120s): baseline OOM-killed (oom_kill=1, RC=137); with TCMALLOC_RELEASE_RATE=10 + TCMALLOC_AGGRESSIVE_DECOMMIT=1 the same run completes cleanly (RC=0).
Contributor
Benchmark Results (commit 3397ec1)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124366869 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10328 ms (21 iters) | ✅ 10289 ms (21 iters) | ≈ -0.4% (±12%) | — / — |
| akka-uct | 25 | ✅ 8839 ms (24 iters) | ✅ 8977 ms (24 iters) | ≈ +1.6% (±9.5%) | — / — |
| finagle-chirper | 21 | ✅ 6015 ms (33 iters) | ✅ 6007 ms (33 iters) | ≈ -0.1% (±25.2%) | |
| finagle-chirper | 25 | ✅ 5496 ms (36 iters) | ✅ 5436 ms (36 iters) | ≈ -1.1% (±24.5%) | |
| fj-kmeans | 21 | ✅ 2696 ms (70 iters) | ✅ 2845 ms (66 iters) | 🔴 +5.5% | — / — |
| fj-kmeans | 25 | ✅ 2826 ms (66 iters) | ✅ 2819 ms (66 iters) | ≈ -0.2% (±2.5%) | — / — |
| future-genetic | 21 | ✅ 2069 ms (90 iters) | ✅ 2099 ms (90 iters) | ≈ +1.4% (±2.6%) | — / — |
| future-genetic | 25 | ✅ 2000 ms (93 iters) | ✅ 2087 ms (90 iters) | 🔴 +4.4% | — / — |
| naive-bayes | 21 | ✅ 1226 ms (139 iters) | ✅ 1256 ms (136 iters) | ≈ +2.4% (±32.8%) | — / — |
| naive-bayes | 25 | ✅ 977 ms (174 iters) | ✅ 1024 ms (167 iters) | ≈ +4.8% (±32.9%) | — / — |
| reactors | 21 | ✅ 16384 ms (15 iters) | ✅ 16737 ms (15 iters) | ≈ +2.2% (±8.6%) | — / — |
| reactors | 25 | ✅ 18760 ms (15 iters) | ✅ 18803 ms (15 iters) | ≈ +0.2% (±4.9%) | — / — |
Internal counter details (ddprof)
ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
| Benchmark | JDK | Dropped rec | Dropped jvmti | Dropped trace | Skipped WC | AGCT fail | Unwind fail |
|---|---|---|---|---|---|---|---|
| akka-uct | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 2 | 1989 / 1899 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 2193 / 2216 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 5 / 2 | 8733 / 8665 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 8400 / 8231 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 4 | 1293 / 1266 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 8 | 1287 / 1263 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 2914 / 3021 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 2845 / 2962 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 4 / 4 | 3538 / 3496 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 8 / 6 | 3491 / 3500 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 1689 / 1697 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 1961 / 1945 | ✅ / ✅ | ✅ / ✅ |
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.
What does this PR do?:
Bounds
Profiler::_class_mapgrowth viabounded_lookup(key, len, size_limit)so a workload with unbounded distinct-class churn (e.g. dynamic class generation) can no longer grow the class-name dictionary for the whole life of the process.Profiler::lookupClassnow returns-1once the cap (MAX_CLASS_MAP_SIZE) is hit, and increments a newCLASS_MAP_AT_CAPACITYcounter.LivenessTracker::flush_tablenow drops the sample instead of narrowing-1into theu32event id (which would have wrapped to0xFFFFFFFFand corrupted liveness attribution), mirroringObjectSampler's existing convention for the samelookupClass()failure mode.Also tunes the
reliability-chaos-aarch64CI job's tcmalloc allocator settings (TCMALLOC_RELEASE_RATE=10,TCMALLOC_AGGRESSIVE_DECOMMIT=1) to fix a separate, allocator-specific OOM that kept recurring after the class-map fix landed.Motivation:
reliability-chaos-*CI jobs were hitting frequent OOMs (exit 137/OOMKilled). Root cause:_class_mapis aStringDictionarywhoserotate()carries the full accumulated key set forward on every JFR chunk (it only shrinks viaclearAll()on profiler restart), so the chaos suite's class-churn antagonists (which mint ~1000 unique class names/sec) grow it without bound until the container OOMs.A second, unrelated OOM kept failing
reliability-chaos-aarch64(tcmalloc, profiler+tracer, JDK 25.0.3) even after the class-map fix.libtcmalloc-minimal4's defaults are slow to return per-thread caches to the OS on aarch64, and the chaos suite'sthread-churn/dump-stormantagonists cycle many short-lived threads over the run, inflating container RSS past the limit independent of any application-level leak. Confirmed on a real aarch64 host under a 3GBmemory.maxcgroup: baseline OOM-killed (oom_kill=1,RC=137); withTCMALLOC_RELEASE_RATE=10+TCMALLOC_AGGRESSIVE_DECOMMIT=1the identical run completes cleanly (RC=0).Additional Notes:
None.
How to test the change?:
Added unit tests:
stringDictionary_ut.cpp::BoundedLookupWithSizeLimitRejectsNewKeyAtCapacity— exercises the 3-argbounded_lookup(key, len, size_limit)at-capacity behavior.livenessTracker_ut.cpp::NegativeClassIdSampleIsDropped/NonNegativeClassIdSampleIsRecorded/UnguardedNegativeClassIdWrapsToMaxU32— cover the drop-on--1guard and the wraparound bug it prevents.Ran via
gtestDebug_stringDictionary_utandgtestDebug_livenessTracker_ut, all passing.For the tcmalloc tuning: reproduced the aarch64 chaos OOM on a real host under a 3GB
memory.maxcgroup (baseline OOM-killed, tuned run completes cleanly withRC=0); final validation is the next scheduledreliability-chaos-aarch64pipeline run.For Datadog employees:
credentials of any kind, I've requested a security review (run the
dd:platform-security-reviewskill, or file a request via the PSEC review form).
bewairealso runs automatically on every PR.Unsure? Have a question? Request a review!