1207143 - 2-4% Linux 64/Win* tsvgx regression on Mozilla-Inbound (v.44) on September 21, 2015 from push 621ab19e86db

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

Talos has detected a Firefox performance regression from your commit 621ab19e86db in bug 1026520.  We need you to address this regression.

This is a list of all known regressions and improvements related to your bug:
http://alertmanager.allizom.org:8080/alerts.html?rev=4313752f69956ae248bd4e7ff3913c8dd4252698&showAll=1

On the page above you can see Talos 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.

You can compare this change to the previous push:
https://treeherder.allizom.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=7641104770a8&newProject=mozilla-inbound&newRevision=621ab19e86db

* data is still being collected, it might take a day or so to get all the data generated.

To learn more about the regressing test, please see: https://wiki.mozilla.org/Buildbot/Talos/Tests#tsvg.2C_tsvgx

Reproducing and debugging the regression:
If you would like to re-run this Talos test on a potential fix, use try with the following syntax:
try: -b o -p linux64,win64,win32 -u none -t svgr  # add "mozharness: --spsProfile" to generate profile data

To run the test locally and do a more in-depth investigation, first set up a local Talos environment:
https://wiki.mozilla.org/Buildbot/Talos/Running#Running_locally_-_Source_Code

Then run the following command from the directory where you set up Talos:
talos --develop -e <path>/firefox -a tsvgx

Making a decision:
As the patch author we need your feedback to help us handle this regression.
*** Please let us know your plans by Friday, or the offending patch will be backed out! ***

Our wiki page oulines the common responses and expectations:
https://wiki.mozilla.org/Buildbot/Talos/RegressionBugsHandling

Comment 1

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

:sinker, can you look into this regression?  As noted above we would really like to understand this regression this week or back it out until have more time to understand it.

Comment 2

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

