1172239 - High CPU usage when open amazon.co.jp in several tabs

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Alice0775 White]

High CPU usage (almost 100%/core).
UI stops response.

Steps To Reproduce:
1. Open URL in several tabs (6-8 tabs)
2. Wait until complete page loading

Actual Results:
High CPU usage (almost 100%/core)
UI stops response.

Expected Results:
Should not consume CPU



Profiler:
http://people.mozilla.org/~bgirard/cleopatra/#report=83831faa8176ca9da8044326eaa41123026a6955

Comment 1

[Alice0775 White]

View > Style > No Style, then CPU usage is almost 0%!

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

I see a lot of users complain about this and some of them tried to switch to Chrome. So, this must make damage to our market share.

Alice-san, do you reproduce this with clean profile?

Comment 3

[Alice0775 White]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #2)
> I see a lot of users complain about this and some of them tried to switch to
> Chrome. So, this must make damage to our market share.
> 
> Alice-san, do you reproduce this with clean profile?

Yes, I can reproduce the high CPU, sluggish and laggy scroll with new clean profile.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

I wonder if this is related to OMTC and/or e10s. They are the cause or can fix this?

Comment 5

[nodaguti]

I was experienced the slowdowns with OMTC enabled and e10s disabled.
But it's not happened after I installed the userscript found at 2ch (http://anago.2ch.net/test/read.cgi/software/1433432508/773-776) that the author says prevents <object>s from being injected by Amazon's script.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Thank you for your information!

> the author says prevents <object>s from being injected by Amazon's script.

Sounds like that the issue is bug 1172205. Although, we're not sure if this is same as bug 1172205.

Comment 7

[Alice0775 White]

I can still reproduce.

The userscript does not help for me.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Then, this is really different bug...

Comment 9

[Alice0775 White]

Attachment: Page archive for the record

Comment 10

[Alice0775 White]

Set layers.progressive-paint = true;

This page seems to be repainted and layout continuously by JavaScript.
And float element seems to require high CPU to repaint and layout.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Roc: do you know good people to investigate this issue? This must be a serious problem for marketing share in Japan since I see a lot of people complaining about this and some of them tried to switch to Google Chrome.

Comment 12

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

I looked into this a bit.

There is a JS setInterval timeout running every 100ms. It adds a class "s-item-container-height-auto" to the s-results-list-atf element (the one with the grid of results), gets some offsets, and then removes the class. I don't know exactly why it's doing this, since the code is all obfuscated, but that class forces the height of each result item (.s-item-container) to 'auto' instead of the manually-set value. So it looks like 10 times a second it's setting item heights to auto and then recomputing the layout. 

In background tabs this setInterval should be throttled to run only once a second.

I don't know why this would be so much more expensive in Firefox than in Chrome, but I guess Alice is right in comment #10, that for some reason the layout and/or restyling is a lot more expensive in Firefox than in Chrome. When I load the page in a stand-alone Firefox instance I get 20% CPU usage (combined 'Web Content' and 'firefox'), whereas in Chrome I get about 2%.

The next step is to try to reduce the Amazon content to a standalone testcase, as simple as possible, that shows that kind of performance difference.

Comment 13

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

If Alice or someone else can do that, that'd be really helpful, otherwise I can try to do it, but it'll take time and over the next week I'll be very busy at Whistler...

Comment 14

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

Attachment: benchmark testcase

I've reduced Alice's file bundle to a standalone HTML file that waits for two seconds and then toggles classes and runs layout the same way the Amazon script does. It reports about 6900ms in my Firefox build and about 120ms in Chrome ... so I think it's showing us what we need to see.

Comment 15

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

Attachment: restyling benchmark

This benchmark just tests restyling. It reports about 3000ms in Firefox and about 50ms in Chrome.

Cameron, maybe you could look into this?

Comment 16

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

