Skip to content

feat(prof): detect parallel threads#3515

Merged
realFlowControl merged 2 commits intomasterfrom
florian/parallel
Dec 15, 2025
Merged

feat(prof): detect parallel threads#3515
realFlowControl merged 2 commits intomasterfrom
florian/parallel

Conversation

@realFlowControl
Copy link
Copy Markdown
Member

@realFlowControl realFlowControl commented Dec 3, 2025

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 a ext-parallel thread, otherwise it is one 🎉

Technically I am checking for the php_parallel_scheduler_context TLS being non-NULL, it is set in the php_parallel_scheduler_setup() function right from the start of the thread main loop and before calling ts_resource(0); which triggers GINIT.

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@github-actions github-actions Bot added profiling Relates to the Continuous Profiler tracing labels Dec 3, 2025
@pr-commenter
Copy link
Copy Markdown

pr-commenter Bot commented Dec 3, 2025

Benchmarks [ profiler ]

Benchmark execution time: 2025-12-11 09:56:15

Comparing candidate commit 78caed3 in PR branch florian/parallel with baseline commit 2790073 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 28 metrics, 8 unstable metrics.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Dec 3, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.69%. Comparing base (2790073) to head (78caed3).
⚠️ Report is 17 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2790073...78caed3. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pr-commenter
Copy link
Copy Markdown

pr-commenter Bot commented Dec 3, 2025

Benchmarks [ tracer ]

Benchmark execution time: 2025-12-11 10:47:37

Comparing candidate commit 78caed3 in PR branch florian/parallel with baseline commit 2790073 in branch master.

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

scenario:EmptyFileBench/benchEmptyFileBaseline

  • 🟩 execution_time [-355.637µs; -129.003µs] or [-10.601%; -3.845%]

scenario:MessagePackSerializationBench/benchMessagePackSerialization

  • 🟩 execution_time [-7.026µs; -4.774µs] or [-6.549%; -4.449%]

scenario:TraceSerializationBench/benchSerializeTrace

  • 🟩 execution_time [-35.506µs; -21.994µs] or [-7.868%; -4.873%]

@realFlowControl realFlowControl force-pushed the florian/parallel branch 4 times, most recently from a8db45e to 1efdc54 Compare December 3, 2025 12:57
@realFlowControl realFlowControl marked this pull request as ready for review December 3, 2025 13:07
@realFlowControl realFlowControl requested a review from a team as a code owner December 3, 2025 13:07
Copy link
Copy Markdown
Collaborator

@morrisonlevi morrisonlevi left a comment

Choose a reason for hiding this comment

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

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?

Comment thread profiling/src/php_ffi.c
@realFlowControl
Copy link
Copy Markdown
Member Author

[...] 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? [...]

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).

[...] so rather than committing to platform specific code [...]

The code is fairly simple IMHO, it boils down to a dlsym(), a cast, a pointer dereference and a NULL compare, it won't get much simpler and honestly the MacOS code path only exists because we are using MacOS for local development.

I get your point though and it would be "nice" to have something explicit, so I've added 78caed3 and opened krakjoe/parallel#352

@realFlowControl realFlowControl enabled auto-merge (squash) December 15, 2025 22:08
Copy link
Copy Markdown
Collaborator

@morrisonlevi morrisonlevi left a comment

Choose a reason for hiding this comment

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

Approved. Thanks for documenting things so I could read up about the platform differences.

@realFlowControl realFlowControl merged commit bfde5e5 into master Dec 15, 2025
2007 of 2008 checks passed
@realFlowControl realFlowControl deleted the florian/parallel branch December 15, 2025 23:23
@github-actions github-actions Bot added this to the 1.15.0 milestone Dec 15, 2025
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 tracing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants