-
Notifications
You must be signed in to change notification settings - Fork 167
fix(prof): follow PHP globals model in allocation profiler #3175
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3175 +/- ##
=========================================
Coverage 79.26% 79.26%
Complexity 2948 2948
=========================================
Files 118 118
Lines 11633 11633
=========================================
Hits 9221 9221
Misses 2412 2412
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Benchmarks [ profiler ]Benchmark execution time: 2025-05-08 18:18:13 Comparing candidate commit 9b58e58 in PR branch Found 4 performance improvements and 0 performance regressions! Performance is the same for 25 metrics, 7 unstable metrics. scenario:php-profiler-exceptions-with-profiler
scenario:php-profiler-exceptions-with-profiler-and-timeline
scenario:walk_stack/50
scenario:walk_stack/99
|
71e58f0
to
386c78f
Compare
386c78f
to
43c1d6f
Compare
4b03225
to
779fa59
Compare
I've committed your suggestion |
2bdc1c9
to
0aada77
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This version of the patch is much easier to review, thank you! I pushed up some cleanup which shouldn't be controversial.
I think I see a better/cleaner way to do this we should discuss over zoom (although you are welcome to try it while I am sleeping). Basically, I think we can move from UnsafeCell
to Cell
, mark ZendMMState
as Copy
, and use a Cell
for the pure global version too. Then, we drop the macros.
At a high level, this works because LocalKey
has convenience methods since Rust 1.73 (local_key_cell_methods
). This avoids needing to use the with
dance:
thread_local! {
static X: Cell<i32> = Cell::new(1);
}
// No `with` stuff needed!
assert_eq!(X.get(), 1);
So for us, this kind of thing:
#[cfg(php_zts)]
ZEND_MM_STATE.with(|cell| {
let zend_mm_state = cell.get();
zend_mm_state_shutdown(zend_mm_state);
});
#[cfg(not(php_zts))]
unsafe {
zend_mm_state_shutdown(ptr::addr_of_mut!(ZEND_MM_STATE));
}
Becomes this (zend_mm_state_shutdown
needs to change too):
ZEND_MM_STATE.set(zend_mm_state_shutdown());
Both ZTS and NTS are handled seamlessly. Or at least, that's the idea.
ad433ef
to
d32ae2e
Compare
Also tidy up consistency and deduplicate some functions.
@realFlowControl, please review my recent commit. It implements the idea to use It also deduplicates |
Honestly, this is mostly just to push a new commit up to the branch. The gitlab CI seemed to somehow pick up the wrong branch, and I want to see if this reproduces it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approve, but as I contributed to this, Florian should review my work. Also, probably do some manual testing of it too (just usual stuff, not asking for anything extreme).
Co-authored-by: Florian Engelhardt <[email protected]>
Description
Since supporting ZTS versions of PHP, we started storing our profiler state in TLS (thread local storage). The assumption was that in PHP ZTS this would store the state per thread and helped us support ZTS PHP, while in NTS it would still store state in a TLS variable, but as there is only one thread, what gives!
It turns out there is at least one extension out there which does the following:
\extension\namespace\do_something()
functionEven though what that extension is doing is a race condition and violates thread safety, we should still not crash. Basically what the allocation profiler did was to make a race condition that might crash super seldom into a 100% guaranteed crash 😜
PROF-11471
Reviewer checklist