feat(prof): detect parallel threads#3515
Conversation
Benchmarks [ profiler ]Benchmark execution time: 2025-12-11 09:56:15 Comparing candidate commit 78caed3 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 28 metrics, 8 unstable metrics. |
a2136ff to
1e8bfd8
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3515 +/- ##
==========================================
- Coverage 61.81% 61.69% -0.12%
==========================================
Files 142 142
Lines 12923 12923
Branches 1695 1695
==========================================
- Hits 7988 7973 -15
- Misses 4183 4196 +13
- Partials 752 754 +2 see 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Benchmarks [ tracer ]Benchmark execution time: 2025-12-11 10:47:37 Comparing candidate commit 78caed3 in PR branch Found 3 performance improvements and 0 performance regressions! Performance is the same for 190 metrics, 1 unstable metrics. scenario:EmptyFileBench/benchEmptyFileBaseline
scenario:MessagePackSerializationBench/benchMessagePackSerialization
scenario:TraceSerializationBench/benchSerializeTrace
|
a8db45e to
1efdc54
Compare
1efdc54 to
c61f1e9
Compare
morrisonlevi
left a comment
There was a problem hiding this comment.
Before I dive into the code, which I've already seen a bit because of R&D week: since you are a maintainer of ext/parallel, should we instead release a new version of it which does something to make this simpler and easier to know? This PR is evidence there is value here, so rather than committing to platform specific code, should we "just" release a new version of ext/parallel?
I thought of this as well but: this won't allow us to detect "old" versions of parallel that would not export that new symbol (so we'd be left with either this code or being blind for those old versions).
The code is fairly simple IMHO, it boils down to a I get your point though and it would be "nice" to have something explicit, so I've added 78caed3 and opened krakjoe/parallel#352 |
morrisonlevi
left a comment
There was a problem hiding this comment.
Approved. Thanks for documenting things so I could read up about the platform differences.
Description
This PR adds code to detect and label threads that have been created by
ext-parallel. It is actually 80% comments and just a few lines of code 😉So in the end it turns out we can access the thread locals variables from other extensions. We do not need anything from that, just check if it is
NULL. In case it is, it is not aext-parallelthread, otherwise it is one 🎉Technically I am checking for the
php_parallel_scheduler_contextTLS being non-NULL, it is set in thephp_parallel_scheduler_setup()function right from the start of the thread main loop and before callingts_resource(0);which triggersGINIT.Reviewer checklist