Skip to content

fix(profiling) wall-time crash after PHP runs on a native thread in ext-grpc - #4198

Open
realFlowControl wants to merge 8 commits into
masterfrom
florian/grpc-profiler-native-thread-repro
Open

realFlowControl wants to merge 8 commits into
masterfrom
florian/grpc-profiler-native-thread-repro

Conversation

@realFlowControl

@realFlowControl realFlowControl commented Sep 15, 2026

Copy link
Copy Markdown
Member

Why

The ext-grpc extension may execute PHP code on native threads that are not initialized as PHP threads, meaning they never go through GINIT/GSHUTDOWN.

  • When we collect a stack trace on one of these threads, we store raw string pointers in runtime cache slots and keep the corresponding strings alive in the thread-local CACHED_STRINGS. At this point, everything is fine.
  • The problem starts when that native thread exits. Rust destroys the thread-local CACHED_STRINGS, which frees the strings, but the runtime cache slots still contain pointers to them.
  • Later, when we collect a sample on the main PHP thread for the same function, we see that the runtime cache slot is already initialized and reuse it. That slot now contains a dangling pointer.
  • We dereference it and 💥.

Why did our earlier tests not catch this?

We disable the runtime-cache optimization for CLI, where PHPTs are executed, so we could not see it.

Description

This PR stores the string cache in the profiler's existing PHP module globals. PHP makes it process-global on NTS and thread-local through TSRM on ZTS, matching the runtime cache lifecycle without custom Rust TLS or an unsafe Sync implementation.

The PHP global is resolved and borrowed once around each full stack walk, not once per frame. GINIT creates the cache and GSHUTDOWN drops it.

The native-thread regression now executes a PHP closure that allocates on a background thread. The helper joins that thread before returning, then the test calls the same closure on the main thread and checks both results and the call count.

I/O profiling is disabled when ext-grpc is loaded, even with all experimental features enabled. gRPC can execute PHP on a native thread while the main thread does I/O, so sampling that I/O can race with changes to the shared NTS PHP stack. The profiler logs the reason and shows it in phpinfo.

The gRPC PHPT sets ZEND_DONT_UNLOAD_MODULES=1 because gRPC itself leaks on unload, even without the profiler. Keeping modules loaded preserves its global references for LeakSanitizer; leak detection stays enabled.

Testing

  • Profiler Rust tests with default and full CI features
  • PHP 8.5 NTS build
  • PHP 7.1 NTS Cargo check and Clippy
  • Native-thread closure PHPT passes on Linux/PHP 8.3 and macOS/PHP 8.5; restoring the old TLS string cache makes it segfault after joining
  • gRPC I/O-disable PHPT passes with real grpc.so and fails against the earlier profiler binary
  • Checked that explicit I/O enablement is blocked with gRPC and still works without it
  • Focused allocation and phpinfo PHPTs pass on Linux/PHP 8.3 and macOS/PHP 8.5
  • C syntax checks pass with PHP 8.2–8.6 headers and PHP 8.3 ZTS headers
  • PHP 8.4 debug-ZTS-ASAN arm64, using the exact CI image: reproduced the gRPC PHPT BORK before the unload workaround; full PHPT suite passes afterward (33 passed, 15 skipped)

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@datadog-official

datadog-official Bot commented Sep 15, 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.

🚦 38 Pipeline jobs failed

DataDog/apm-reliability/dd-trace-php | profiling tests: [8.0, arm64, php-compile-extension-alpine-, ] — ❌ 1 test failed · 🔧 Needs a code fix, caused by this PR

View more details · View in GitLab

❌ profiling/tests/phpt/grpc_io_disabled.phpt ([profiling] ext-grpc disables I/O profiling even with all experimental features enabled) from PHP.profiling.tests.phpt
002+ I/O Profiling Enabled => true
001- %AI/O profiling is disabled because ext-grpc can execute PHP on native threads.
002- %AI/O Profiling Enabled => false (incompatible with ext-grpc)
DataDog/apm-reliability/dd-trace-php | profiling tests: [7.2, arm64, php-compile-extension-alpine-, ] — ❌ 1 test failed

View more details · View in GitLab

❌ grpc_io_disabled.phpt - [profiling] ext-grpc disables I/O profiling even with all experimental features enabled from php-src.php-src.profiling.tests.phpt
002+ 
001- %AI/O profiling is disabled because ext-grpc can execute PHP on native threads.
002- %AI/O Profiling Enabled => false (incompatible with ext-grpc)
004+ I/O Profiling Enabled => true
DataDog/apm-reliability/dd-trace-php | profiling tests: [7.3, arm64, php-compile-extension-alpine-, ] — ❌ 1 test failed

View more details · View in GitLab

❌ [profiling] ext-grpc disables I/O profiling even with all experimental features enabled from PHP.profiling.tests.phpt
002+ 
001- %AI/O profiling is disabled because ext-grpc can execute PHP on native threads.
002- %AI/O Profiling Enabled => false (incompatible with ext-grpc)
004+ I/O Profiling Enabled => true

