Skip to content

fix(profiling): correct mixed-size allocation upscaling - #4210

Draft
realFlowControl wants to merge 3 commits into
masterfrom
florian/allocation-upscaling-regression
Draft

realFlowControl wants to merge 3 commits into
masterfrom
florian/allocation-upscaling-regression

Conversation

@realFlowControl

@realFlowControl realFlowControl commented Sep 17, 2026

Copy link
Copy Markdown
Member

Description

Warning

Do not merge until DataDog/libdatadog#2543 is merged and libdatadog has tagged a new release containing the fix. Before merging, update the submodule pin to that release and restore the tracking branch to main.

Add a mixed-size allocation regression to the existing profiler correctness CI. One call site makes 523264 allocations of 64 KiB and 1024 allocations of 32 MiB, with expected totals of 524288 allocations and 68652367872 payload bytes. The JSON documents the weighting math and uses 6% tolerances.

Temporarily pin libdatadog to GitHub branch florian/fix-poisson-upscaling, commit 0b40b2f46e779bdf5284ba52b2957533308b478d, from DataDog/libdatadog#2543. The lockfile remains compatible with the updated pin. There are no local-path overrides or PHP profiler implementation changes.

The upstream fix now accumulates weighted f64 totals directly in the existing eight-byte slots for global and matching ordinary-label Poisson rules, rather than retaining separate correction maps. Other values remain integers; stack and label aggregation is preserved. Endpoint-dependent and overlapping rules retain a deferred path. PHP already registers its rules before samples, as required by the updated API.

Before the fix, this workload reports about 97.1% too few allocations and 15% too few bytes. The rebuilt PHP 8.5 NTS profiler passes the unchanged mixed-size regression 5/5 times at the 4 MiB interval: count errors are 0.1–2.3% and byte errors are 0.0–1.2%, using rounded analyzer percentages. The existing allocation correctness test also passes, including the one-byte interval with and without ZendMM.

Memory validation upstream: 32 ordinary-label rules and 3,300 aggregate keys previously retained 105,600 separate correction entries; the compact implementation retains zero. This is a storage regression check, not an RSS or CPU benchmark. The earlier performance numbers measured the superseded implementation; CPU/RSS measurements for the compact implementation are still pending.

Local validation on macOS arm64 with PHP 8.5 NTS: libdatadog tests, doctests, profiling FFI examples, Clippy, and formatting passed. The final pin was checked with locked Cargo dependency resolution and the profiler was rebuilt: PHP Rust tests passed 35/35; PHPT had 36 passes, 11 platform/version/environment skips, and no failures; all 13 correctness cases and all 11 profiler-disabled scenarios passed.

An initial timeline failure also reproduced on the unchanged branch point 05e721689: the recorded [eval] event was below the test's 1% minimum share. Both versions then passed four consecutive reruns. No test tolerances were changed. Linux, ZTS, and the full PHP-version matrix were not run locally; full GitHub CI is pending.

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@datadog-prod-us1-6

datadog-prod-us1-6 Bot commented Sep 17, 2026

Copy link
Copy Markdown

Pipelines  Tests

Unblock PR with BitsAI

❌ Errors

Your PR has failed checks. Please review the issues below and take necessary action before merging.

🚦 249 Pipeline jobs failed

DataDog/apm-reliability/dd-trace-php | Extension Tea Tests: [7.0, debug-zts] — 🔧 Needs a code fix, caused by this PR

View more details · View in GitLab

DataDog/apm-reliability/dd-trace-php | Extension Tea Tests: [7.0, debug] — 🔧 Needs a code fix, caused by this PR

View more details · View in GitLab

DataDog/apm-reliability/dd-trace-php | Extension Tea Tests: [7.0, nts] — 🔧 Needs a code fix, caused by this PR

View more details · View in GitLab

View all 249 failed jobs.

ℹ️ Info

No other issues found (see more)

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 54.72% (-13.56%)

Useful? React with 👍 / 👎

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: d0350a1 | Docs | View more details | Give us feedback!

@pr-commenter

pr-commenter Bot commented Sep 17, 2026

Copy link
Copy Markdown

Benchmarks [ profiler ]

Benchmark execution time: 2026-09-21 15:55:34

Comparing candidate commit d0350a1 in PR branch florian/allocation-upscaling-regression with baseline commit 05e7216 in branch master.

Found 0 performance improvements and 3 performance regressions! Performance is the same for 26 metrics, 7 unstable metrics.

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

  • 🟩 = significantly better candidate vs. baseline
  • 🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

scenario:php-profiler-timeline-memory-control

  • 🟥 cpu_user_time [+26.112ms; +41.818ms] or [+3.987%; +6.386%]
  • 🟥 execution_time [+32.218ms; +36.561ms] or [+4.477%; +5.081%]

scenario:walk_stack/1

  • 🟥 wall_time [+273.383ns; +277.031ns] or [+2.203%; +2.232%]

Add a correctness workload using the default 4 MiB sampling distance and run it in GitHub CI. It exposes merge-before-upscale errors in allocation count and bytes; the new check intentionally fails until upscaling is fixed.
Use the GitHub libdatadog branch and commit from DataDog/libdatadog#2543. Keep this pin temporary until the upstream fix is merged and released.
@realFlowControl
realFlowControl force-pushed the florian/allocation-upscaling-regression branch from e7f6c13 to dab33b4 Compare September 21, 2026 14:32
Use the updated libdatadog upscaling fix without changing sampling intervals or correctness tolerances.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

profiling Relates to the Continuous Profiler

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant