Closed Bug 1350930 Opened 3 years ago Closed 2 years ago

Add tracing markers for style + reflow flushes outside the refresh tick

Categories

(Core :: Gecko Profiler, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files)

The markers we have at the moment are only for flush phases of the refresh tick. If some JavaScript is causing a sync flush we don't show tracing markers. We should.
Attachment #8920445 - Flags: feedback?(felash)
Attachment #8920445 - Flags: review?(bzbarsky)
What is the actual cost of profiler_get_backtrace() when the profiler is not running?  That is, how much does it slow down a "trigger-reflow-and-flush-in-a-loop" testcase?
Comment on attachment 8920445 [details]
Bug 1350930 - Move profiler markers for reflow and style flushes from the refresh driver into the PresShell.

https://reviewboard.mozilla.org/r/191422/#review196878
Attachment #8920445 - Flags: review?(bzbarsky) → review+
Attachment #8920445 - Flags: feedback?(felash)
When testing the performance impact of this patch, I noticed that I was capturing stacks in the wrong place. nsIPresShell::DoObserveStyleFlushes() and nsIPresShell::DoObserveLayoutFlushes() are also only called once per refresh tick.
I'm going to change the patch to capture stacks in SetNeedLayoutFlush() and SetNeedStyleFlush() instead. I'm going to get performance numbers for that change as soon as I have an --enable-release build (because in my current local build, times in the microbenchmark are at least 2x of that in a regular Nightly due to slow stylo rust code).
The patch doesn't seem to impact profiler-off times at all: I get ~32µs per flush both with and without the patch.
It does affect profiler-on times quite a bit: 36µs -> 62µs over here. This is both from adding the markers and from capturing the backtrace.

The implementation of profiler_get_backtrace() could be slightly optimized to have even less overhead when the profiler isn't running: it grabs the profile state lock. It could have a RacyFeatures::IsActive() check before that, which would just check an atomic. But since the lock is going to be uncontended when the profiler is off, and callers of profiler_get_backtrace() have call frequencies on the order of microseconds (as opposed to nanoseconds), adding that atomic check doesn't seem necessary at this point.
Boris, are you fine with the change to use SetNeedLayoutFlush() and SetNeedStyleFlush() to capture the backtrace, and does comment 7 sound satisfactory?
Flags: needinfo?(bzbarsky)
> are you fine with the change to use SetNeedLayoutFlush() and SetNeedStyleFlush() to capture the backtrace

If we only do that when the boolean is not already set, right?  Seems OK in that case...
Flags: needinfo?(bzbarsky)
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/a554f8204f3b
Move profiler markers for reflow and style flushes from the refresh driver into the PresShell. r=bz
https://hg.mozilla.org/mozilla-central/rev/a554f8204f3b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1418836
Assignee: nobody → mstange
You need to log in before you can comment on or make changes to this bug.