Closed Bug 1405950 Opened 2 years ago Closed 2 years ago

3.19 - 79.65% glterrain (linux64, osx-10-10, windows10-64, windows7-32) regression on push 83203c2de90ab514f56ab533e840990eca866bc8 (Wed Oct 4 2017)

Categories

(Core :: WebVR, defect)

Unspecified
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- unaffected
firefox58 --- fixed

People

(Reporter: igoldan, Assigned: kip)

References

Details

(Keywords: perf, regression, talos-regression)

Attachments

(1 file)

Talos has detected a Firefox performance regression from push:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=83203c2de90ab514f56ab533e840990eca866bc8

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

 80%  glterrain summary osx-10-10 opt e10s     3.34 -> 6.00
 33%  glterrain summary windows7-32 pgo e10s   3.45 -> 4.58
 31%  glterrain summary windows7-32 opt e10s   3.40 -> 4.45
 22%  glterrain summary windows10-64 opt e10s  2.86 -> 3.49
  3%  glterrain summary linux64 pgo e10s       10.32 -> 10.65


You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=9819

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
Component: Untriaged → WebVR
Product: Firefox → Core
:kip Can you please look over this regression?
Flags: needinfo?(kgilbert)
I believe I have found the cause of the regression.

The "Bug 1400407 - Part 1" patch changed the "if" conditions in SurfaceFactory::NewTexClient:

         if (cur->Surf()->mSize == size){
-            // In the general case, textureClients transit textures through
-            // CompositorForwarder. But, the textureClient created by VRManagerChild
-            // has a different LayerIPCChannel, PVRManager. Therefore, textureClients
-            // need to be separated into different cases.
-            if ((aLayersChannel && aLayersChannel == cur->GetAllocator()) ||
-                (cur->GetAllocator() != gfx::VRManagerChild::Get())) {
+            if (aLayersChannel && aLayersChannel == cur->GetAllocator()) {
                 cur->Surf()->WaitForBufferOwnership();
                 return cur.forget();
             }
         }

Earlier updates ensured that this expression would always be true:

  cur->GetAllocator() != gfx::VRManagerChild::Get()

When the cleanup in Bug 1400407 refactored it out of the "if" condition, the logic was reversed.

The inner "if" statement should have been removed entirely.

The regression is likely due to the reduced recycling of textures.

I'm preparing a patch now.
Assignee: nobody → kgilbert
Flags: needinfo?(kgilbert)
Attachment #8915753 - Flags: review?(jgilbert)
Comment on attachment 8915753 [details]
Bug 1405950 - Corrected bug in SurfaceFactory::NewTextClient preventing matching textures from being located in mRecycleFreePool

https://reviewboard.mozilla.org/r/186960/#review192062
Attachment #8915753 - Flags: review?(jgilbert) → review+
Pushed by kgilbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0b70ead3e555
Corrected bug in SurfaceFactory::NewTextClient preventing matching textures from being located in mRecycleFreePool r=jgilbert
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/35ae93cde64d
Corrected bug in SurfaceFactory::NewTextClient preventing matching textures from being located in mRecycleFreePool r=jgilbert on a CLOSED TREE
See Also: → 1406018
https://hg.mozilla.org/mozilla-central/rev/35ae93cde64d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
I'm confirming the performance fix:

== Change summary for alert #9868 (as of October 06 2017 00:44 UTC) ==

Improvements:

 44%  glterrain summary osx-10-10 opt e10s     5.88 -> 3.31
 24%  glterrain summary windows7-32 pgo e10s   4.56 -> 3.44
 21%  glterrain summary windows7-32 opt e10s   4.50 -> 3.56
 15%  glterrain summary windows10-64 opt e10s  3.44 -> 2.91
 14%  glterrain summary windows10-64 pgo e10s  3.44 -> 2.94
  3%  glterrain summary linux64 opt e10s       10.88 -> 10.57

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9868
See Also: → 1414099
You need to log in before you can comment on or make changes to this bug.