1381844 - 2.92% quantum_pageload_amazon (windows7-32) regression on push e9ec92fd4f47d69cec602e81bb283d045b6bd87a (Sat Jul 15 2017)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Ionuț Goldan [:igoldan]]

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

Comment 1

[Ionuț Goldan [:igoldan]]

:emilio Is this change a temporary one? Will you eventually back out it, after diagnosing the bugs you're interested in?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 3

[Bobby Holley (:bholley)]

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.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1381844: Remove a few redundant casts.

Review commit: https://reviewboard.mozilla.org/r/159832/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159832/

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1381844: Be more explicit about the kind of style context we handle all the time.

Review commit: https://reviewboard.mozilla.org/r/159834/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159834/

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8888803 [details]
Bug 1381844: Remove a few redundant casts.

https://reviewboard.mozilla.org/r/159832/#review165300

Comment 8

[Bobby Holley (:bholley)]

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.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8888803 [details]
Bug 1381844: Remove a few redundant casts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159832/diff/1-2/

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8888804 [details]
Bug 1381844: Be more explicit about the kind of style context we handle all the time.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159834/diff/1-2/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c9ee2503c120
https://hg.mozilla.org/mozilla-central/rev/e8691885ec73

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

these changes don't fix the regression, is there other work we can think of?

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Yup, I'm working through the correct fix in bug 1383634.

Comment 15

[Kevin Brosnan [:kbrosnan]]

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.

Comment 16

[Bobby Holley (:bholley)]

(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

[Ryan VanderMeulen [:RyanVM]]

Is this fixed now?

Comment 18

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 19

[Joel Maher ( :jmaher ) (UTC -8)]

did a try push on tip, + a backout: https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&fromchange=e8e439a7ab3de22a98c9a8ebc3febcd7787f2cef&tochange=653606b74cf8e41e382e5ace927a727c61a06944

Comment 20

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 21

[Bobby Holley (:bholley)]

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.

Comment 22

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 23

[Bobby Holley (:bholley)]

(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!

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/40807a516e5e
Remove diagnostic assertions. r=me

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/40807a516e5e

Comment 26

[Jet Villegas (inactive)]

Should this now be closed/fixed?

Comment 27

[Xidorn Quan [:xidorn] UTC+11]

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

[Xidorn Quan [:xidorn] UTC+11]

Comment 27 is just a FYI. I don't think we need to change that code since it's going away anyway...