Last Comment Bug 709256 - Make no-op flushes faster
: Make no-op flushes faster
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: mozilla11
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
Depends on: 735805
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-09 12:54 PST by Boris Zbarsky [:bz] (Out June 25-July 6)
Modified: 2012-03-16 21:37 PDT (History)
7 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (261 bytes, text/html)
2011-12-09 13:07 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details
part 1. Fast-path RestyleTracker::ProcessRestyles when there aren't any restyles to process. (2.92 KB, patch)
2011-12-09 13:09 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
roc: review+
Details | Diff | Review
part 2. Short-circuit PresShell::ProcessReflowCommands when there aren't any. (6.76 KB, patch)
2011-12-09 13:10 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details | Diff | Review
Part 2 as diff -w, for ease of review (3.75 KB, patch)
2011-12-09 13:13 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
roc: review+
Details | Diff | Review
part 3. Skip calling PresShell::FlushPendingNotifications altogether if there might not be anything to flush. (12.82 KB, patch)
2011-12-09 18:59 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
roc: review+
Details | Diff | Review
part 4. Don't flush the sink for HTML if we've already started layout. (1.49 KB, patch)
2011-12-09 19:01 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
bugs: review+
Details | Diff | Review
part 5. Fast-path nsAnimationManager::DispatchEvents when there are no events. (2.75 KB, patch)
2011-12-09 19:17 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
dbaron: review+
Details | Diff | Review
Part 2 with less change to code (3.87 KB, patch)
2011-12-09 20:38 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
roc: review+
Details | Diff | Review
Part 3 actually passing tests (21.21 KB, patch)
2011-12-14 08:41 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
roc: review+
Details | Diff | Review

Description Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-09 12:54:50 PST
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.
Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-09 13:07:35 PST
Created attachment 580520 [details]
Testcase
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-09 13:09:35 PST
Created attachment 580521 [details] [diff] [review]
part 1.  Fast-path RestyleTracker::ProcessRestyles when there aren't any restyles to process.
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-09 13:10:59 PST
Created attachment 580522 [details] [diff] [review]
part 2.  Short-circuit PresShell::ProcessReflowCommands when there aren't any.
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-09 13:13:38 PST
Created attachment 580523 [details] [diff] [review]
Part 2 as diff -w, for ease of review
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-09 13:23:40 PST
How much perf improvement is there on your testcase?
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-09 13:26:34 PST
With the two patches so far, about 40% faster.  But there are more patches to come.
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-09 18:40:22 PST
> 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.
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-09 18:41:44 PST
Oh, and with these patches, on the testcase in bug 709217 we spend about 16 seconds, not 25 seconds.
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-09 18:58:13 PST
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.
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-09 18:59:39 PST
Created attachment 580603 [details] [diff] [review]
part 3.  Skip calling PresShell::FlushPendingNotifications altogether if there might not be anything to flush.
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-09 19:01:14 PST
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...
Comment 12 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-09 19:01:35 PST
Created attachment 580604 [details] [diff] [review]
part 4.  Don't flush the sink for HTML if we've already started layout.
Comment 13 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-12-09 19:03:22 PST
(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;
  }
Comment 14 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-09 19:05:35 PST
Yeah, I considered that.  It'd just help by a few %, but we can do it.

We really have way too much code.  :(
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-09 19:10:51 PST
(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?
Comment 16 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-09 19:14:26 PST
> 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.
Comment 17 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-09 19:17:01 PST
Created attachment 580606 [details] [diff] [review]
part 5.  Fast-path nsAnimationManager::DispatchEvents when there are no events.
Comment 18 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-12-09 19:27:49 PST
Comment on attachment 580606 [details] [diff] [review]
part 5.  Fast-path nsAnimationManager::DispatchEvents when there are no events.

r=dbaron
Comment 19 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-09 20:28:25 PST
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.
Comment 20 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-09 20:38:31 PST
Created attachment 580612 [details] [diff] [review]
Part 2 with less change to code
Comment 21 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-10 01:17:05 PST
Hmm.  These patches fail try.  I'll do some digging on Monday.
Comment 22 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-10 03:17:26 PST
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.
Comment 23 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-12 08:38:36 PST
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...
Comment 24 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-12 14:05:14 PST
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.
Comment 25 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-13 11:49:59 PST
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.
Comment 26 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-13 11:57:36 PST
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.
Comment 27 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-13 20:32:27 PST
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!
Comment 28 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-12-14 08:41:00 PST
Created attachment 581652 [details] [diff] [review]
Part 3 actually passing tests

Note You need to log in before you can comment on or make changes to this bug.