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

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement)

Description

[Markus Stange [:mstange]]

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.

Comment 1

[Markus Stange [:mstange]]

Attachment: Bug 1350930 - Move profiler markers for reflow and style flushes from the refresh driver into the PresShell.

Review commit: https://reviewboard.mozilla.org/r/191422/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/191422/

Comment 2

[Boris Zbarsky [: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 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

[Markus Stange [:mstange]]

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

Comment 5

[Markus Stange [:mstange]]

Attachment: reflowflushoverhead.html (microbenchmark for cheap-to-reflow reflow flushes)

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8920445 [details]
Bug 1350930 - Move profiler markers for reflow and style flushes from the refresh driver into the PresShell.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191422/diff/1-2/

Comment 7

[Markus Stange [:mstange]]

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.

Comment 8

[Markus Stange [:mstange]]

Boris, are you fine with the change to use SetNeedLayoutFlush() and SetNeedStyleFlush() to capture the backtrace, and does comment 7 sound satisfactory?

Comment 9

[Boris Zbarsky [: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...

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8920445 [details]
Bug 1350930 - Move profiler markers for reflow and style flushes from the refresh driver into the PresShell.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191422/diff/2-3/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

Comment 12

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/a554f8204f3b