Make no-op flushes faster

RESOLVED FIXED in mozilla11

Status

()

Core
Layout
P2
normal
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla11
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 3 obsolete attachments)

261 bytes, text/html
Details
2.92 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.49 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.75 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
3.87 KB, patch
roc
: review+
Details | Diff | Splinter Review
21.21 KB, patch
roc
: review+
Details | Diff | Splinter Review
Right now even a no-op flush is pretty heavyweight.  The attached testcase spends 80+% of its time in flushes.

Some patches coming up to deal with the most significant culprits.
Created attachment 580520 [details]
Testcase
Created attachment 580521 [details] [diff] [review]
part 1.  Fast-path RestyleTracker::ProcessRestyles when there aren't any restyles to process.
Attachment #580521 - Flags: review?(roc)
Created attachment 580522 [details] [diff] [review]
part 2.  Short-circuit PresShell::ProcessReflowCommands when there aren't any.
Created attachment 580523 [details] [diff] [review]
Part 2 as diff -w, for ease of review
Attachment #580523 - Flags: review?(roc)
Attachment #580521 - Flags: review?(roc) → review+
How much perf improvement is there on your testcase?
Attachment #580523 - Flags: review?(roc) → review+
With the two patches so far, about 40% faster.  But there are more patches to come.
> How much perf improvement is there on your testcase?

The endgame looks like it reduces the time for the testcase from about 1100ms to about 150ms.  There's still 16% under nsDocument::FlushPendingNotifications, and some DOM binding time (another 15% of total), but at least half the time is self time in GetOffsetRect and time in GetAllInFlowRectsUnion.

Cleaning up the patches a bit before posting.
Oh, and with these patches, on the testcase in bug 709217 we spend about 16 seconds, not 25 seconds.
OK.  So with the two patches above the main hotspots are self time in PresShell::FlushPendingNotifications, the GetRootPrescontext calls it makes and the FlushExternalResources calls on the document.

For this last, I could fast-path the "no external resources" case, but I'm not sure it's necessarily worth it.  The other two seem hard to deal with.  The next thing is nsAnimationManager::DispatchEvents (self time and array swaps) and script blocker removal from the presshell flush.

So what I'm going to do is to bypass flushing the presshell altogether if possible.
Created attachment 580603 [details] [diff] [review]
part 3.  Skip calling PresShell::FlushPendingNotifications altogether if there might not be anything to flush.
Attachment #580603 - Flags: review?(roc)
Comment on attachment 580603 [details] [diff] [review]
part 3.  Skip calling PresShell::FlushPendingNotifications altogether if there might not be anything to flush.