On the benchmark with reflow, restyling is computing the change hint "nsChangeHint_NeedReflow | nsChangeHint_ClearAncestorIntrinsics | nsChangeHint_ClearDescendantIntrinsics | nsChangeHint_NeedDirtyReflow | nsChangeHint_ReflowChangesSizeOrPosition" for each of the 24 .s-item-container elements. We shouldn't need to clear intrinsics in this case; nsStylePosition::CalcDifference has some comments on this. In this case we should be able to get by with just nsChangeHint_NeedReflow and nsChangeHint_ReflowChangesSizeOrPosition, which would make the reflows a lot more efficient.

Comment 17

[Cameron McCormack (:heycam)]

I think it's similar to the situation in bug 1136893.  We'd be doing an eRestyle_Subtree on the #s-results-list-atf element, whose class changed, even though we really only need to restyle its descendant .s-item-container elements, since the only relevant rule is:

  ul.s-item-container-height-auto .s-item-container { background:yellow; }

Comment 18

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

Bug 522390 is where we started adding the change hints we don't want here.

I wonder if we could fix the hints for height changes follows:
1) Drop nsChangeHint_NeedDirtyReflow for standards-mode documents, since AFAICT we only added it to avoid problems with quirks-mode percentage sizing.
2) Web content should only have nsBoxFrames for content which is IsInNativeAnonymousSubtree. Keep a per-prescontext flag to indicate whether we have seen nsBoxFrames for elements that are not IsInNativeAnonymousSubtree. If we have not, and the content whose height is changing is not IsInNativeAnonymousSubtree, we can drop nsChangeHint_ClearAncestorIntrinsics.
3) I think if we set NS_FRAME_CONTAINS_RELATIVE_BSIZE on the ancestors of percentage-height intrinsic elements (which we should, right?), we only need to add nsChangeHint_ClearDescendantIntrinsics when NS_FRAME_CONTAINS_RELATIVE_BSIZE is present on the frame.

Boris, what do you think?

Comment 19

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

(In reply to Cameron McCormack (:heycam) from comment #17)
> I think it's similar to the situation in bug 1136893.

Thanks. Please fix it! :-)

Comment 20

[Alice0775 White]

amazon.com is also affected.
Open [1] in 10-12 tabs, Nightly(firefox.exe or plugin-container.exe) hogs CPU.


[1] http://www.amazon.com/s/ref=nb_sb_noss?url=search-alias%3Daps&field-keywords=Desktop+Computer&rh=i%3Aaps%2Ck%3ADesktop+Computer

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

roc, Cameron:

Thank you for your work! I'm very happy. I hope that the bug will be fixed as soon as possible because I still see some comments of some people who are thinking to switch their default browser to Chrome by this issue.

Comment 22

[Boris Zbarsky [:bzbarsky]]

> since AFAICT we only added it to avoid problems with quirks-mode percentage sizing.

Is that true?  The testcases in bug 522390 are standards-mode.  The reason we need the dirty reflow is because of bug 522390 comment 3 the discussion about anonymous scrolledframes and whatnot...

> Keep a per-prescontext flag to indicate whether we have seen nsBoxFrames

That would not help with ClearAncestorIntrinsics, I think, though the comments are a bit misleading on this score.  Consider this testcase:

  <div style="float: left">
    <div style="height: 100px">
      <img style="height: 50%" href="whatever">
    </div>
  </div>

In this case the intrinsic width of the floated div is the width of the image, which depends on its height, which depends on the height of the intermediate div there, no?

That said, we could perhaps do the thing you propose with NS_FRAME_CONTAINS_RELATIVE_BSIZE for both ancestor and descendant intrinsics; I don't recall exactly how that flag works.  But the code we're looking at here doesn't have a frame.

Comment 23

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

