perf(prof): speed up interrupt_count checks - #4074
Conversation
On PHP 8.3 and older, the execute_internal hook called the full profiler interrupt handler after every internal function. Check EG(vm_interrupt) first so the TLS, request-state, and atomic work only runs when PHP has a pending VM interrupt. Across six balanced 60-second runs per binary on PHP 8.3 ZTS, mean allocation-loop throughput increased from 34,160,664/s to 36,061,991/s (+5.57%) and median throughput increased by 5.92%. Native samples reduced the manual interrupt path from 14.04% to 8.62% of main-thread samples. Wall-time correctness remained unchanged: a four-second CPU/sleep workload attributed 50.23% to sleep before and 50.31% after. Validation: PHP 7.3 ZTS cargo check; PHP 8.3 ZTS and PHP 8.5 NTS cargo test (22 passed each). https://datadoghq.atlassian.net/browse/PROF-15506
|
realFlowControl
added
Overhead
Benchmarks [ profiler ]Benchmark execution time: 2026-07-30 11:44:59 Comparing candidate commit ac2e342 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 27 metrics, 9 unstable metrics.
|
Benchmarks [ tracer ]Benchmark execution time: 2026-07-28 19:42:54 Comparing candidate commit be25644 in PR branch Found 2 performance improvements and 0 performance regressions! Performance is the same for 192 metrics, 0 unstable metrics.
|
morrisonlevi
left a comment
morrisonlevi
left a comment
There was a problem hiding this comment.
Early on in ddog_php_prof_interrupt_function we check the interrupt count being greater-than 0. I am surprised this helped so much. I'm going to play with it a bit locally.
This reverts commit be25644.
This reverts commit 02acd65.
…guard-internal-interrupt
This is very similar to the approach taken by Florian, which did a
check for EG(vm_interrupt). Although I was able to reproduce a small
speedup there, I realized that it's more nuanced than that:
1. This doesn't apply to actual VM interrupts, only to things which
need to check for a pending interrupt.
2. EG(vm_interrupt) technically isn't related to the thing we care
about, which is the interrupt count.
So this adds ddog_php_prof_interrupt_function_unlikely which is the
same as ddog_php_prof_interrupt_function at a high level, but it is
optimized to assume that there isn't a pending interrupt (opposite of
ddog_php_prof_interrupt_function).
84d0bcd to
8cb3cde
Compare
|
I could not reproduce such a significant gain! Florian, will you pull my changes and see what those give on your benchmarks? My changes performed more consistently for me locally, and keep the same spirit (fast return if there isn't a pending interrupt). The reason I also move the interrupt_count into ProfilerGlobals and access SystemSettings through the global so we don't have to use REQUEST_LOCALS at all on the hot path here. Nine pinned 60-second runs each, PHP 8.3 ZTS on my benchmark:
|
And use system settings from the global, rather than through the REQUEST_LOCALS.
![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: 32abe928af
ℹ️ 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".
This reverts commit 32abe92.
This reverts commit 8cb3cde.
These changes specifically speed up the hot paths of the interrupt
function. Notably, this can be a bit hot on 8.3 and below because of
execute_internal, and it's used on frameless functions on 8.4+. The
biggest change comes from avoiding REQUEST_LOCALS. In this case, this
is totally safe because interrupt_count is atomic.
This also removes the profiling_enabled check of the interrupt
function. This is unnecessary because:
1. We do not set interrupts when disabled to begin with, generally.
trigger_time_sample can still trigger but this is test only.
2. For edge cases having a pending interrupt going into a fork, the
child will call `Profiler::kill()` which will cause
`Profiler::get()` to return None.
morrisonlevi
changed the title
morrisonlevi
changed the title
There was a problem hiding this comment.
I approve of this PR--I basically rewrote it! Florian, please review. I made sure to preserve your commit history. Without your work here this wouldn't haven't happened. Also, some of the TSRM stuff was pulled from another one of your PRs, so you do have code in here still despite the reverted commits!
There was a problem hiding this comment.
I can't formally approve this PR as the author 😉
But here we are: ✅
About the two nitpicks: I am pretty sure the compiler will just inline them, so feel free to ignore. One upside (that I am not sure about) could be that in a debug build we might actually not inline and have the symbol available.
2 participants
Description
Speed up
interrupt_countchecks by making itRelaxedand moving it to theProfilerGlobalsinstead of usingREQUEST_LOCALS. The biggest change comes from avoidingREQUEST_LOCALS, which in this case is totally safe becauseinterrupt_countis atomic.Notably, part of the interrupt handler checks can be a bit hot in certain cases where we don't expect there to be an interrupt very often:
execute_internalwrapperSo we want the "0 interrupt count" check to be as fast and as early as possible.
This also removes the
profiling_enabledcheck of the interrupt function. This is unnecessary because:trigger_time_samplecan still trigger but this is test only.Profiler::kill()which will causeProfiler::get()to return None.These changes were inspired by and directly derived from iterating on Florian's original proposal (see Original description below). My (Levi) own benchmarks are similar to his (I hope), here is throughput:
Original description
Profiler interrupt checks run after internal, traced, and frameless calls even though a profiler interrupt is usually not pending. Export an
unlikelyentry point that reads the profiler-owned per-threadinterrupt_countwithOrdering::Relaxedand returns immediately when it is zero; only a pending interrupt reaches the authoritativeSeqCstswap and stack collection. The regular Zend VM interrupt callback remains unchanged.Move
interrupt_countfromREQUEST_LOCALSto PHP module globals so the hot path avoids borrowing request locals. The PHP 8.3-and-older internal-function hook and tracer hook call sites use the new entry point; PHP 8.4+ frameless handling uses the same relaxed precheck. Tracer symbol lookup falls back to the old entry point for compatibility with older profiler versions.Benchmark
Fresh release builds from
master(24c420794) and this branch (32abe928a), using six balanced 60-second runs per binary on PHP 8.3 ZTS, macOS ARM64:Wall-time correctness on a four-second CPU/sleep workload remained stable:
sleep(): 2.01s (50.37%)sleep(): 2.012s (50.16%)Reviewer checklist
https://datadoghq.atlassian.net/browse/PROF-15506