230170 - [FIX]Look into batching style reresolutions

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

See
http://groups.google.com/groups?dq=&hl=en&lr=&ie=UTF-8&oe=UTF-8&frame=right&th=3001bb6df618b720&seekm=bstfk7%241kr1%40ripley.netscape.com#link1

The basic idea is to not reresolve style immediately but to post a reresolve
command (akin to reflow commands) and perform some basic coalescing on the
commands.  Then off a timer process the reresolves.  This way setting multiple
properties on foo.style only reresolves once.

Comment 1

[Boris Zbarsky [:bzbarsky]]

actually,
http://groups.google.com/groups?dq=&hl=en&lr=&ie=UTF-8&oe=UTF-8&frame=right&th=63a773e19c80e547&seekm=mailman.1072824000.17326.mozilla-style%40mozilla.org#link1
has more info

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Quick hack

This is a quick hack for testing to see whether it does what I think it should.
 Under no cirumstances should this be checked in; it has so many issues I'm not
even sure where to start.

After applying this patch and rebuilding layout/, style reresolves on attr
changes will be batched (most importantly, they will be batched for inline
style changes).  To disable the batching, set the boolean pref
"nglayout.debug.disable_reresolve_batching" to true.  This should allow easy
testing of the effect of this patch in a build that is otherwise identical.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, that pref is only read once, so if you change it you need to restart.

Comment 4

[Hixie (not reading bugmail)]

i don't have a tree but can test this if you e-mail me a build.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Same hack, working around gcc on Windows

Comment 6

[Boris Zbarsky [:bzbarsky]]

Ian, see <http://web.mit.edu/bzbarsky/www/testBuilds/restyle-test.tar.gz>.  Just
as a warning, this build came from the tree this hack lives in, and so has a
whole slew of other changes in it in addition to the hack in question; mostly
changes that are half-baked enough that I haven't bothered attaching patches to
bugs (or in some cases filing bugs).  So expect all the usual "it will eat your
files" warnings to apply.  I don't think it kills babies, but I haven't really
tested that, so don't bet on it.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Note to self -- the restyle event should probably block onload; otherwise some
XUL dialogs get confused.  <sigh>.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to tip, for ease of testing

Comment 9

[kmike]

I applied this patch to 20040530 linux trunk tree. Here're some perfomance
numbers for your consideration:

http://www.hixie.ch/tests/adhoc/perf/dom/real-world/006.html
Before:
Elapsed time: 23 422 ms
Average per loop: 468.44 ms
Fastest loop: 389 ms
Slowest loop: 566 ms

After:
Elapsed time: 14 458 ms
Average per loop: 289.16 ms
Fastest loop: 237 ms
Slowest loop: 374 ms

Next 2 tests report much lower execution time than the actual running time.

http://www.hixie.ch/tests/adhoc/perf/dom/artificial/007.html
Before:
Test complete. Elapsed time: 13.625s

After:
Test complete. Elapsed time: 3.509s
(very visible improvement, though there're some noticeable jerks at the
beginning and at the end of the test)

http://www.hixie.ch/tests/adhoc/perf/dom/artificial/009.html
Before:
Elapsed time: 2355ms. Average time per iteration: 1.57ms.

After:
Elapsed time: 856ms. Average time per iteration: 0.5706666666666667ms.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Actually, testing performance with that patch is pointless (the numbers you will
get are bogus).  I'm interested in actual appearance (like whether you get frame
dropping), not in performance, thus far.  That is, I know this patch makes
things faster; I'm interested in knowing what it breaks.

(In addition, all the tests you ran have code that's designed to make sure the
browser flushes out reflows all the time (so that the numbers will mean
something, since the tests are producing numbers, not results).  I'm not sure
that makes the "real-world" test particularly real-world, but... In any case,
the current patch doesn't do any style reresolve flushing, so you'll get bogus
results.  I fully expect the two "artificial" tests to slow down a bit with
final versions patch (though the "real-world" test will indeed get sped up).)

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to tip again, a bit improved

This is basically ready to go.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Same thing, but diff -w

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 153376 [details] [diff] [review]
Same thing, but diff -w

David, could you give this a skim?  Not a full review, just a review of the
general approach?

Basically, we stash away the content node and the restyle hint and changehint
we got from the content node for that attr.  We put off the reresolve and the
application of the changes to the tree on an event.  We do NOT save the primary
frame with the other data; this protects us against nodes being removed from
the tree or some such silliness that destroys the frame while we're waiting for
our event.  Actually catching all cases of frame destruction and cleaning this
hashtable didn't seem worthwhile.

Come to think of it, perhaps ProcessRestyle should just bail if the content
node is no longer in a document... I'll make that change locally.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Hmm... actually, if something has a ReStyle_LaterSiblings hint and gets removed
from the document before we get to restyle them, we'll never get to it....

Then again, if something with that hint gets removed from the document _period_
we fail to restyle things properly at the moment, and when we fix that we'll
automatically fix this issue.  So I think it's fine.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 153376 [details] [diff] [review]
Same thing, but diff -w

This seems to cause some problems with XUL submenus (they only come up the
second time and subsequent times you hover on the menuitem, not the first
time)...  Looking into why.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 153375 [details] [diff] [review]
Updated to tip again, a bit improved

