SymDB: restore benchmarks#5798
Conversation
p-datadog
added
the
AI Generated
![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: 1445da9a6f
ℹ️ 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".
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: bbf03fa | Docs | Datadog PR Page | Give us feedback! |
|
Is it just a plain revert? |
|
Yes this needs a fix. |
p-datadog
changed the title
This reverts commit 50fc341.
Adds stderr trace lines around each phase of run() and inside run_arm's bg-thread stop logic. Local validate run is 3.09s; CI hits the 10s expect_in_fork timeout. The traces will identify which phase grows on CI. To revert: this commit only modifies symbol_database_baseline_matrix.rb.
expect_in_fork captures stderr into a tempfile and discards it on success. The baseline_matrix bench's per-phase trace (added in the previous commit) is therefore invisible in CI logs when the spec passes. This commit overrides fork_expectations to print the captured stderr before asserting, and bumps timeout_seconds to 60 so a slow runner doesn't truncate the trace. Revert with the previous diagnostic commit once CI timing is understood.
Revert "DIAGNOSTIC: surface bench fork stderr + bump timeout to 60s" Revert "DIAGNOSTIC: trace timing in baseline_matrix to locate CI >10s phase" Diagnostic data collected; replacing with the real fix in the next commit.
The baseline_matrix benchmark's validate-mode run was approaching the
10s expect_in_fork timeout on slow CI runners. Diagnostic traces over
two CI runs measured 6.4s and 8.7s wall, with the entire variance and
~80% of the time coming from work that is only meaningful in real
measurement mode:
- calibrate_cpu_chunk runs 8 cpu_chunks (~580ms on CI) to find the
per-host wall duration. Validate mode doesn't measure anything, so
return a placeholder 0.01s instead.
- 3.times { GC.start } between arms (8 invocations × 0.5-0.9s each
on CI = 4-6s) stabilizes heap state between measured runs. Validate
mode skips this entirely.
- bg.join(0.5) caps how long we wait for the bg extract_all thread
after the measurement window. Three with_extractor arms × ~0.48s on
CI = ~1.5s. Shrink to 0.05 in validate mode since the bg thread is
doing token work on 10 classes.
Local validate-mode wall: 2.08s → 1.13s.
The full bench's measurement behavior in real mode is unchanged.
2221d95 to
324271e
Compare
BenchmarksBenchmark execution time: 2026-06-23 18:16:13 Comparing candidate commit bbf03fa in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 48 metrics, 1 unstable metrics.
|
The bg-thread block calls bg.join (line 219) and bg.join (line 221 after bg.kill), not bg.value. Two comments referenced bg.value: - line 188: "propagates out via bg.value below" - line 217: "10 arms × bg.value across the matrix" Thread#join re-raises an exception that terminated the thread in the calling thread, same as Thread#value. Behavior is correct; only the comments were stale.
There was a problem hiding this comment.
Pull request overview
Restores the Symbol Database (SymDB) microbenchmark scripts and their “bitrot” validation coverage that were previously removed, so the benchmarks exist on disk again and are runnable under RSpec (even though they are not yet re-enabled in the microbenchmarks CI job).
Changes:
- Re-adds three SymDB benchmark scripts under
benchmarks/. - Re-introduces an RSpec “validate benchmarks” spec for SymDB to ensure the scripts load and run.
- Updates
benchmarks/README.mdto document the SymDB benchmark prefix and its validation spec.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
spec/datadog/symbol_database/validate_benchmarks_spec.rb |
Adds coverage that loads/runs the SymDB benchmark scripts under VALIDATE_BENCHMARK=true. |
benchmarks/symbol_database_extraction.rb |
Restores the extraction benchmark script (wall/CPU/memory sampling). |
benchmarks/symbol_database_baseline_matrix.rb |
Restores the baseline matrix benchmark script (baseline CPU tiers vs ops/sec impact). |
benchmarks/symbol_database_background_impact.rb |
Restores the background extraction impact benchmark script (latency/throughput ratios). |
benchmarks/README.md |
Documents the symbol_database_ benchmark prefix and its validation spec. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| unless bg.join(VALIDATE_BENCHMARK_MODE ? 0.05 : 0.5) | ||
| bg.kill | ||
| bg.join | ||
| end |
| # extract_all until the main thread signals stop. Any exception | ||
| # propagates out via bg.join below — Thread#join re-raises in the | ||
| # caller when the thread terminated with an exception. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9634204a56
ℹ️ 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".
| loop do | ||
| iter_wall_start = Process.clock_gettime(Process::CLOCK_MONOTONIC) | ||
| iter_times_start = Process.times | ||
| scopes = extractor.extract_all |
There was a problem hiding this comment.
When this benchmark is used to validate the SymDB memory-overhead requirement, calling extract_all without a block measures the legacy path that materializes and retains the full Array<Scope>; during repeated iterations the previous array can remain live while the next one is built. The production upload path explicitly uses the block form (lib/datadog/symbol_database/component.rb:491-494) to avoid materializing the full array, so these RSS numbers can flag or hide regressions in a path customers do not exercise. Use the streaming form here and count yielded scopes instead of storing the whole result.
Useful? React with 👍 / 👎.
Adds STDERR.puts diagnostics around the TracePoint(:class) hook in install_hot_load_hook to investigate the Ruby 2.6-only flake of spec/datadog/symbol_database/component_spec.rb "does not propagate exceptions when logger.debug itself raises". Logs at: - hook install: pid, existing TracePoint count, thread - hook fired: mod name, thread, report_on_exception, TracePoint count - outer rescue entry: caught class + message + thread - pre-debug: thread + logger types - inner rescue entry: caught class + message + backtrace + TracePoint count - outer rescue done: thread (proves outer handler completed normally) No semantic change. To be reverted once we see CI output. Co-Authored-By: Claude <[email protected]>
dd-octo-sts
Bot
added
the
debugger
Typing analysisThis PR does not change typing compared to the base branch. |
The `install_hot_load_hook` method still carried the `DIAG (REVERT)` debugging instrumentation added in 1c514f5; the later "delete ai slop" commit cleaned the benchmark scripts but missed this file. The instrumentation ran `ObjectSpace.each_object(TracePoint).count` — a full-heap scan that holds the GVL — plus `STDERR.puts`, inside the `:class` TracePoint block, so it executed on every class definition in the process. This caused both failing checks: - Test (macos-15, 3.2): under the full suite, constant class loads from background threads each triggered the heap scan, starving the component's scheduler thread on the GVL. The hot-load extraction did not complete within the spec's wait window, so `extracted_modules` was empty and "extracts a class defined after the initial upload completes" failed with `expected [] to include "SymdbHotLoadSpecNewClass"`. - End-to-end uds-rails 2: the Rails weblog boot loads thousands of classes, each paying the per-load heap scan, stalling startup until the runner was canceled (~18 min with no progress after "Starting containers"). Removing the instrumentation restores `install_hot_load_hook` to its master state (verified byte-identical). No behavioral code changed. Verified locally: spec/datadog/symbol_database/component_spec.rb passes (52 examples, 0 failures); the previously-failing example passes 3/3 runs. The macos contention scenario requires the full suite on a macOS runner and was not reproduced locally; the fix is justified by removal of the per-class-load GVL-holding heap scan.
What does this PR do?
Restores the SymDB benchmark scripts (
symbol_database_background_impact.rb,symbol_database_baseline_matrix.rb,symbol_database_extraction.rb), theirbenchmarks/README.mdentries, and thevalidate_benchmarks_spec.rbcoverage that were removed by #5795.The
benchmarks/execution.ymlchange from the original revert is not included here — the scripts are restored on disk, but they are not yet wired into the microbenchmarks CI job.Motivation:
Revert requested.
Change log entry
None.
Follow-up required before these benchmarks can be enabled in CI
benchmark_ips.pyconverter expects[{item: "...", samples: [<numbers>]}]; the current scripts emit nested-dict structures (extraction stats, RPS-drop curves by load tier, baseline vs treatment ops/sec) and crash the converter withTypeError: string indices must be integers— this is the same failure that benchmarks: remove SymDB benchmarks #5795 was created to work around. Reworkemitin each script so every tracked measurement becomes its ownitemwith per-repsamples, and updatevalidate_benchmarks_spec.rbto assert the new shape.&othergroup inbenchmarks/execution.yml. This second step must land after (1) is on master, because the microbenchmarks job uses the candidate branch'sexecution.ymlto drive both the candidate and baseline runs — listing the scripts before they exist on master causes the baseline-sidebundle exec ruby benchmarks/symbol_database_*.rbinvocations to fail withLoadError.Additional Notes:
None.
How to test the change?
bundle exec rspec spec/datadog/symbol_database/validate_benchmarks_spec.rb