Vaibhav (:vaibhav1994) Reporter
	Description • 9 years ago

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

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 1 • 9 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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 2 • 9 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.

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 3 • 9 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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 4 • 9 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.

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 5 • 9 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.

	Kelsey Gilbert [:jgilbert]
	Comment 6 • 9 years ago

If the fix to [A] fixes [B], we should backout our hacky fix to [B].

[A]: bug 1136352
[B]: bug 1137251

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 7 • 9 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?

	Sotaro Ikeda [:sotaro] Assignee
	Comment 8 • 9 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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 9 • 9 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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 10 • 9 years ago

Added fix to handle ImageBridge does not existed case.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 11 • 9 years ago

I confirmed the patch works on Windows 8.1 with ImageBridge disabled. It did not cause memory leak.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 12 • 9 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 13 • 9 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 14 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e27d15a42ff

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/1e27d15a42ff

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 16 • 9 years ago

verified this is fixed by looking at the graphs!

	Sotaro Ikeda [:sotaro] Assignee
	Comment 17 • 9 years ago

(In reply to Joel Maher (:jmaher) from comment #16)
> verified this is fixed by looking at the graphs!

Thanks.