(In reply to Boris Zbarsky [:bz] from comment #22)
> > since AFAICT we only added it to avoid problems with quirks-mode percentage sizing.
> 
> Is that true?  The testcases in bug 522390 are standards-mode.  The reason
> we need the dirty reflow is because of bug 522390 comment 3 the discussion
> about anonymous scrolledframes and whatnot...

OK. Then I think we can fix that by propagating NS_FRAME_CONTAINS_RELATIVE_BSIZE up through anonymous scrollframes.

> > Keep a per-prescontext flag to indicate whether we have seen nsBoxFrames
> 
> That would not help with ClearAncestorIntrinsics, I think, though the
> comments are a bit misleading on this score.  Consider this testcase:
> 
>   <div style="float: left">
>     <div style="height: 100px">
>       <img style="height: 50%" href="whatever">
>     </div>
>   </div>
> 
> In this case the intrinsic width of the floated div is the width of the
> image, which depends on its height, which depends on the height of the
> intermediate div there, no?

Yes. I guess we need another state bit for "this frame or its children has (or may have) an intrinsic ISize that depends on BSize". We could extend NS_FRAME_CONTAINS_RELATIVE_BSIZE to cover that case but I think a separate bit is more sane and possibly more performant.

> That said, we could perhaps do the thing you propose with
> NS_FRAME_CONTAINS_RELATIVE_BSIZE for both ancestor and descendant
> intrinsics; I don't recall exactly how that flag works.  But the code we're
> looking at here doesn't have a frame.

That's true. I guess we need another nsChangeHint or two, say nsChangeHint_HeightChange/nsChangeHint_WidthChange, and convert them in the RestyleManager.

David, any thoughts?

Comment 24

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

Karl, can you shake the trees at Amazon to see if they can fix this on their side? Even on Chrome, they're still waking up every 100ms to do useless work, and that's bad for power consumption on any browser even if we make it not noticeable to users.

Comment 25

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

FWIW I modified the testcase so that instead of setting .s-item-container-height-auto on the ancestor element and relying on descendant selectors, it sets a new .height-auto class directly on each .s-item-container, which triggers the same height:auto!important rule on that specific element. This should minimize the amount of restyling work we have to do. With that, execution time drops from ~6800ms to ~5700ms. That suggests that most of the time is in reflow and the optimizations here are more important than the restyling optimizations --- though we'll still need restyling optimizations to get close to Chrome.

Comment 26

[Cameron McCormack (:heycam)]

Thanks, that's good to know.  I'm continuing to work on optimizing class changes on ancestors when there are matching style rules that have descendant combinators in bug 1180118.

Comment 27

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

If I just hack the height hint to nsChangeHint_NeedReflow | nsChangeHint_ReflowChangesSizeOrPosition, I get down to 2000ms, which is still 10x Chrome.

However, when I look at the profile of that, it's almost entirely restyle now. So my test changes didn't eliminate all the restyling that I'd hoped for. This is what I'm testing:

  function justReflow() {
    var start = Date.now();
    var items = document.getElementsByClassName("s-item-container");
    var item = items[0];
    for (var i = 0; i < 50; ++i) {
      for (var j = 0; j < items.length; ++j) {
        items[j].classList.add('height-auto');
      }
      v.push(item.offsetHeight);
      for (var j = 0; j < items.length; ++j) {
        items[j].classList.remove('height-auto');
      }
      v.push(item.offsetHeight);
    }
    var end = Date.now();
    alert((end - start) + "ms");
  }
  setTimeout(justReflow, 2000);

There's still a lot of RestyleChildren activity. Not sure why, but I'll leave that to Cameron to figure out.

The good news is that with those hints hacked, reflow is only about 3% of the work under setTimeout, so fixing the hints should solve the reflow side of the problem.

Comment 28

[Karl Dubost💡 :karlcow]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)
> Karl, can you shake the trees at Amazon to see if they can fix this on their
> side?

Contacted today Amazon about it. :) We will see. Thanks for pinging me.

Comment 29

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=094493ff7860

Comment 30

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

I have a set of layout patches (not the ones above, they were rather broken) that I optimistically think are close to correct.  They cut the time for the test in comment #14 from 6900ms to about 3000ms on my machine. Of that, less than 2% is now in layout.

Comment 31

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e4aade14b4c

Comment 32

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f84b85024714

Comment 33

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

Greenish on try. The only significant failure is on WinXP M5, but that was a regression introduced by https://hg.mozilla.org/integration/mozilla-inbound/rev/ec3acc5237a8 which was later backed out on inbound.

Comment 34

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

Attachment: MozReview Request: Bug 1172239. Expand height change hint to its components. r=heycam

