378480 - [FIX]Investigate Tp2 regression from bug 84582

Status: RESOLVED INVALID

(Core :: CSS Parsing and Computation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 84582 comment 49 and on.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So I'm a little at a loss.  I tried profiling the DOM2 page in question locally.  But I get the results I expected.  A current build is a little (but not twice!) slower than one without the patch for bug 84582, and not reflowing in InitialReflow makes things faster.

To be precise, over 10 pageloads a build without the patch for bug 84582 spends:

  57000 hits in frame construction (all ContentAppended)
  90000 hits in reflow (35k from a reflow event, 55k from a scrollport event)
   1000 hits in painting
  60000 hits in parsing and content model construction

A current build spends:

   51000 hits in frame construction (37k ContentAppended, 14k ContentInserted)
  102000 hits in reflow (29k reflow event, 27k scrollport event, 46k initial)
     700 hits in painting
   60000 hits in parsing and content model construction

A build with InitialReflow gutted spends:

   50000 hits in frame construction (36k ContentAppended, 14k ContentInserted)
   95000 hits in reflow (all off a reflow event)
   50000 hits in parsing and content model construction (?)

So at the very least it gets us back to where we used to be... over here.  But not on tinderbox.

Of course I'm also not seeing the time-doubling that tinderbox sees for this file, but that's a matter of timing -- we would have to do incremental reflows at _just_ the wrong places to get that, I think...

Anything obvious I'm missing?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Let's try that again

This is the same as the original patch to gut InitialReflow, but with a content sink tweak to make sure we don't end up double-constructing frames for some content.  Ideally, this would fix the reftest orange I got when I tried the InitialReflow thing too...

I've done more profiling, but it seems that the numbers I was seeing initially were mostly luck; the numbers seem to be all over the place now.  :(

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 262540 [details] [diff] [review]
Let's try that again

This actually hurts Tp2, and I get reftest orange again (mac-only for some reason)...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmmm...  So I found one interesting thing!

With the new setup, but the time the initial reflow happens on the root scrollframe there can be all sorts of stuff there.  In particular, the first reflow we're guessing we won't need a vertical scrollbar... and in this case guessing wrong.

And putting off that first reflow to even later (by gutting InitialReflow) made things worse, because we had _more_ content to reflow twice.

I'll try to double-check the numbers tomorrow to make sure, but I bet this is it.  Ideally, we should guess that we need a scrollbar for the cases when we deferred StartLayout() due to stylesheets, and don't need one for the cases when we didn't. Or something.

Comment 5

[Boris Zbarsky [:bzbarsky]]

roc, thoughts on comment 4?

Another option, by the way, would be to do the gutting of InitialReflow and just remove the special-casing of the scrollframe's first reflow altogether.  That might really be best, but gutting InitialReflow is just a tad risky...

[Note to self: If we _do_ gut it, we need to make sure to block onload on the firing of the reflow event it posts.]

In any case, the tag flush in StartLayout is absolutely needed for correctness, imo.  Filed bug 378559 on it.

Comment 6

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

I think you should just augment the check "if (GetStateBits() & NS_FRAME_FIRST_REFLOW)" to also check that StartLayout was not deferred. If it was deferred, it should be fine to carry on with the existing path: use the history hint if available and otherwise guess "scrollbar needed".

I'm surprised this is responsible for a significant performance delta; I never saw much impact playing with this code.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Well...  This is the DOM2 Core spec page, being loaded from local disk (Tp2).  On my machine (which is slower than the tinderbox), builds from before bug 84582 reflow it three times: once during StartLayout(), once in the middle, and once at the end.  On a slightly faster machine, I wouldn't be surprised if we only reflowed it once in StartLayout() (very fast, no content) and once when the whole thing was done loading.  With the change from bug 84582 we reflow it twice in the StartLayout() call, and a good chunk of the content would already be there (this is where local disc matters).  So I can see it significantly increasing the pageload time.

Comment 8

[Bill Gianopoulos [:WG9s]]

This might sound like a silly idea, but it might be better to do the first reflow guessing that a vertical scroll bar will be required.  That way the only time anything would need to be redone is in the case where a veritcal scroollbar is not required and either content is centered horizontally in the window or a horizontal scrollbar is reqired.

But pages with no vertical scrollbar should have less content to reflow. 

Maybe this is why IE always displays the scrollbar and just grays it out when it it not required.
 

Comment 9

[Boris Zbarsky [:bzbarsky]]

> This might sound like a silly idea

Up to now it has been, since the first reflow always happened when the only content in the document was the <head>.  ;)

Comment 10

