Skip to content

fix(prof) FrankenPHP idle phase#3169

Merged
realFlowControl merged 6 commits intomasterfrom
florian/fix-frankenphp-workermode-idle
Apr 1, 2025
Merged

fix(prof) FrankenPHP idle phase#3169
realFlowControl merged 6 commits intomasterfrom
florian/fix-frankenphp-workermode-idle

Conversation

@realFlowControl
Copy link
Member

@realFlowControl realFlowControl commented Apr 1, 2025

Description

This PR fixes FrankenPHP idle phase detection in worker mode. Turns out, that frankenphp_handle_request() does also execute the closure given to it, so the current way of measuring the idle phase in worker mode is incorrect. So what I am doing now instead of hooking frankenphp_handle_request() is hooking into sapi_module.activate/ sapi_module.deactivate in case the detected SAPI is FrankenPHP. If one of those functions is being called while frankenphp_handle_request() is the current execute frame, we are indeed in the idle phase.

Why does this work?

FrankenPHP in frankenphp_worker_request_shutdown() calls into sapi_deactivate() and in frankenphp_worker_request_startup() it calls into sapi_activate().

There is also a small cleanup: I have extracted the idle phase start and stop code into their own functions.

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@github-actions github-actions bot added profiling Relates to the Continuous Profiler tracing labels Apr 1, 2025
@codecov-commenter
Copy link

codecov-commenter commented Apr 1, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.07%. Comparing base (f115837) to head (f23afb5).
Report is 7 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3169      +/-   ##
============================================
- Coverage     79.09%   79.07%   -0.02%     
  Complexity     2890     2890              
============================================
  Files           114      114              
  Lines         11443    11443              
============================================
- Hits           9051     9049       -2     
- Misses         2392     2394       +2     
Flag Coverage Δ
tracer-php 79.07% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file 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 f115837...f23afb5. 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

pr-commenter bot commented Apr 1, 2025

Benchmarks [ profiler ]

Benchmark execution time: 2025-04-01 11:49:31

Comparing candidate commit f23afb5 in PR branch florian/fix-frankenphp-workermode-idle with baseline commit f115837 in branch master.

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

@realFlowControl realFlowControl marked this pull request as ready for review April 1, 2025 11:43
@realFlowControl realFlowControl requested a review from a team as a code owner April 1, 2025 11:43
@realFlowControl realFlowControl added this to the 1.8.0 milestone Apr 1, 2025
Copy link
Collaborator

@bwoebi bwoebi left a comment

Choose a reason for hiding this comment

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

Looks great to me :-)

@realFlowControl realFlowControl merged commit 7e44ce2 into master Apr 1, 2025
732 of 747 checks passed
@realFlowControl realFlowControl deleted the florian/fix-frankenphp-workermode-idle branch April 1, 2025 12:41
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