Bug 1172239. Expand height change hint to its components. r=heycam

Comment 35

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

Attachment: MozReview Request: Bug 1172239. Make vertically-resizing scrollframes reflow their percent-height descendants if necessary, and remove nsChangeHint_NeedDirtyReflow for height changes. r=bz

Bug 1172239. Make vertically-resizing scrollframes reflow their percent-height descendants if necessary, and remove nsChangeHint_NeedDirtyReflow for height changes. r=bz

Comment 36

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

Attachment: MozReview Request: Bug 1172239. Add NS_FRAME_DESCENDANT_INTRINSIC_ISIZE_DEPENDS_ON_BSIZE. r=bz

Bug 1172239. Add NS_FRAME_DESCENDANT_INTRINSIC_ISIZE_DEPENDS_ON_BSIZE. r=bz

Comment 37

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

Attachment: MozReview Request: Bug 1172239. Use nsChangeHint_UpdateComputedBSize to only dirty intrinsic sizes when necessary. r=bz

Bug 1172239. Use nsChangeHint_UpdateComputedBSize to only dirty intrinsic sizes when necessary. r=bz

Comment 38

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

Attachment: MozReview Request: Bug 1172239. Ensure nested overflow:auto elements are tested. r=bz

Bug 1172239. Ensure nested overflow:auto elements are tested. r=bz

Comment 39

[Cameron McCormack (:heycam)]

Comment on attachment 8635694 [details]
MozReview Request: Bug 1172239. Expand height change hint to its components. r=heycam

https://reviewboard.mozilla.org/r/13589/#review12211

Comment 40

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

[Tracking Requested - why for this release]:
Not a regression, but a pretty severe bug (probably introduced by Amazon site changes) that we need to release a fix for ASAP.

Comment 41

[Lawrence Mandel [:lmandel] (use needinfo)]

Tracking 40+. We have 3 betas left in the 40 Beta cycle. Leaving 39? in order to keep this on the list of potential point release bugs.

Comment 42

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23)
> David, any thoughts?

Seems like you have a handle on it at this point.  Your solution seems reasonable to me, although a bit risky to land straight to beta.

I think we should have a followup bug on making the change hints work correctly for vertical writing modes.  I thought I saw bugmail go by, but I can't find it now.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8635695 [details]
MozReview Request: Bug 1172239. Make vertically-resizing scrollframes reflow their percent-height descendants if necessary, and remove nsChangeHint_NeedDirtyReflow for height changes. r=bz

https://reviewboard.mozilla.org/r/13591/#review12273

::: layout/style/nsStyleStruct.cpp:1599
(Diff revision 1)
>      // Height changes can affect descendant intrinsic sizes due to replaced

Please fix the comments here!

r=me with that fixed.

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8635696 [details]
MozReview Request: Bug 1172239. Add NS_FRAME_DESCENDANT_INTRINSIC_ISIZE_DEPENDS_ON_BSIZE. r=bz

https://reviewboard.mozilla.org/r/13593/#review12275

r=me

Comment 45

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8635697 [details]
MozReview Request: Bug 1172239. Use nsChangeHint_UpdateComputedBSize to only dirty intrinsic sizes when necessary. r=bz

https://reviewboard.mozilla.org/r/13595/#review12277