[Boris Zbarsky [:bzbarsky]]

Some numbers, in case people care:

Build from before bug 84582: 219371 hits
Current build, 10 pageloads of the testcase: 232986 hits
Same, but with the initial reflow clause commented out: 195788 hits
Same, but with InitialReflow also gutted: 176450 hits.

Times spent in reflow are 93254, 114362, 85346, 59658 hits respectively.

So I'll probably go ahead and do just the scrollframe thing in this bug, and file a followup on InitialReflow (and probably backing out the scrollframe thing).

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to that effect

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Additional patch

This fixes tests... we were mis-guessing on one of the printing reftests, and incrementally reflowing a page sequence, which doesn't work so hot.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Well... that didn't seem to help on tinderbox, either with the Linux builds or the Windows ones.  It does help over here, which means I'm somewhat at a loss as to what's going on with Tinderbox.  Both Linux and Windows there definitely have the problem with this one page, however...

Unfortunately I don't really have any more time to spend on this, so unless someone can come up with a good reason not to I'll be backing out bug 84582.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Pulling a guess out of my ass without knowing the code in question:

Could we for some reason be doing the initial reflow early enough now that it doesn't need a scrollbar. And then once we have a whole pile of content on the next reflow we first guess that we don't need a scrollbar (since we didn't on last reflow) but then having to reflow again since we now realize that we do need a scrollbar.

In other worse, could we be inserting a pile more content now between the first and second reflow? Possibly because the pending stylesheet loads stall reflow longer?

Comment 15

[Boris Zbarsky [:bzbarsky]]

The code from before this patch landed pulled that "second reflow" number from session history.  We only started using the "last time" number from the _third_ reflow....  so if that were the sequence of events we shouldn't have gotten a Tp or Tp2 jump to start with.

Comment 16

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

How about adding some instrumentation to CanvasFrame to print each time it gets reflowed and print the height of the canvasframe to give us an idea of how much content was in there? Then check that in just long enough to retrieve the data from the tinderbox log.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch with printfs

The relevant output seems to be:

  Finished canvas reflow for http://localhost/pageload/base/www.w3.org_DOML2Core/index.html:
  Height: 873074, overflow height: 873074
  Finished canvas reflow for http://localhost/pageload/base/www.w3.org_DOML2Core/index.html:
  Height: 1840094, overflow height: 1840094
  Finished canvas reflow for http://localhost/pageload/base/www.w3.org_DOML2Core/index.html:
  Height: 2807366, overflow height: 2807366

Those are in twips, so that first number is 14000-some pixels.

For comparison, the relevant output for Tp is:

  Finished canvas reflow for ....www.w3.org_DOML2Core:
  Height: 480, overflow height: 480
  Finished canvas reflow for ....www.w3.org_DOML2Core:
  Height: 396420, overflow height: 396420
  Finished canvas reflow for ....www.w3.org_DOML2Core:
  Height: 799620, overflow height: 799620
  Finished canvas reflow for ....www.w3.org_DOML2Core:
  Height: 813720, overflow height: 813720

Now I've been doing my local test on the Tp version of this page, because what's what I have access to.  I have NO idea why the Tp2 version is ending up at a totally different size.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Zipped-up tinderbox log just in case

Comment 19

[Boris Zbarsky [:bzbarsky]]

ok, rhelmer showed me how to get the Tp2 content... and it's identical to the Tp content.  I have no idea why they're ending up at sizes that differ by a factor of 3.5 or so.  The tests _might_ run in different width, by about 20px.  But that shouldn't give a difference that big.

I also have no idea why the first reflow has such a small height for Tp.  It really shouldn't.

On the other hand, given the Tp2 numbers we should definitely be guessing we want a scrollbar on that first reflow there.

The only other thing I can think of is that we do that initial reflow, then post a reflow event (probably from a scrollbar reflow callback or something) which we didn't use to post, then load more data, then end up basically doing one more incremental reflow than before.  Or something.

I suppose I could try gutting InitialReflow and seeing whether that helps on tinderbox...  Filed bug 378975.

Any other ideas?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Given bug 379093 I'm not sure how much I trust the Tp and Tp2 numbers, somehow...

Comment 21

[Boris Zbarsky [:bzbarsky]]

OK.  So the checkin for bug 379093 made Tp _and_ Tp2 jump on the Windows perf box.  And it made Tp2 jump on the argo-vm perf box.

Since those were the metrics that showed this regression to start with, and since over here I'm definitely seeing improvements with the patches I landed (on a slower machine than the test box, I think), I strongly feel that the Tp tests were just on crack.  :(