Closed
Bug 1139935
Opened 10 years ago
Closed 10 years ago
2.37% WinXP glterrain regression on Mozilla-Inbound-Non-PGO (v.39) on March 04, 2015 from push 9daf2b5ad7f8
Categories
(Testing :: Talos, defect)
Tracking
(firefox39 fixed)
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: vaibhav1994, Assigned: sotaro)
References
Details
(Keywords: perf, regression, Whiteboard: [talos_regression])
Attachments
(1 file)
4.55 KB,
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
Talos has detected a Firefox performance regression from the commit 9daf2b5ad7f8. 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=9daf2b5ad7f8&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.
To learn more about the regressing test, please see: https://wiki.mozilla.org/Buildbot/Talos/Tests#glterrain
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 win32 -u none -t g1 # 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 glterrain
Making a decision:
As the patch author we need your feedback to help us handle this regression.
Our wiki page oulines the common responses and expectations:
https://wiki.mozilla.org/Buildbot/Talos/RegressionBugsHandling
Comment 1•10 years ago
|
||
Sotaro- can you take a look at this and let us know how to proceed here based on:
https://wiki.mozilla.org/Buildbot/Talos/RegressionBugsHandling
Flags: needinfo?(sotaro.ikeda.g)
Assignee | ||
Comment 2•10 years ago
|
||
If it is actually cause the regression, it could be backed out if Bug 1136352 fix exists in gecko. Touch, it is not clear why Bug 1137251 caused the regression.
Flags: needinfo?(sotaro.ikeda.g)
Comment 3•10 years ago
|
||
bug 1136352 is in gecko, is this patch not necessary? backing out is a last resort, usually there is a good reason to land a patch. What benefits do we get from the patch? This is a small regression that appears limited to WinXP, so lets weigh our options before backing out.
Flags: needinfo?(sotaro.ikeda.g)
Assignee | ||
Comment 4•10 years ago
|
||
Bug 1137251 is landed because the bug needs to be fixed as soon as possible on release build and Bug 1137251 is simplest fix. Another reason is AsyncTransactionTracker is actually necessitated only on gonk.
Flags: needinfo?(sotaro.ikeda.g)
Comment 5•10 years ago
|
||
It might be a good idea to accept this and close this regression bug as wontfix. Ultimately that is not my decision at this time.
Comment 6•10 years ago
|
||
If the fix to [A] fixes [B], we should backout our hacky fix to [B].
[A]: bug 1136352
[B]: bug 1137251
Comment 7•10 years ago
|
||
Sotaro, what :jgilbert said indicates we should backout this as it is fixed by bug 113652. Can you confirm and either backout or let us know and I can backout?
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #7)
> Sotaro, what :jgilbert said indicates we should backout this as it is fixed
> by bug 113652. Can you confirm and either backout or let us know and I can
> backout?
I am going to create a patch to backout. because bug 1136352 needs ImageBridge existence.
Assignee | ||
Comment 9•10 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> (In reply to Joel Maher (:jmaher) from comment #7)
> > Sotaro, what :jgilbert said indicates we should backout this as it is fixed
> > by bug 113652. Can you confirm and either backout or let us know and I can
> > backout?
>
> I am going to create a patch to backout. because bug 1136352 needs
> ImageBridge existence.
Sorry, ImageBridge is necessary only when RemoveTextureFromCompositableAsync() is called out side of Layer transaction. It is used only on b2g. This check is already added by bug 1136352.
Flags: needinfo?(sotaro.ikeda.g)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → sotaro.ikeda.g
Assignee | ||
Comment 10•10 years ago
|
||
Added fix to handle ImageBridge does not existed case.
Assignee | ||
Comment 11•10 years ago
|
||
I confirmed the patch works on Windows 8.1 with ImageBridge disabled. It did not cause memory leak.
Assignee | ||
Updated•10 years ago
|
Attachment #8575506 -
Flags: review?(nical.bugzilla)
Assignee | ||
Comment 12•10 years ago
|
||
Assignee | ||
Comment 13•10 years ago
|
||
Updated•10 years ago
|
Attachment #8575506 -
Flags: review?(nical.bugzilla) → review+
Assignee | ||
Comment 14•10 years ago
|
||
Comment 15•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment 16•10 years ago
|
||
verified this is fixed by looking at the graphs!
Assignee | ||
Comment 17•10 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #16)
> verified this is fixed by looking at the graphs!
Thanks.
You need to log in
before you can comment on or make changes to this bug.
Description
•