View all 38 failed jobs.

⚠️ Warnings

2 Test performance regressions detected

Test New execution time Base Execution time Increase DataDog link
profiling/tests/phpt/allocation_generator_01.phpt ([profiling] profiling should not crash during a ZEND_GENERATOR_CREATE)
from PHP.profiling.tests.phpt
25.67s 616.266006ms +25.05s (+4065%) View in Datadog
profiling/tests/phpt/allocation_generator_01.phpt ([profiling] profiling should not crash during a ZEND_GENERATOR_CREATE)
from php.profiling.tests.phpt
25.05s 623.21545ms +24.43s (+3920%) View in Datadog

ℹ️ Info

No other issues found (see more)

❄️ No new flaky tests detected

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 67.44% (-0.02%)

Useful? React with 👍 / 👎

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

@realFlowControl realFlowControl changed the title Fix profiler wall-time crash after PHP runs on a native thread fix(profiling) wall-time crash after PHP runs on a native thread in ext-grpc Sep 15, 2026
@github-actions github-actions Bot added the profiling Relates to the Continuous Profiler label Sep 15, 2026
@pr-commenter

pr-commenter Bot commented Sep 15, 2026

Copy link
Copy Markdown

Benchmarks [ profiler ]

Benchmark execution time: 2026-09-21 15:52:35

Comparing candidate commit 9eb1463 in PR branch florian/grpc-profiler-native-thread-repro with baseline commit 60b2826 in branch master.

Found 0 performance improvements and 6 performance regressions! Performance is the same for 23 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 [+39.026ms; +45.133ms] or [+6.036%; +6.981%]
  • 🟥 execution_time [+36.699ms; +41.874ms] or [+5.265%; +6.008%]

scenario:php-profiler-timeline-memory-with-profiler

  • 🟥 cpu_user_time [+36.474ms; +81.409ms] or [+2.683%; +5.988%]
  • 🟥 execution_time [+26.293ms; +46.206ms] or [+2.281%; +4.008%]

scenario:walk_stack/50

  • 🟥 wall_time [+580.043ns; +592.676ns] or [+3.825%; +3.908%]

scenario:walk_stack/99

  • 🟥 wall_time [+607.287ns; +611.300ns] or [+4.009%; +4.035%]

@realFlowControl
realFlowControl force-pushed the florian/grpc-profiler-native-thread-repro branch 2 times, most recently from 0506219 to 8fb0387 Compare September 16, 2026 07:01
@realFlowControl
realFlowControl force-pushed the florian/grpc-profiler-native-thread-repro branch from 8fb0387 to 0dae555 Compare September 16, 2026 08:46
@realFlowControl
realFlowControl force-pushed the florian/grpc-profiler-native-thread-repro branch from efaf119 to 7b5940b Compare September 16, 2026 12:20
@realFlowControl
realFlowControl marked this pull request as ready for review September 18, 2026 16:49
@realFlowControl
realFlowControl requested review from a team as code owners September 18, 2026 16:49
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 18, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-18T16:54:50.900315Z e638a4a Draft marked ready
🔒 Security Review Completed 2026-09-18T16:54:01.483809Z e638a4a Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e638a4a8d9

ℹ️ 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".

pub allocation_profiling_stats: UnsafeCell<MaybeUninit<allocation::AllocationProfilingStats>>,
/// String cache backing pointers stored in PHP runtime cache slots.
#[cfg(php_run_time_cache)]
pub cached_strings: UnsafeCell<MaybeUninit<RefCell<StringSet>>>,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Synchronize the cache shared by NTS native threads

When an NTS process receives a sample from an ext-grpc native thread while its PHP thread is also collecting a sample, both threads now access this process-global RefCell<StringSet> through try_borrow_mut(). RefCell is not thread-safe, so its borrow state and the underlying StringSet can race, causing undefined behavior or another crash in the concurrent native-thread workload this change is intended to support. The shared cache needs synchronization, or stack collection must otherwise be serialized.

Useful? React with 👍 / 👎.

Run a PHP closure on a native thread, join it, then call it again on the main thread in the regression test.
@pr-commenter

pr-commenter Bot commented Sep 21, 2026

Copy link
Copy Markdown

Benchmarks [ tracer ]

Benchmark execution time: 2026-09-21 16:44:41

Comparing candidate commit 9eb1463 in PR branch florian/grpc-profiler-native-thread-repro with baseline commit 60b2826 in branch master.

Found 2 performance improvements and 1 performance regressions! Performance is the same for 191 metrics, 0 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:HookBench/benchWithoutHook

  • 🟥 execution_time [+3.577µs; +5.324µs] or [+4.329%; +6.444%]

scenario:MessagePackSerializationBench/benchMessagePackSerialization

  • 🟩 execution_time [-5.614µs; -3.506µs] or [-4.841%; -3.023%]

scenario:MessagePackSerializationBench/benchMessagePackSerialization-opcache

  • 🟩 execution_time [-7.476µs; -5.324µs] or [-6.278%; -4.471%]

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