Mental note: this diff is missing the nsPresShell.cpp changes.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to fix the menu issue, including all parts now

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: As diff -w

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 153497 [details] [diff] [review]
As diff -w

OK, I think this is ready to go.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 153497 [details] [diff] [review]
As diff -w

Why a pref?  Why not just remove the old code?	Is it temporary?

>+ProcessRestyle(nsISupports* aContent,

>+  if (aData.mChangeHint != NS_STYLE_HINT_NONE ||
>+      (aData.mRestyleHint & eReStyle_Self)) {
>+    nsIFrame* primaryFrame = nsnull;
>+    shell->GetPrimaryFrameFor(content, &primaryFrame);
>+    shell->FrameConstructor()->RestyleElement(context, content, primaryFrame,
>+                                              aData.mChangeHint);
>+  }

This removes the HasAttributeDependentStyle optimization for AttributeChanged
cases where we get an attribute change hint from the content.  I'd prefer not
doing that, i.e.:

if (aData.mRestyleHint & eReStyle_Self) {
  // existing code above
} else if (aData.mChangeHint != NS_STYLE_HINT_NONE) {
  nsStyleChangeList changeList;
  changeList.AppendChange(primaryFrame, content, hint);
  ProcessRestyledFrames(changeList, context)
}

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm wondering whether you should remove this bit of nsFrameManager.cpp:

        if (!(aMinChange & nsChangeHint_ReconstructFrame)) {
          // if frame gets regenerated, let it keep old context

and unconditionally give the frame its new style context.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 153497 [details] [diff] [review]
As diff -w

>+ProcessRestyle(nsISupports* aContent,

>+  if (!content->GetDocument() ||
>+      content->GetDocument() != context->GetDocument()) {
>+    // Content node has been removed from our document; nothing else to do here
>+  }

I assume you intended to have a return statement here.

I think you need to add FlushPendingNotifications(Flush_StyleReresolves) calls
in nsComputedDOMStyle.cpp, no?

Other than the issues in this comment and the 2 before it, this looks fine.

Comment 23

[Boris Zbarsky [:bzbarsky]]

> Why a pref?  Why not just remove the old code?	Is it temporary?

The idea was to give people a simple way to switch back to the old code to test
regressions.... At this point, I've been running with this for long enough that
I think it's ok with the old code just removed, so I'll do that.

> This removes the HasAttributeDependentStyle optimization for AttributeChanged

Good catch.  Will fix.

> I'm wondering whether you should remove this bit of nsFrameManager.cpp:

I don't think I can.  The code in nsCSSFrameConstructor::ContentRemoved (which
is what handles frame reconstructs) looks at the display value of the frame, for
instance, to do proper cleanup.  So the frame needs to keep its old display
value till that code is hit.

> I assume you intended to have a return statement here.

Yeah.

> I think you need to add FlushPendingNotifications(Flush_StyleReresolves) calls

Yes.  I thought I'd included those changes in the diff, but I see I did not... :(

Updated patch coming up in a few hours.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> I think you need to add FlushPendingNotifications(Flush_StyleReresolves) calls

Actually, that call is already there; I added it back when I changed
FlushPendingNotifications... See nsComputedDOMStyle::GetPropertyCSSValue.

I do have to add this to the InspectorCSSUtils code, though.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments (and merged to tip)

Comment 26

[Boris Zbarsky [:bzbarsky]]

Fix checked in (with a followup fix to nsCSSFrameConstructor to deal with a
crash  this introduced).

The Tdhtml test times dropped about 20% with this patch.  One of the three mac
tinderboxen showed a rise in Txul, which I'm going to look into, but the other
two were fine, as were the non-Mac machines...

Comment 27

[Mike Kaply [:mkaply]]

I have verified that this change caused a major IBM application to break.

Is there anything in this code that could have broken web applications?

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OH yeah. but if you have a testcase, I'm confident bz will be able to fix it
before 1.8 ships.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Michael, there are certainly things in this code that could lead to breakage
(and I have several regressions to deal with when I get back already).  Please
file bugs on issues you have run into, attach testcases, and I'll look into them.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Note: this caused regression bug 255153 (and fixing it caused a bunch of other
regressions that are marked dependent on bug 256242).  There was also a
fundamental problem in the patch (see bug 257694).  These are mostly fixed, now.

Comment 31

[Aaron Kaluszka]

Any chance this could have caused bug 255899?

Comment 32

[Boris Zbarsky [:bzbarsky]]

> Any chance this could have caused bug 255899?

Pretty minor (given that this patch went in after the build reported as broken
in that bug was put up on FTP, assuming the firefox builds are typically morning
builds... of course it would be nice if people would post actual build IDs
instead of the Gecko ID from the UA string).

Comment 33

[Aaron Kaluszka]

Bug 255899 comment 7 seems to verify that this is the cause of that bug...

Comment 34

[Robert Strong (they/them - no direct email)]

Could this have caused bug 262031 and is the current behavior as described in
that bug the one that should be kept? Thanks

Comment 35

[Wayne Woods]

Any chance this could have caused bug 308030? The timing is about right, and I can't see anything obvious in Bonsai.

Comment 36

[Boris Zbarsky [:bzbarsky]]

It's pretty much impossible to tell without a testcase.