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

Status: RESOLVED FIXED

(Core :: WebVR, defect)

Description

[Ionuț Goldan [:igoldan]]

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

Comment 1

[Ionuț Goldan [:igoldan]]

:kip Can you please look over this regression?

Comment 2

[:kip (Kearwood Gilbert)]

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.

Comment 3

[:kip (Kearwood Gilbert)]

Attachment: Bug 1405950 - Corrected bug in SurfaceFactory::NewTextClient preventing matching textures from being located in mRecycleFreePool

- As the aLayersChannel parameter is never passed and now no longer used,
  it has been removed from the SurfaceFactory::NewTextClient.

Review commit: https://reviewboard.mozilla.org/r/186960/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186960/

Comment 4

[Kelsey Gilbert [: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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b70ead3e555
Corrected bug in SurfaceFactory::NewTextClient preventing matching textures from being located in mRecycleFreePool r=jgilbert

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Please ignore the backout in https://hg.mozilla.org/integration/autoland/rev/7c205fdf625d653af75c352cea38c86924e482c7
It relanded as https://hg.mozilla.org/integration/autoland/rev/35ae93cde64d.

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/35ae93cde64d

Comment 9

[Ionuț Goldan [:igoldan]]

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