Closed Bug 230170 Opened 16 years ago Closed 16 years ago

[FIX]Look into batching style reresolutions

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.8alpha3

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 3 open bugs)

Details

(Keywords: perf)

Attachments

(1 file, 7 obsolete files)

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.
Depends on: 229391
Blocks: 203448
Blocks: 212831
Blocks: 229391
No longer depends on: 229391
Blocks: 186442
Depends on: 240874
Attached patch Quick hack (obsolete) — Splinter Review
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.
Oh, that pref is only read once, so if you change it you need to restart.
i don't have a tree but can test this if you e-mail me a build.
Attachment #147026 - Attachment is obsolete: true
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.
Note to self -- the restyle event should probably block onload; otherwise some
XUL dialogs get confused.  <sigh>.
Depends on: 144072
Attachment #147155 - Attachment is obsolete: true
Depends on: 244235
Blocks: 64516
Target Milestone: mozilla1.8alpha1 → mozilla1.9alpha
Blocks: 186465
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.
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).)
Blocks: 249546
This is basically ready to go.
Attachment #147695 - Attachment is obsolete: true
Attached patch Same thing, but diff -w (obsolete) — Splinter Review
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.
Attachment #153376 - Flags: review?(dbaron)
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 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.
Attachment #153376 - Flags: review?(dbaron)
Comment on attachment 153375 [details] [diff] [review]
Updated to tip again, a bit improved

Mental note: this diff is missing the nsPresShell.cpp changes.
Attachment #153375 - Attachment is obsolete: true
Attachment #153376 - Attachment is obsolete: true
Attached patch As diff -w (obsolete) — Splinter Review
Comment on attachment 153497 [details] [diff] [review]
As diff -w

OK, I think this is ready to go.
Attachment #153497 - Flags: superreview?(dbaron)
Attachment #153497 - Flags: review?(dbaron)
Blocks: 194952
Summary: Look into batching style reresolutions → [FIX]Look into batching style reresolutionsa
Target Milestone: mozilla1.9alpha → mozilla1.8alpha3
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)
}
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 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.
Summary: [FIX]Look into batching style reresolutionsa → [FIX]Look into batching style reresolutions
> 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.
> 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.
Attachment #153496 - Attachment is obsolete: true
Attachment #153497 - Attachment is obsolete: true
Attachment #153497 - Flags: superreview?(dbaron)
Attachment #153497 - Flags: superreview-
Attachment #153497 - Flags: review?(dbaron)
Attachment #153497 - Flags: review-
Attachment #155630 - Flags: superreview?(dbaron)
Attachment #155630 - Flags: review?(dbaron)
Attachment #155630 - Flags: superreview?(dbaron)
Attachment #155630 - Flags: superreview+
Attachment #155630 - Flags: review?(dbaron)
Attachment #155630 - Flags: review+
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...
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
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?
OH yeah. but if you have a testcase, I'm confident bz will be able to fix it
before 1.8 ships.
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.
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.
Any chance this could have caused bug 255899?
> 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).
Blocks: 255899
Bug 255899 comment 7 seems to verify that this is the cause of that bug...
No longer blocks: 255899
Depends on: 255899
Could this have caused bug 262031 and is the current behavior as described in
that bug the one that should be kept? Thanks
Depends on: 262031
Depends on: 295690
Depends on: 312695
Blocks: 289517
Any chance this could have caused bug 308030? The timing is about right, and I can't see anything obvious in Bonsai.
It's pretty much impossible to tell without a testcase.
Depends on: 331156
Depends on: 257237
Depends on: 255270
Depends on: 265416
Depends on: 279308
Depends on: 277653
Depends on: 282265
No longer blocks: 289517
Depends on: 289517
Depends on: 291183
Depends on: 311659
Depends on: 311661
You need to log in before you can comment on or make changes to this bug.