::: layout/base/RestyleManager.cpp:590
(Diff revision 1)
> +             dirtyType == nsIPresShell::eStyleChange) {

Hmm.  So this forces an NS_FRAME_IS_DIRTY reflow if we had a ClearDescendantIntrinsics hint.  Why do we want to do that?

Or is this not really changing behavior because every caller that passed StyleChangeReflow also passed NeedDirtyReflow?  If so, we should probably assert that in the ClearDescendantIntrinsics case, or just document that ClearDescendantIntrinsics and this new case imply dirty reflow or something and stop passing it in unnecessarily.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8635698 [details]
MozReview Request: Bug 1172239. Ensure nested overflow:auto elements are tested. r=bz

https://reviewboard.mozilla.org/r/13597/#review12279

::: layout/reftests/percent-overflow-sizing/nestedHeightQuirks.html:1
(Diff revision 1)
> +<body onload="document.getElementById('a').style.removeProperty('height');">

Please add a comment here, and in the reference, about this being purposefully in quirks mode.

::: layout/reftests/percent-overflow-sizing/nestedHeight.html:4
(Diff revision 1)
> +  <div style="overflow:auto">

Since this is auto-height, doesn't the percentage height on the kid end up auto too, no matter that the parent has a fixed height?  I'm not sure what this test is trying to test....

r=me assuming the standards-mode test is just there for symmetry with the quirks mode test.

Comment 47

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

https://reviewboard.mozilla.org/r/13595/#review12277

> Hmm.  So this forces an NS_FRAME_IS_DIRTY reflow if we had a ClearDescendantIntrinsics hint.  Why do we want to do that?
> 
> Or is this not really changing behavior because every caller that passed StyleChangeReflow also passed NeedDirtyReflow?  If so, we should probably assert that in the ClearDescendantIntrinsics case, or just document that ClearDescendantIntrinsics and this new case imply dirty reflow or something and stop passing it in unnecessarily.

If we don't do that, then when we turn nsChangeHint_UpdateComputedBSize into eStyleChange, we hit this assertion in PresShell::FrameNeedsReflow:

  NS_PRECONDITION(!(aIntrinsicDirty == eStyleChange &&
                    aBitToAdd == NS_FRAME_HAS_DIRTY_CHILDREN),
                  "bits don't correspond to style change reason");

Comment 48

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

With these patches, plus my patches in bug 1184842 (try running), I'm at 750ms for the testcase in comment #19. So 1184842 makes a huge difference.

Comment 49

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

(In reply to Boris Zbarsky [:bz] from comment #46)
> ::: layout/reftests/percent-overflow-sizing/nestedHeight.html:4
> (Diff revision 1)
> > +  <div style="overflow:auto">
> 
> Since this is auto-height, doesn't the percentage height on the kid end up
> auto too, no matter that the parent has a fixed height?  I'm not sure what
> this test is trying to test....
> 
> r=me assuming the standards-mode test is just there for symmetry with the
> quirks mode test.

Yeah, it's really just a sanity check, not testing anything that was previously broken.

Comment 50

[Boris Zbarsky [:bzbarsky]]

> we hit this assertion in PresShell::FrameNeedsReflow

Ah.  So presumably all callers that pass ClearDescendantIntrinsics already include NeedDirtyReflow too?  Can you add an explicit assert for that in this method so it's clear that the eStyleChange bit doesn't affect whether ClearDescendantIntrinsics does a dirty reflow?

Comment 51

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

(In reply to Boris Zbarsky [:bz] from comment #50)
> > we hit this assertion in PresShell::FrameNeedsReflow
> 
> Ah.  So presumably all callers that pass ClearDescendantIntrinsics already
> include NeedDirtyReflow too?

Correct.

> Can you add an explicit assert for that in
> this method so it's clear that the eStyleChange bit doesn't affect whether
> ClearDescendantIntrinsics does a dirty reflow?

Will do.

Comment 52

[Cameron McCormack (:heycam)]

Attachment: restyle log with bug 1180118 patches applied

My patches in bug 1180118 help, although by themselves not a great deal -- see bug 1180118 comment 11.

Here's a restyle log for the first style flush of the restyle benchmark (the first item.offsetHeight).  We're now succesfully avoiding doing selector matching on all descendants of the ul#s-results-list-atf except for the div.s-item-container elements, but we're not getting as much out of the bug 931668 optimizations as I'd hoped.  You can see that we continue restyling a bunch of elements under the div.s-item-container elements (we've got frames with shared style contexts and frames for generated content, both of which don't let us stop restyling).

So we end up creating a new style contexts, and resolving computed styles for, a bunch of these descendants that we still ideally wouldn't have to.  If the original page is only making non-inherited style changes on those div.s-item-container elements and no child element explicitly inherits a non-inherited property -- and from the original benchmark attachment that looks to be the case, as we just have the height:auto change and no explicit inherits on the div.s-item-containers' children -- then bug 1180120 should let us stop restyling immediately after doing the div.s-item-container.  That would dramatically reduce the restyling done.

This still depends on bug 1180118 and bug 1184842 being done.

Comment 53

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #48)
> With these patches, plus my patches in bug 1184842 (try running), I'm at
> 750ms for the testcase in comment #19. So 1184842 makes a huge difference.

That patch was totally incorrect so ignore those "results". With a more-correct path, I'm not seeing any speedup on the Amazon testcase.

Comment 54

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

https://reviewboard.mozilla.org/r/13591/#review12273

> Please fix the comments here!

The comment here is still correct. The comment goes on to explain that those are handled by nsChangeHint_UpdateComputedBSize.

Comment 55

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

Comment on attachment 8635697 [details]
MozReview Request: Bug 1172239. Use nsChangeHint_UpdateComputedBSize to only dirty intrinsic sizes when necessary. r=bz

Bug 1172239. Use nsChangeHint_UpdateComputedBSize to only dirty intrinsic sizes when necessary. r=bz

Comment 56

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

Comment on attachment 8635698 [details]
MozReview Request: Bug 1172239. Ensure nested overflow:auto elements are tested. r=bz

Bug 1172239. Ensure nested overflow:auto elements are tested. r=bz

Comment 57

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

Comment on attachment 8635698 [details]
MozReview Request: Bug 1172239. Ensure nested overflow:auto elements are tested. r=bz

https://reviewboard.mozilla.org/r/13597/#review12383

Carrying forward r+

Comment 58

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #52)
> [...] -- then bug 1180120 should
> let us stop restyling immediately after doing the div.s-item-container. 
> That would dramatically reduce the restyling done.

This didn't work, per bug 1180120 comment 2.  I'm currently pursuing bug 1186291 so that the ReparentStyleContext calls of the descendants of the element whose non-inherited property changed are cheaper.

Comment 59

[Boris Zbarsky [:bzbarsky]]

https://reviewboard.mozilla.org/r/13595/#review12277

> If we don't do that, then when we turn nsChangeHint_UpdateComputedBSize into eStyleChange, we hit this assertion in PresShell::FrameNeedsReflow:
> 
>   NS_PRECONDITION(!(aIntrinsicDirty == eStyleChange &&
>                     aBitToAdd == NS_FRAME_HAS_DIRTY_CHILDREN),
>                   "bits don't correspond to style change reason");

OK, so with the explicit asserts discussed in comment 50 and comment 51, looks good.

Comment 60

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8635697 [details]
MozReview Request: Bug 1172239. Use nsChangeHint_UpdateComputedBSize to only dirty intrinsic sizes when necessary. r=bz

https://reviewboard.mozilla.org/r/13595/#review12553

Comment 61

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

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/3967fdbe67082f842e3305ac278d601a55884c2a
changeset:  3967fdbe67082f842e3305ac278d601a55884c2a
user:       Robert O'Callahan <[e-mail]>
date:       Fri Jul 17 15:21:29 2015 +1200
description:
Bug 1172239. Expand height change hint to its components. r=heycam

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/9cce849f4a1e44d71006936481d51fa4736758c8
changeset:  9cce849f4a1e44d71006936481d51fa4736758c8
user:       Robert O'Callahan <[e-mail]>
date:       Fri Jul 17 17:08:54 2015 +1200
description:
Bug 1172239. Make vertically-resizing scrollframes reflow their percent-height descendants if necessary, and remove nsChangeHint_NeedDirtyReflow for height changes. r=bz

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/5d02adf03ecee75f8eadbd6c8a524827d4dc81ea
changeset:  5d02adf03ecee75f8eadbd6c8a524827d4dc81ea
user:       Robert O'Callahan <[e-mail]>
date:       Sat Jul 18 12:24:53 2015 +1200
description:
Bug 1172239. Add NS_FRAME_DESCENDANT_INTRINSIC_ISIZE_DEPENDS_ON_BSIZE. r=bz

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/c256fda0789dfc719f8dbec2ac13db1508030e88
changeset:  c256fda0789dfc719f8dbec2ac13db1508030e88
user:       Robert O'Callahan <[e-mail]>
date:       Wed Jul 22 16:36:56 2015 +1200
description:
Bug 1172239. Use nsChangeHint_UpdateComputedBSize to only dirty intrinsic sizes when necessary. r=bz

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/09878dde0cc89f979c98a4ac00ad3d7053b8702f
changeset:  09878dde0cc89f979c98a4ac00ad3d7053b8702f
user:       Robert O'Callahan <[e-mail]>
date:       Wed Jul 22 16:37:00 2015 +1200
description:
Bug 1172239. Ensure nested overflow:auto elements are tested. r=bz

Comment 62

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #58)
> This didn't work, per bug 1180120 comment 2.  I'm currently pursuing bug
> 1186291 so that the ReparentStyleContext calls of the descendants of the
> element whose non-inherited property changed are cheaper.

This also didn't work as well as I'd hoped -- I only got a 100ms reduction (from 1800ms to 1700ms) on the restyling benchmark.

But I went back to bug 1172239 and have a WIP patch (whose correctness I'm still unsure of) which does prevent the children of the div.s-item-container from being restyled, by detecting that it's a change to a non-inherited property and knowing that no descendants currently rely on the changed reset style.  With the WIP v0.2 patch from bug 1180118 applied, a hack that simulates the effect of the patches in bug 1184842 (I haven't rebased over the final patches there yet), and the aforemented WIP patch for bug 1172239, the restyling benchmark time is down to 180ms on my machine.

Comment 63

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #62)
> But I went back to bug 1172239 and have a WIP patch (whose correctness I'm
> still unsure of) which does prevent the children of the div.s-item-container
> from being restyled, by detecting that it's a change to a non-inherited
> property and knowing that no descendants currently rely on the changed reset
> style.  With the WIP v0.2 patch from bug 1180118 applied, a hack that
> simulates the effect of the patches in bug 1184842 (I haven't rebased over
> the final patches there yet), and the aforemented WIP patch for bug 1172239,
> the restyling benchmark time is down to 180ms on my machine.

s/bug 1172239/bug 1180120/g in the above.

Comment 64

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3967fdbe6708
https://hg.mozilla.org/mozilla-central/rev/9cce849f4a1e
https://hg.mozilla.org/mozilla-central/rev/5d02adf03ece
https://hg.mozilla.org/mozilla-central/rev/c256fda0789d
https://hg.mozilla.org/mozilla-central/rev/09878dde0cc8

Comment 65

[Lawrence Mandel [:lmandel] (use needinfo)]

40 and 41 are marked as affected but reading comment 58 and comment 61 I don't know if it is worth the risk of uplift to take this in beta8. 

roc - What do you think about the risk and value of uplifting this fix to beta8?

Comment 66

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

I don't know either. It's a tough call. These patches alone don't even fix the whole issue, though they should reduce the load by more than half. I would lean towards taking it, since the problem is severe, no regressions have been reported yet, and it's very likely that any regressions can be fixed by a simple and safe revert in nsStylePosition::CalcDifference.

Comment 67

[Cameron McCormack (:heycam)]

And I think of the three bugs required to fix the whole issue, this bug's patches are the least likely to cause regressions.  The other two involve somewhat invasive changes to restyle processing, and I'd be wary of uplifting them.

Comment 68

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

Comment on attachment 8635697 [details]
MozReview Request: Bug 1172239. Use nsChangeHint_UpdateComputedBSize to only dirty intrinsic sizes when necessary. r=bz

Approval Request Comment
[Feature/regressing bug #]: None
[User impact if declined]: Very bad performance on Amazon pages. Actually performance will be bad with these changes, but worse without them.
[Describe test coverage new/current, TreeHerder]: We have many automated tests covering this code, though it's complex code and there is potential for unknown regressions.
[Risks and why]: This is a relatively risky change to core layout code for beta. The argument for making the change is that the problem is severe on a major site and does not occur in Chrome.
[String/UUID change made/needed]: None.

Comment 69

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #68)
> [Feature/regressing bug #]: None

Any idea when this issue started? Is this a recent issue due to an Amazon site change? If this issue has been around for a while it may be better to push for a more complete fix in 41 and not take the risk of further regression in 40.

Comment 70

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8635697 [details]
MozReview Request: Bug 1172239. Use nsChangeHint_UpdateComputedBSize to only dirty intrinsic sizes when necessary. r=bz

I'm going to go with Roc's recommendation and take this fix in beta8. We should check the results with beta8 and revert the fix as described in comment 66 in beta9 if need be. Beta+ Aurora+

Comment 71

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

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #42)
> I think we should have a followup bug on making the change hints work
> correctly for vertical writing modes.  I thought I saw bugmail go by, but I
> can't find it now.

I filed bug 1188061 on this.

Comment 72

[Liz Henry (:lizzard) (relman/hg->git project)]

Doing some cleanup for 39 tracking requests. I don't think we need to track this for 39, since 40 should be released in a couple of weeks.

Comment 73

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d28eee5f0702
https://hg.mozilla.org/releases/mozilla-aurora/rev/ca132b351d4d
https://hg.mozilla.org/releases/mozilla-aurora/rev/31611d94a1cc
https://hg.mozilla.org/releases/mozilla-aurora/rev/f16ed3a6a2c9
https://hg.mozilla.org/releases/mozilla-aurora/rev/5d69d033da18

Comment 74

[Ryan VanderMeulen [:RyanVM]]

Patch 3 has conflicts on beta.

Comment 75

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

I got the patches rebased but I guess I missed beta8 and now the tree is closed :-(

Comment 76

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

Ryan corrected me. I pushed to beta, but I guess the changes won't be in beta8 unless it respins.

Comment 77

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dd9232b36d1a
https://hg.mozilla.org/releases/mozilla-beta/rev/4d8514b036ba
https://hg.mozilla.org/releases/mozilla-beta/rev/7ee5b25607b4
https://hg.mozilla.org/releases/mozilla-beta/rev/0ebb7a9bc84c
https://hg.mozilla.org/releases/mozilla-beta/rev/c4efe7244ef3

Comment 78

[Sylvestre Ledru [:Sylvestre]]

Roc, as discussed in bug 1190635, I think the safest move would be to backout this patch from beta. Is that OK with you?

Comment 79

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

Yes.

Comment 80

[Sylvestre Ledru [:Sylvestre]]

Thanks. 
Ryan, could you backout the 5 patches from m-r?

Comment 81

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9a114caf7a29

Will merge to m-r when green on Treeherder.

Comment 82

[Carsten Book [:Tomcat]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #81)
> https://hg.mozilla.org/releases/mozilla-beta/rev/9a114caf7a29
> 
> Will merge to m-r when green on Treeherder.

and was green on TH and so merged to m-r -> https://hg.mozilla.org/releases/mozilla-release/rev/9a114caf7a29

Comment 83

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

Now that the relevant style system bugs are fixed in Nightly, Amazon should be good in Nightly. Can anyone independently confirm?

Comment 84

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

Er, sorry, bug 1191600 is not yet fixed in Nightly. Cameron, do we need that for Amazon?

Comment 85

[Cameron McCormack (:heycam)]

No that patch isn't needed for Amazon.  (But thanks for reminding me to request review on that patch.)

Comment 86

[Alice0775 White]

Reproduced with Firerfox39.

And I cannot reproduce the high CPU problem any more. CPU usage is quite reduced on Nightly43.0a1.
CPU Usage: 0-5%/8tabs

https://hg.mozilla.org/mozilla-central/rev/f2518b8a7b97b5bb477e94bc9131584007aac887
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0 ID:20150831030209

Comment 87

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

Excellent. Thank you Alice!

Comment 88

[Cameron McCormack (:heycam)]

[Tracking Requested - why for this release]:
Resetting tracking flag for 42, since to completely resolve the issue we need to take all the dependent bugs' patches too.  Currently that includes bug 1202512, bug 1192302 and bug 1180120.