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)

56 Branch
defect
Not set
normal

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
Component: Untriaged → CSS Parsing and Computation
Product: Firefox → Core
: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)
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)
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)
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 on attachment 8888803 [details]
Bug 1381844: Remove a few redundant casts.

https://reviewboard.mozilla.org/r/159832/#review165300
Attachment #8888803 - Flags: review?(bobbyholley) → 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+
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
these changes don't fix the regression, is there other work we can think of?
Flags: needinfo?(emilio+bugs)
Yup, I'm working through the correct fix in bug 1383634.
Depends on: 1383634
Flags: needinfo?(emilio+bugs)
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.
(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?
Is this fixed now?
Assignee: nobody → emilio+bugs
Flags: needinfo?(emilio+bugs)
Version: unspecified → 56 Branch
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)
Flags: needinfo?(jmaher)
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)
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)
(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
Flags: needinfo?(ionut.goldan)
Keywords: leave-open
Target Milestone: --- → mozilla57
Flags: needinfo?(ionut.goldan)
Should this now be closed/fixed?
Flags: needinfo?(bobbyholley)
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(bobbyholley)
Resolution: --- → FIXED
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 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.