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

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jmaher, Assigned: sinker)

Tracking

({perf, regression})

Trunk
mozilla44
perf, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 unaffected, firefox43 unaffected, firefox44 unaffected, firefox45 unaffected, firefox46 fixed, b2g-v2.5 unaffected, b2g-master fixed)

Details

(Whiteboard: [talos_regression])

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

4 years ago
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
(Reporter)

Comment 1

4 years ago
: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.
Flags: needinfo?(tlee)
(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?
Blocks: 1097464
Flags: needinfo?(jmaher)
(Assignee)

Comment 3

4 years ago
ok!
Flags: needinfo?(tlee)
(Reporter)

Comment 4

4 years ago
: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
No longer blocks: 1026520
Flags: needinfo?(jmaher)
(Assignee)

Comment 5

4 years ago
Created attachment 8666485 [details] [diff] [review]
Improve GetBounds() to avoid recomputing every time
(Assignee)

Comment 6

4 years ago
(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.
(Assignee)

Updated

4 years ago
Assignee: nobody → tlee
(Assignee)

Updated

4 years ago
Attachment #8666485 - Flags: review?(roc)
(Assignee)

Comment 8

3 years ago
Created attachment 8666596 [details] [diff] [review]
Improve GetBounds() to avoid recomputing every time, v2

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

Updated

3 years ago
Attachment #8666485 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Reporter)

Comment 9

3 years ago
:sinker, thanks for fixing this!
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
Flags: needinfo?(tlee)
(Assignee)

Comment 11

3 years ago
ok!
Flags: needinfo?(tlee)
Keywords: checkin-needed
(Assignee)

Comment 12

3 years ago
Created attachment 8668802 [details] [diff] [review]
Improve GetBounds() to avoid recomputing every time, v3

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

Updated

3 years ago
Attachment #8666596 - Attachment is obsolete: true
(Assignee)

Comment 13

3 years ago
Created attachment 8668883 [details] [diff] [review]
Improve GetBounds() to avoid recomputing every time, v4

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

Updated

3 years ago
Attachment #8668802 - Attachment is obsolete: true
(Assignee)

Comment 14

3 years ago
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.
Attachment #8668883 - Flags: review?(roc)
(Assignee)

Updated

3 years ago
Attachment #8668883 - Flags: review?(roc)
(Assignee)

Comment 16

3 years ago
Created attachment 8669235 [details] [diff] [review]
Improve GetBounds() to avoid recomputing every time, v5

--
Attachment #8668883 [details] [diff] - Attachment is obsolete: true
Attachment #8669235 - Flags: review?(roc)
(Assignee)

Updated

3 years ago
Attachment #8668883 - Attachment is obsolete: true
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"?
Attachment #8669235 - Flags: review?(roc)
(Assignee)

Comment 18

3 years ago
Created attachment 8669624 [details] [diff] [review]
Improve GetBounds() to avoid recomputing every time, v6

--
Attachment #8669235 [details] [diff] - Attachment is obsolete: true
Attachment #8669624 - Flags: review?(roc)
(Assignee)

Updated

3 years ago
Attachment #8669235 - Attachment is obsolete: true
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"
Attachment #8669624 - Flags: review?(roc)
(Assignee)

Comment 20

3 years ago
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 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?
(Assignee)

Comment 22

3 years ago
Created attachment 8670107 [details] [diff] [review]
Improve GetBounds() to avoid recomputing every time, v7

--
Attachment #8669624 [details] [diff] - Attachment is obsolete: true
Attachment #8670107 - Flags: review?(roc)
(Assignee)

Updated

3 years ago
Attachment #8669624 - Attachment is obsolete: true
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?
Attachment #8670107 - Flags: review?(roc)
(Assignee)

Comment 24

3 years ago
Created attachment 8670719 [details] [diff] [review]
Improve GetBounds() to avoid recomputing every time, v8

--
Attachment #8670107 [details] [diff] - Attachment is obsolete: true
Attachment #8670719 - Flags: review?(roc)
(Assignee)

Updated

3 years ago
Attachment #8670107 - Attachment is obsolete: true
(Reporter)

Comment 26

3 years ago
what are the next steps here?
(Reporter)

Comment 27

3 years ago
:sinker, are we ready to land this patch?
Flags: needinfo?(tlee)
(Assignee)

Comment 28

3 years ago
I would land this patch next week after fixing some cses of reftests.
Flags: needinfo?(tlee)
(Assignee)

Comment 29

3 years ago
Created attachment 8675404 [details] [diff] [review]
Improve GetBounds() to avoid recomputing every time, v9

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

Updated

3 years ago
Attachment #8670719 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/32b5ae598c63
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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
status-firefox43: --- → unaffected
status-firefox44: fixed → unaffected
status-firefox45: --- → fixed
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/586f2eaf3833
status-b2g-v2.5: --- → unaffected
status-b2g-master: --- → fixed
status-firefox42: --- → unaffected
(Assignee)

Updated

3 years ago
Blocks: 1240783
This along with the change that caused this regression were backed out from Firefox 45.

https://hg.mozilla.org/releases/mozilla-aurora/rev/64ec448f156d
status-firefox45: fixed → unaffected
status-firefox46: --- → fixed
You need to log in before you can comment on or make changes to this bug.