Skip to content

feat(profiling): add idle phases to timeline#2343

Merged
realFlowControl merged 7 commits into
masterfrom
florian/idle-phases
Nov 18, 2023
Merged

feat(profiling): add idle phases to timeline#2343
realFlowControl merged 7 commits into
masterfrom
florian/idle-phases

Conversation

@realFlowControl

@realFlowControl realFlowControl commented Nov 1, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Description

This PR will add idle phases between requests and idle phases due to the sleep()-class functions to the timeline view.
In detail this covers:

  • idle phase between RSHUTDOWN and RINIT (PHP process not handling a request)
  • and PHP process sleeping due to a sleep-class function call, namely
    • sleep()
    • usleep()
    • time_nanosleep()
    • and time_sleep_until()

Also includes one unrelated change: Moves the call to exception::exception_profiling_mshutdown(); a few lines up, so that it happens before the config and the profiler is shutdown.

This PR superseeds #2315 and will only contain idle phases as in sleeping or waiting to handle a new request in synchronous PHP (as used in PHP-FPM or Apache mod_php). Everything else (like event loop idle phases) shall go to another PR.

image

PROF-8370

Readiness checklist

  • (only for Members) Changelog has been added to the release document.
  • Tests added for this feature/bug.

Reviewer checklist

  • Appropriate labels assigned.
  • Milestone is set.
  • Changelog has been added to the release document. For community contributors the reviewer is in charge of this task.

@realFlowControl realFlowControl added the profiling Relates to the Continuous Profiler label Nov 1, 2023
@realFlowControl realFlowControl added this to the 0.94.0 milestone Nov 1, 2023
@realFlowControl
realFlowControl marked this pull request as ready for review November 1, 2023 00:31
@realFlowControl
realFlowControl requested a review from a team as a code owner November 1, 2023 00:31
@pr-commenter

pr-commenter Bot commented Nov 1, 2023 •
edited
Loading

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2023-11-15 22:33:37

Comparing candidate commit 2f860cb in PR branch florian/idle-phases with baseline commit a1f35f6 in branch master.

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

@morrisonlevi

morrisonlevi commented Nov 1, 2023 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Also includes one unrelated change: Moves the call to exception::exception_profiling_mshutdown(); a few lines up, so that it happens before the config and the profiler is shutdown.

It seems sensible, but could you expand on that a little bit more? I wonder if there's some correct-by-construction pattern we should employ here.

@realFlowControl

realFlowControl commented Nov 9, 2023

Copy link
Copy Markdown
Member Author

Also includes one unrelated change: Moves the call to exception::exception_profiling_mshutdown(); a few lines up, so that it happens before the config and the profiler is shutdown.

It seems sensible, but could you expand on that a little bit more? I wonder if there's some correct-by-construction pattern we should employ here.

The profiler is not needed in the exception::exception_profiling_mshutdown(), but it is needed to be around and valid in the timeline::timeline_mshutdown() as in that function we close the idle time box that was started in the last timeline::timeline_prshutdown(). This is done in order to report the last idle time when shutting down the PHP process.

This involves also keeping the static TAGS around and not clean them in prshutdown(). This is something that might have side effects that I could not foresee, so please pay special attention to this change when doing the review.

We might not need this in case we opt-in to just ignore the "last" idle timebox, which might be a corner case anyway, as this is the special case where the process shuts down.

@morrisonlevi morrisonlevi changed the title feat(profiling) add idle phases to timeline feat(profiling): add idle phases to timeline Nov 15, 2023
morrisonlevi
morrisonlevi reviewed Nov 15, 2023
View reviewed changes
Comment thread profiling/src/lib.rs
@morrisonlevi
refactor: extract {try_,}sleeping_fn helpers
2f860cb 
Be slightly more cautious about errors as well.
Also use #[no_mangle] with ddog_php_prof_ prefix functions.
realFlowControl
realFlowControl commented Nov 16, 2023
View reviewed changes

@realFlowControl realFlowControl left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

LGTM 🚢

morrisonlevi
morrisonlevi approved these changes Nov 16, 2023
View reviewed changes

@morrisonlevi morrisonlevi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As long as you've checked that it actually still works after my commit (not just that the commits look okay) then I'm good with it! The prof-correctness tests pass which is a good sign, obviously, but I still like to check this kind of thing manually.

@realFlowControl
realFlowControl merged commit e5d7e68 into master Nov 18, 2023
@realFlowControl
realFlowControl deleted the florian/idle-phases branch November 18, 2023 10:22
@realFlowControl realFlowControl mentioned this pull request Nov 28, 2023
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@morrisonlevi morrisonlevi morrisonlevi approved these changes

Assignees

No one assigned

Labels

profiling Relates to the Continuous Profiler

Projects

None yet

Milestone

0.94.0

Development

Successfully merging this pull request may close these issues.

2 participants

@realFlowControl @morrisonlevi