Closed Bug 1172239 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox39 - wontfix
firefox40 + wontfix
firefox41 + fixed
firefox42 + fixed

People

(Reporter: alice0775, Assigned: roc)

References

(Depends on 3 open bugs, )

Details

(Keywords: jp-critical, perf)

Attachments

(9 files)

250.76 KB, application/x-compress-7z
Details
518.29 KB, text/html
Details
518.36 KB, text/html
Details
40 bytes, text/x-review-board-request
heycam
: review+
Details
40 bytes, text/x-review-board-request
bzbarsky
: review+
Details
40 bytes, text/x-review-board-request
bzbarsky
: review+
Details
40 bytes, text/x-review-board-request
bzbarsky
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
6.41 MB, text/plain
Details
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
Keywords: perf
View > Style > No Style, then CPU usage is almost 0%!
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?
Severity: normal → major
Flags: needinfo?(alice0775)
Keywords: jp-critical
(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.
Flags: needinfo?(alice0775)
I wonder if this is related to OMTC and/or e10s. They are the cause or can fix this?
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.
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.
I can still reproduce.

The userscript does not help for me.
Then, this is really different bug...
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.
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.
Flags: needinfo?(roc)
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.
Flags: needinfo?(roc)
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...
Keywords: testcase-wanted
Attached file 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.
Attached file 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?
Attachment #8629189 - Flags: feedback?(cam)
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.
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; }
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?
Flags: needinfo?(bzbarsky)
(In reply to Cameron McCormack (:heycam) from comment #17)
> I think it's similar to the situation in bug 1136893.

Thanks. Please fix it! :-)
Depends on: 1136893
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
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.
> 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.
Flags: needinfo?(bzbarsky)
(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?
Flags: needinfo?(dbaron)
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.
Flags: needinfo?(kdubost)
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.
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.
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.
(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.
Flags: needinfo?(kdubost)
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.
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.
Bug 1172239. Make vertically-resizing scrollframes reflow their percent-height descendants if necessary, and remove nsChangeHint_NeedDirtyReflow for height changes. r=bz
Attachment #8635695 - Flags: review?(bzbarsky)
Bug 1172239. Add NS_FRAME_DESCENDANT_INTRINSIC_ISIZE_DEPENDS_ON_BSIZE. r=bz
Attachment #8635696 - Flags: review?(bzbarsky)
Bug 1172239. Use nsChangeHint_UpdateComputedBSize to only dirty intrinsic sizes when necessary. r=bz
Attachment #8635697 - Flags: review?(bzbarsky)
Assignee: nobody → roc
Attachment #8635694 - Flags: review?(cam) → review+
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
[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.
Component: General → Layout
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.
(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.
Flags: needinfo?(dbaron)
Attachment #8635695 - Flags: review?(bzbarsky) → review+
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 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
Attachment #8635696 - Flags: review?(bzbarsky) → review+
Attachment #8635697 - Flags: review?(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 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.
Attachment #8635698 - Flags: review?(bzbarsky) → review+
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");
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.
Depends on: 1184842
(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.
> 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?
(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.
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.
(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.
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 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
Attachment #8635697 - Flags: review?(bzbarsky)
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
Attachment #8635698 - Flags: review+ → review?(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/#review12383

Carrying forward r+
Attachment #8635698 - Flags: review+
(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.
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 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
Attachment #8635697 - Flags: review?(bzbarsky) → review+
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/3967fdbe67082f842e3305ac278d601a55884c2a
changeset:  3967fdbe67082f842e3305ac278d601a55884c2a
user:       Robert O'Callahan <robert@ocallahan.org>
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 <robert@ocallahan.org>
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 <robert@ocallahan.org>
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 <robert@ocallahan.org>
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 <robert@ocallahan.org>
date:       Wed Jul 22 16:37:00 2015 +1200
description:
Bug 1172239. Ensure nested overflow:auto elements are tested. r=bz
(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.
(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.
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?
Flags: needinfo?(roc)
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.
Flags: needinfo?(roc)
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 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.
Attachment #8635697 - Flags: approval-mozilla-beta?
Attachment #8635697 - Flags: approval-mozilla-aurora?
(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.
Flags: needinfo?(roc)
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+
Attachment #8635697 - Flags: approval-mozilla-beta?
Attachment #8635697 - Flags: approval-mozilla-beta+
Attachment #8635697 - Flags: approval-mozilla-aurora?
Attachment #8635697 - Flags: approval-mozilla-aurora+
Flags: needinfo?(roc)
(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.
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.
Patch 3 has conflicts on beta.
Flags: needinfo?(roc)
I got the patches rebased but I guess I missed beta8 and now the tree is closed :-(
Flags: needinfo?(roc)
Ryan corrected me. I pushed to beta, but I guess the changes won't be in beta8 unless it respins.
Depends on: 1190635
Roc, as discussed in bug 1190635, I think the safest move would be to backout this patch from beta. Is that OK with you?
Flags: needinfo?(roc)
Yes.
Flags: needinfo?(roc)
Thanks. 
Ryan, could you backout the 5 patches from m-r?
Flags: needinfo?(ryanvm)
https://hg.mozilla.org/releases/mozilla-beta/rev/9a114caf7a29

Will merge to m-r when green on Treeherder.
Flags: needinfo?(ryanvm)
(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
Attachment #8635697 - Flags: approval-mozilla-beta+
Now that the relevant style system bugs are fixed in Nightly, Amazon should be good in Nightly. Can anyone independently confirm?
Flags: needinfo?(alice0775)
Er, sorry, bug 1191600 is not yet fixed in Nightly. Cameron, do we need that for Amazon?
Flags: needinfo?(alice0775) → needinfo?(cam)
No that patch isn't needed for Amazon.  (But thanks for reminding me to request review on that patch.)
Flags: needinfo?(cam) → needinfo?(alice0775)
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
Flags: needinfo?(alice0775)
Excellent. Thank you Alice!
[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.
Depends on: 1209994
Depends on: 1242172
Depends on: 1242461
Depends on: 1258659
Depends on: 1355837
No longer depends on: 1355837
Depends on: 1351062
Depends on: 1482938
You need to log in before you can comment on or make changes to this bug.