Closed
Bug 1381844
Opened 6 years ago
Closed 6 years ago
2.92% quantum_pageload_amazon (windows7-32) regression on push e9ec92fd4f47d69cec602e81bb283d045b6bd87a (Sat Jul 15 2017)
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | disabled |
firefox57 | --- | fixed |
People
(Reporter: igoldan, Assigned: bholley)
References
Details
(Keywords: perf, regression, talos-regression)
Attachments
(2 files)
Talos has detected a Firefox performance regression from push: https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=e9ec92fd4f47d69cec602e81bb283d045b6bd87a As author of one of the patches included in that push, we need your help to address this regression. Regressions: 3% quantum_pageload_amazon summary windows7-32 opt e10s 2,993.02 -> 3,080.54 You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=8035 On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the Talos jobs in a pushlog format. To learn more about the regressing test(s), please see: https://wiki.mozilla.org/Buildbot/Talos/Tests For information on reproducing and debugging the regression, either on try or locally, see: https://wiki.mozilla.org/Buildbot/Talos/Running *** Please let us know your plans within 3 business days, or the offending patch(es) will be backed out! *** Our wiki page outlines the common responses and expectations: https://wiki.mozilla.org/Buildbot/Talos/RegressionBugsHandling
Updated•6 years ago
|
Component: Untriaged → CSS Parsing and Computation
Product: Firefox → Core
Reporter | ||
Comment 1•6 years ago
|
||
:emilio Is this change a temporary one? Will you eventually back out it, after diagnosing the bugs you're interested in?
Flags: needinfo?(emilio+bugs)
Comment 2•6 years ago
|
||
Yeah, so it's temporary, but I'd really want the assertions to be around for a bit... depending on how prioritary is it to get the regressions fixed (it's a diagnostic assert, so I thought not too much, but maybe things are different?) I can either investigate it and find why it's regressing that particular test¸ get them back to plain MOZ_ASSERT I suppose.
Flags: needinfo?(emilio+bugs)
Assignee | ||
Comment 3•6 years ago
|
||
I think we need to do something about this. These are the core metrics for 57 and so we shouldn't regress them for an extended period of time. We have a few days worth of crash data with these assertions enabled, and we can re-enable them for a few days down the line if we have reason to suspect we're getting it wrong. We could either investigate which path is hot enough for this to matter (which might indicate other nice avenues for optimization), or we can make the assertions debug-only. Up to you.
Flags: needinfo?(emilio+bugs)
Comment 4•6 years ago
|
||
I've investigated these and they're mostly the StyleContext casts... I went and moved a bunch of them up the callstack. If this doesn't make things better (which could be quite possible), I'll just put the assertions back as debug assertions.
Flags: needinfo?(emilio+bugs)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•6 years ago
|
||
mozreview-review |
Comment on attachment 8888803 [details] Bug 1381844: Remove a few redundant casts. https://reviewboard.mozilla.org/r/159832/#review165300
Attachment #8888803 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8888804 [details] Bug 1381844: Be more explicit about the kind of style context we handle all the time. https://reviewboard.mozilla.org/r/159834/#review165314 That's not a small patch. :-) This will make it a lot simpler to rip out the old style system down the road. Thanks for doing this! r=me with those nits addressed. ::: layout/base/GeckoRestyleManager.cpp:925 (Diff revision 1) > - nsStyleContext* prevStyle = prevContinuation->StyleContext(); > - nsStyleContext* selfStyle = aFrame->StyleContext(); > + auto* prevStyle = prevContinuation->StyleContext(); > + auto* selfStyle = aFrame->StyleContext(); Hm, why this change? ::: layout/base/GeckoRestyleManager.cpp:960 (Diff revision 1) > // tree has already been changed, so this check would just fail. > - nsStyleContext* oldContext = aFrame->StyleContext(); > + GeckoStyleContext* oldContext = aFrame->StyleContext()->AsGecko(); > > - RefPtr<nsStyleContext> newContext; > + RefPtr<GeckoStyleContext> newContext; > nsIFrame* providerFrame; > - nsStyleContext* newParentContext = aFrame->GetParentStyleContext(&providerFrame); > + auto* newParentContext = Same here. ::: layout/base/nsCSSFrameConstructor.cpp:1943 (Diff revision 1) > GeckoRestyleManager::ReframingStyleContexts* rsc = > geckoRM->GetReframingStyleContexts(); > + > if (rsc) { > - nsStyleContext* oldStyleContext = rsc->Get(container, aPseudoElement); > - if (oldStyleContext) { > + RefPtr<GeckoStyleContext> newContext = > + already_AddRefed<GeckoStyleContext>(pseudoStyleContext.forget().take()->AsGecko()); Seems like this RefPtr-conversion logic (which appears in a few places) might be nice to encapsulate as a static method on nsStyleContext (accepting a RefPtr by RValue-reference to consume it). ::: layout/style/nsStyleSet.cpp:1852 (Diff revision 1) > - RefPtr<nsStyleContext> result = ResolveStyleForInternal(aTarget, > + RefPtr<GeckoStyleContext> result = ResolveStyleForInternal(aTarget, > aParentContext, > treeContext, > eWithoutAnimation); Nit: Indentation.
Attachment #8888804 -
Flags: review?(bobbyholley) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 11•6 years ago
|
||
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c9ee2503c120 Remove a few redundant casts. r=bholley https://hg.mozilla.org/integration/autoland/rev/e8691885ec73 Be more explicit about the kind of style context we handle all the time. r=bholley
Updated•6 years ago
|
Keywords: leave-open
![]() |
||
Comment 12•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c9ee2503c120 https://hg.mozilla.org/mozilla-central/rev/e8691885ec73
Comment 13•6 years ago
|
||
these changes don't fix the regression, is there other work we can think of?
Flags: needinfo?(emilio+bugs)
Comment 14•6 years ago
|
||
Yup, I'm working through the correct fix in bug 1383634.
Depends on: 1383634
Flags: needinfo?(emilio+bugs)
Comment 15•6 years ago
|
||
Note that it may be difficult to tell when this is fixed due to changes in how we measure first non blank in tp6 bug 1363845.
Assignee | ||
Comment 16•6 years ago
|
||
(In reply to Kevin Brosnan [:kbrosnan] from comment #15) > Note that it may be difficult to tell when this is fixed due to changes in > how we measure first non blank in tp6 bug 1363845. Seems like we could compare trunk against a try push backing out bug 1380789?
Comment 17•6 years ago
|
||
Is this fixed now?
Assignee: nobody → emilio+bugs
status-firefox55:
--- → unaffected
status-firefox56:
--- → affected
status-firefox-esr52:
--- → unaffected
Flags: needinfo?(emilio+bugs)
Version: unspecified → 56 Branch
Comment 18•6 years ago
|
||
I think it should be better now, though maybe there's still some overhead... I'm not super-familiar with talos, but happy to do the comparison suggested in comment 16 to see if it persists.
Flags: needinfo?(emilio+bugs)
Updated•6 years ago
|
Flags: needinfo?(jmaher)
Comment 19•6 years ago
|
||
did a try push on tip, + a backout: https://treeherder.mozilla.org/#/jobs?repo=try&author=jmaher@mozilla.com&fromchange=e8e439a7ab3de22a98c9a8ebc3febcd7787f2cef&tochange=653606b74cf8e41e382e5ace927a727c61a06944
Comment 20•6 years ago
|
||
the backout shows a 6% improvement: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=e8e439a7ab3d&newProject=try&newRevision=653606b74cf8e41e382e5ace927a727c61a06944&framework=1&showOnlyImportant=0 so I would say this isn't fixed yet
Flags: needinfo?(jmaher)
Updated•6 years ago
|
status-firefox57:
--- → affected
Assignee | ||
Comment 21•6 years ago
|
||
Ok. Emilio, I think we should land the backout in that case. We haven't seen any of these on crash-stats AFAICT, so I don't think the tradeoff is worth it. 6% on amazon is very large, and we're spending a lot of time agonizing over these numbers.
Flags: needinfo?(emilio+bugs)
Comment 22•6 years ago
|
||
Yup, I agree, sounds fine I guess... We can consider re-introducing the asserts in everything but style contexts or such in the future, I suppose.
Flags: needinfo?(emilio+bugs)
Assignee | ||
Comment 23•6 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #22) > Yup, I agree, sounds fine I guess... We can consider re-introducing the > asserts in everything but style contexts or such in the future, I suppose. K thanks!
Assignee: emilio+bugs → bobbyholley
Comment 24•6 years ago
|
||
Pushed by bholley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/40807a516e5e Remove diagnostic assertions. r=me
Comment 25•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/40807a516e5e
Reporter | ||
Updated•6 years ago
|
Flags: needinfo?(ionut.goldan)
Updated•6 years ago
|
Reporter | ||
Updated•6 years ago
|
Flags: needinfo?(ionut.goldan)
Assignee | ||
Updated•6 years ago
|
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(bobbyholley)
Resolution: --- → FIXED
Comment 27•6 years ago
|
||
mozreview-review |
Comment on attachment 8888804 [details] Bug 1381844: Be more explicit about the kind of style context we handle all the time. https://reviewboard.mozilla.org/r/159834/#review228080 ::: layout/style/GeckoStyleContext.h:16 (Diff revision 2) > - GeckoStyleContext(nsStyleContext* aParent, > + static already_AddRefed<GeckoStyleContext> > + TakeRef(already_AddRefed<nsStyleContext> aStyleContext) > + { > + auto* context = aStyleContext.take(); > + MOZ_ASSERT(context); > + > + return already_AddRefed<GeckoStyleContext>(context->AsGecko()); > + } This function isn't really necessary. You can simply do `context.forget().downcast<GeckoStyleContext>()` to generate `already_AddRefed<GeckoStyleContext>` from `already_AddRefed<nsStyleContext>`.
Comment 28•6 years ago
|
||
Comment 27 is just a FYI. I don't think we need to change that code since it's going away anyway...
You need to log in
before you can comment on or make changes to this bug.
Description
•