Closed Bug 1350930 Opened 3 years ago Closed 2 years ago
Add tracing markers for style + reflow flushes outside the refresh tick
Bug 1350930 - Move profiler markers for reflow and style flushes from the refresh driver into the PresShell.
59 bytes, text/x-review-board-request
871 bytes, text/html
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+
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?
> 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...
Pushed by firstname.lastname@example.org: 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
You need to log in before you can comment on or make changes to this bug.