(In reply to Joel Maher (:jmaher) from comment #0)
> Talos has detected a Firefox performance regression from your commit
> 621ab19e86db in bug 1026520.  We need you to address this regression.

That changset (which I'm assuming is the one you intended based on summary and component and needinfo?) is actually from bug 1097464.

> This is a list of all known regressions and improvements related to your bug:
> http://alertmanager.allizom.org:8080/alerts.
> html?rev=4313752f69956ae248bd4e7ff3913c8dd4252698&showAll=1

This URL should be:

http://alertmanager.allizom.org:8080/alerts.html?rev=621ab19e86db28c38bbbf9119fbf6831ea344c54&showAll=1


Did you mean to file a separate bug on the regressions from bug 1026520?

Comment 3

[Thinker Li [:sinker]]

ok!

Comment 4

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

:dbaron, thanks for the comment. I had replied a couple days ago, but it appears I didn't save the changes or they didn't make it through for some reason.

You are correct, this is not related to bug 1026520, it was a typo on my part.  Thanks for catching that.  Lastly the list of alerts generated are now complete on:
http://alertmanager.allizom.org:8080/alerts.html?rev=621ab19e86db28c38bbbf9119fbf6831ea344c54&showAll=1

Originally they were not there, but in backfilling data to ensure it was complete we filled in the holes and generated data properly.

:sinker, thanks for looking into this.  If you have questions about the tests or how to interpret try server, do let me know

Comment 5

[Thinker Li [:sinker]]

Attachment: Improve GetBounds() to avoid recomputing every time

Comment 6

[Thinker Li [:sinker]]

(In reply to Thinker Li [:sinker] from comment #5)
> Created attachment 8666485 [details] [diff] [review]
> Improve GetBounds() to avoid recomputing every time

With this patch, it removes 2~3% at my laptop remaining 0.2% of diff.

Comment 7

[Thinker Li [:sinker]]

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

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=7641104770a8&newProject=try&newRevision=3e814a5ab1c2

Comment 8

[Thinker Li [:sinker]]

Attachment: Improve GetBounds() to avoid recomputing every time, v2

--
Attachment #8666485 [details] [diff] - Attachment is obsolete: true

Comment 9

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

:sinker, thanks for fixing this!

Comment 10

[Carsten Book [:Tomcat]]

hey :sinker, this patch had the wrong bug number in it and also i had to back this out after checkin for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=14793633&repo=mozilla-inbound

Comment 11

[Thinker Li [:sinker]]

ok!

Comment 12

[Thinker Li [:sinker]]

Attachment: Improve GetBounds() to avoid recomputing every time, v3

--
Attachment #8666596 [details] [diff] - Attachment is obsolete: true

Comment 13

[Thinker Li [:sinker]]

Attachment: Improve GetBounds() to avoid recomputing every time, v4

--
Attachment #8668802 [details] [diff] - Attachment is obsolete: true

Comment 14

[Thinker Li [:sinker]]

Comment on attachment 8668883 [details] [diff] [review]
Improve GetBounds() to avoid recomputing every time, v4

Use a deterministic way to update bounds of transform items.  All transform items in a preserves3d context are updated when the item starting context chains is created.

Comment 15

[Thinker Li [:sinker]]

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=7641104770a8&newProject=try&newRevision=4e64b07e091f

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

Comment 16

[Thinker Li [:sinker]]

Attachment: Improve GetBounds() to avoid recomputing every time, v5

--
Attachment #8668883 [details] [diff] - Attachment is obsolete: true

Comment 17

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

Comment on attachment 8669235 [details] [diff] [review]
Improve GetBounds() to avoid recomputing every time, v5

Review of attachment 8669235 [details] [diff] [review]:
-----------------------------------------------------------------

This code is potentially very confusing so it's important we try very hard to use clear terminology.

::: layout/base/nsDisplayList.h
@@ +1613,5 @@
> +   * computed from calling DoUpdateBoundsPreserves3D() of all
> +   * descendants in the same context.  This function travels
> +   * recursively to all items in the context for this purpose.
> +   */
> +  virtual void DoUpdateBoundsPreserves3D(nsDisplayListBuilder* aBuilder) {}

The comment and name here should be more precise. What exactly does "in a preserves3d context" mean? And what exactly is a "3D context chain"?

@@ +3895,5 @@
> +
> +  /**
> +   * This function updates bounds for items starting 3D context chains.
> +   *
> +   * \see Bug nsDisplayItem::DoUpdateBoundsPreserves3D()

Delete "Bug"

@@ +3912,5 @@
> +   * This item is an additional item as the boundary between parent
> +   * and child 3D context.
> +   * \see nsIFrame::BuildDisplayListForStackingContext().
> +   */
> +  bool IsAdditionalSeparator() { return mHasPresetTransform; }

We should rename mHasPresetTransform to mIsTransformSeparator. And call this method IsTransformSeparator and give an example in the comment of when it would be used.

@@ +3916,5 @@
> +  bool IsAdditionalSeparator() { return mHasPresetTransform; }
> +  /**
> +   * This item is the boundary between parent and child 3D context.
> +   */
> +  bool Is3DContextBoundary() {

This needs to be more precise and use a more precise name since "3D context boundary" is not defined anywhere. Can we call this "IsLeafOf3DContext"?

@@ +3925,5 @@
> +  /**
> +   * For true, this item is one of start points of 3D context chains
> +   * of a preserves3D context.
> +   */
> +  bool Start3DContextChain() {

This also needs to be more precise since "3D context chain" is not a defined term anywhere as far as I know.

How about "IsRootOf3DContext"?

Comment 18

[Thinker Li [:sinker]]

Attachment: Improve GetBounds() to avoid recomputing every time, v6

--
Attachment #8669235 [details] [diff] - Attachment is obsolete: true

Comment 19

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

Comment on attachment 8669624 [details] [diff] [review]
Improve GetBounds() to avoid recomputing every time, v6

Review of attachment 8669624 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsDisplayList.h
@@ +1606,5 @@
> +   * 3D rendering context.
> +   *
> +   * This function is called by UpdateBoundsPreserves3D() of
> +   * nsDisplayTransform(), it is called by
> +   * BuildDisplayListForStackingContext() in turn on transform items

", and it is called by BuildDisplayListForStackingContext() on transform items"

@@ +3645,5 @@
> +    }
> +
> +  private:
> +    // UpdateBounds() works only for true, and NOOP for false.
> +    bool mEnableUpdateBounds;

I think we should call this mFrameExtends3DContext (and invert the meaning).

Actually, can't we just check mFrame->Extends3DContext instead of copying the result of that into this flag?

@@ +3930,5 @@
> +   */
> +  bool IsRootOf3DContext() {
> +    return mFrame->Extend3DContext() &&
> +      !mFrame->Combines3DTransformWithAncestors() &&
> +      !IsTransformSeparator();

I don't think IsRootOf3DContext is the right name for this. Can you explain a bit more what you're trying to define here?

@@ +3954,5 @@
>    ComputeTransformFunction mTransformGetter;
>    nsRect mChildrenVisibleRect;
>    uint32_t mIndex;
> +  nsRect mBounds;
> +  // True for mBounds is a valid value.

"True if mBounds is valid"

Comment 20

[Thinker Li [:sinker]]

Comment on attachment 8669624 [details] [diff] [review]
Improve GetBounds() to avoid recomputing every time, v6

Review of attachment 8669624 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsDisplayList.h
@@ +3645,5 @@
> +    }
> +
> +  private:
> +    // UpdateBounds() works only for true, and NOOP for false.
> +    bool mEnableUpdateBounds;

Yes! They are logic equivalent. Checking frame in UpdateBounds() is obscure for me.  A flag for this reduce coupling too.

@@ +3930,5 @@
> +   */
> +  bool IsRootOf3DContext() {
> +    return mFrame->Extend3DContext() &&
> +      !mFrame->Combines3DTransformWithAncestors() &&
> +      !IsTransformSeparator();

This function is called only for an assertion in UpdateBoundsPreserves3D() to make sure UpdateBoundsPreserves3D() would be called only on the transform items that it's frame establish a 3D rendering context.  Maybe IsEstablisherOf3DContext()?

Comment 21

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

Comment on attachment 8669624 [details] [diff] [review]
Improve GetBounds() to avoid recomputing every time, v6

Review of attachment 8669624 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsDisplayList.h
@@ +3645,5 @@
> +    }
> +
> +  private:
> +    // UpdateBounds() works only for true, and NOOP for false.
> +    bool mEnableUpdateBounds;

If I look at the UpdateBounds() code, I see that it checks mEnableUpdateBounds, then to figure out what that means, I have to find all callers to SetEnableUpdateBounds, so then I find out that Init() sets it to !mFrame->Extend3DContext(). That's all more work than if we just wrote mFrame->Extend3DContext() directly in UpdateBounds(). So please do that, with a good comment explaining why we're checking that there.

@@ +3930,5 @@
> +   */
> +  bool IsRootOf3DContext() {
> +    return mFrame->Extend3DContext() &&
> +      !mFrame->Combines3DTransformWithAncestors() &&
> +      !IsTransformSeparator();

How about renaming UpdateboundsPreserves3D to UpdateBoundsFor3D, and call it unconditionally, and change the assertion to an "if"? And then inline IsRootOf3DContext so we don't have to name it? And add a comment explaining why you're using this condition?

Comment 22

[Thinker Li [:sinker]]

Attachment: Improve GetBounds() to avoid recomputing every time, v7

--
Attachment #8669624 [details] [diff] - Attachment is obsolete: true

Comment 23

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

Comment on attachment 8670107 [details] [diff] [review]
Improve GetBounds() to avoid recomputing every time, v7

Review of attachment 8670107 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsDisplayList.h
@@ +3627,5 @@
>        nsDisplayWrapList(aBuilder, aFrame, aItem) {}
>      virtual ~StoreList() {}
>  
> +    virtual void UpdateBounds(nsDisplayListBuilder* aBuilder) override {
> +      if (mFrame->Extend3DContext()) {

Add a comment explaining why this check is here.

@@ +3890,5 @@
> +   * 3D rendering context.
> +   *
> +   * \see nsDisplayItem::DoUpdateBoundsPreserves3D()
> +   */
> +  void UpdateBoundsFor3D(nsDisplayListBuilder* aBuilder) {

Why not just do this automatically in the nsDisplayTransform constructor that we call from BuildDisplayListForStackingContext, instead of having a separate method that BuildDisplayListForStackingContext must call?

Comment 24

[Thinker Li [:sinker]]

Attachment: Improve GetBounds() to avoid recomputing every time, v8

--
Attachment #8670107 [details] [diff] - Attachment is obsolete: true

Comment 25

[Thinker Li [:sinker]]

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

Comment 26

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

what are the next steps here?

Comment 27

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

:sinker, are we ready to land this patch?

Comment 28

[Thinker Li [:sinker]]

I would land this patch next week after fixing some cses of reftests.

Comment 29

[Thinker Li [:sinker]]

Attachment: Improve GetBounds() to avoid recomputing every time, v9

--
Attachment #8670719 [details] [diff] - Attachment is obsolete: true

Comment 30

[Thinker Li [:sinker]]

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

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=e0048600aa2c&newProject=try&newRevision=431fad8cd2f9

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/32b5ae598c63

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/32b5ae598c63

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/32b5ae598c63

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Backed out from Aurora44 so that bug 1097464 could be backed out cleanly. It remains fixed on Fx45+ where bug 1097464 is still landed.

https://hg.mozilla.org/releases/mozilla-aurora/rev/586f2eaf3833

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/586f2eaf3833

Comment 36

[Ryan VanderMeulen [:RyanVM]]

This along with the change that caused this regression were backed out from Firefox 45.

https://hg.mozilla.org/releases/mozilla-aurora/rev/64ec448f156d