This doesn't make me all that terribly happy, but the other option is to keep track of all those "have work that needs flushing" booleans directly on the document or presshell instead of on all the individual things that need flushing...
Created attachment 580604 [details] [diff] [review]
part 4.  Don't flush the sink for HTML if we've already started layout.
Attachment #580604 - Flags: review?(bugs)
(In reply to Boris Zbarsky (:bz) from comment #9)
> The next thing is nsAnimationManager::DispatchEvents (self time and array
> swaps)

How about making nsAnimationManager::DispatchEvents start with:

  if (mPendingEvents.IsEmpty()) {
    return;
  }
Yeah, I considered that.  It'd just help by a few %, but we can do it.

We really have way too much code.  :(
(In reply to Boris Zbarsky (:bz) from comment #11)
> Comment on attachment 580603 [details] [diff] [review]
> part 3.  Skip calling PresShell::FlushPendingNotifications altogether if
> there might not be anything to flush.
> 
> This doesn't make me all that terribly happy, but the other option is to
> keep track of all those "have work that needs flushing" booleans directly on
> the document or presshell instead of on all the individual things that need
> flushing...

You mean the other way around? Keep track of booleans on all the individual things that need flushing?

I think this approach seems reasonable, but would it make more sense to put the flag on the presshell instead of the document?
> Keep track of booleans on all the individual things that need flushing?

Those things already keep track of it in various ways.  So those booleans are already present.  What I'm trying to avoid is having to poll them all on every flush.

As far as where the flag should go...  Having it on the presshell would work too, I guess.  It'd be a little more work in the document code to check it, but setting it might be simpler in some of the consumers.  On the other hand, the set would still have to go look at the document (for the display document thing).  So just putting it on the document seemed simpler.
Created attachment 580606 [details] [diff] [review]
part 5.  Fast-path nsAnimationManager::DispatchEvents when there are no events.
Attachment #580606 - Flags: review?(dbaron)
Comment on attachment 580606 [details] [diff] [review]
part 5.  Fast-path nsAnimationManager::DispatchEvents when there are no events.

r=dbaron
Attachment #580606 - Flags: review?(dbaron) → review+
Comment on attachment 580522 [details] [diff] [review]
part 2.  Short-circuit PresShell::ProcessReflowCommands when there aren't any.

> +    NS_ABORT_IF_FALSE(!mShouldUnsuppressPainting || mIsDestroying,

This actually fails.  In particular, we can enter ProcessReflowCommands from HandlePostedReflowCallbacks called from DidDoReflow from ResizeReflow.

And even in ProcessReflowCommands, DidDoReflow comes before we set mShouldUnsuppressPainting to false.

So I think I just need to change the logic here to be closer to what it used to be.
Created attachment 580612 [details] [diff] [review]
Part 2 with less change to code
Attachment #580612 - Flags: review?(roc)
Attachment #580522 - Attachment is obsolete: true
Attachment #580523 - Attachment is obsolete: true
Hmm.  These patches fail try.  I'll do some digging on Monday.
Attachment #580612 - Flags: review?(roc) → review+
Comment on attachment 580603 [details] [diff] [review]
part 3.  Skip calling PresShell::FlushPendingNotifications altogether if there might not be anything to flush.

Review of attachment 580603 [details] [diff] [review]:
-----------------------------------------------------------------

It's hard to be sure we're calling PresShellNeedsFlush everywhere we need to, but my reading of PresShell::FlushPendingNotifications suggests you are.

::: content/base/public/nsIDocument.h
@@ +1600,5 @@
>    void WarnOnceAbout(DeprecatedOperations aOperation);
>  
>    virtual void PostVisibilityUpdateEvent() = 0;
>  
> +  void PresShellNeedsFlush() {

I would probably call this SetPresShellNeedsFlush to make it clear it's not a getter.
Attachment #580603 - Flags: review?(roc) → review+
OK.  So one reason for the test fails is that part 3 is just wrong.  :(

The invariant it depends on is that any time a flush happens, the next time something is queued up after that a call to SetPresShellNeedsFlush will be made.  This invariant is not maintained by nsCSSFrameConstructor::PostRestyleEventInternal, at the very least.

I'll make sure things pass try before asking for the next review here...
OK, so there was a similar issue for pending reflows as pending restyles.  The other problem I found was that a presshell flush might or might not flush reflows (obviously).  So I switched to separate booleans for layout and style flushes, so that we can optimize away repeated style-only flushes even if there are dirty reflow roots about.  Going to see how try deals.
That fixed most of the problems.

There's a remaining failure from part 2 in layout/generic/test/test_bug633762.html

There's a failure in toolkit/content/tests/chrome/test_mousescroll.xul (chrome test) that I can't reproduce locally so far.

There's a failure in browser/devtools/highlighter/test/browser_inspector_highlighter.js (browser-chrome test) from my revised part 3.

Looking into those.
Ah, the part 2 fail was because the return value from ProcessReflowCommands is supposed to be whether we completed, not whether we interrupted.  So the right thing to return in the short-circuit case is "true", not false.  Making that change fixes the test.
browser_inspector_highlighter fails because it has a resize event handler that tries to get layout information.

We run the pending resize event quite early in FlushPendingNotifications.  This happens before the presshell has actually flushed layout and whatnot.  But the document code sets its "needs flush" booleans to "false" before the call into the presshell, because that way it can correctly handle them being reset to true again from inside the flush.  So the reentering call from inside the resize event ends up not actually flushing, and the caller in this case gets incorrect layout information.  Fun.  Good thing we have tests!

Updated

6 years ago
Attachment #580604 - Flags: review?(bugs) → review+
Created attachment 581652 [details] [diff] [review]
Part 3 actually passing tests
Attachment #581652 - Flags: review?(roc)
Attachment #580603 - Attachment is obsolete: true
Attachment #581652 - Flags: review?(roc) → review+
https://hg.mozilla.org/mozilla-central/rev/f586cb3fa70d
https://hg.mozilla.org/mozilla-central/rev/d79786b46951
https://hg.mozilla.org/mozilla-central/rev/cdc587a042ae
https://hg.mozilla.org/mozilla-central/rev/43c603bd0163
https://hg.mozilla.org/mozilla-central/rev/a41a3d881600
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla11
Version: 9 Branch → Trunk
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Blocks: 735805
No longer blocks: 735805
Depends on: 735805
Depends on: 1279202
You need to log in before you can comment on or make changes to this bug.