1562825 - 3.15 - 3.96% Base Content Heap Unclassified (windows10-64-shippable, windows10-64-shippable-qr) regression on push 133ac0d11428079ceaeceab4dda050b4ba240e3b (Sat June 29 2019)

Status: RESOLVED FIXED

(Core :: WebVR, defect, P2)

Description

[Alexandru Ionescu (needinfo me) [:alexandrui]]

We have detected an awsy regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=133ac0d11428079ceaeceab4dda050b4ba240e3b

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

Regressions:

4% Base Content Heap Unclassified windows10-64-shippable-qr opt 1,552,937.00 -> 1,614,421.33
4% Base Content Heap Unclassified windows10-64-shippable-qr opt 1,553,547.33 -> 1,614,604.00
3% Base Content Heap Unclassified windows10-64-shippable opt 1,574,870.33 -> 1,624,525.33

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

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 jobs in a pushlog format.

To learn more about the regressing test(s), please see: https://wiki.mozilla.org/AWSY/Tests

Comment 1

[:kip (Kearwood Gilbert)]

I am investigating this now.

Comment 2

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Hi Kearwood, any news on this?

Comment 3

[:kip (Kearwood Gilbert)]

(In reply to Alexandru Ionescu :alexandrui from comment #2)

Hi Kearwood, any news on this?

Refactoring which was included in that push merged several classes, which may have caused an increased allocation on initialization that would not have normally occurred until a WebVR/WebXR site becomes active. I haven't found a single source of allocation to the degree in the report, but have identified some ways that we can defer allocations until a WebVR/WebXR site becomes active.

I'll follow up with some patches shortly.

Comment 4

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Hi Kearwood. I assume you got somehow to a conclusion until now. Could you share it? Thanks.

Comment 5

[:kip (Kearwood Gilbert)]

(In reply to Alexandru Ionescu :alexandrui from comment #4)

Hi Kearwood. I assume you got somehow to a conclusion until now. Could you share it? Thanks.

Hello,

I apologize as my time has been spread thin recently. Investigation of this is still ongoing. Although it does not initially appear to account for the amount of memory in the report, it seems that the clearest way forward is to make some iterative changes that lazily initialize some of the VRManager related structures.

I will be starting PTO in a couple of weeks. If this is not resolved by then, I'll be sure to hand off my findings to others to continue until I am back.

Comment 6

[Pascal Chevrel:pascalc]

Hi Kearwood, could you give us an update please? thanks

Comment 7

[:kip (Kearwood Gilbert)]

I have returned from PTO. I'll bump this up my queue and/or see if my colleagues have made any discoveries.

Comment 8

[Pascal Chevrel:pascalc]

Hi kip, any news on this p2? Thanks

Comment 9

[Daosheng Mu[:daoshengmu]]

According to https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=133ac0d11428079ceaeceab4dda050b4ba240e3b changset, that is related to our initial work for using command buffer for VR service tests. I guess we can take a look into the timing of this command buffer creation.

Comment 10

[Daosheng Mu[:daoshengmu]]

I am going to work on this bug this week.

Comment 11

[Daosheng Mu[:daoshengmu]]

Try looks good now, https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=bce310a40b8bc3dbb6e4a19352ae6e70b89056e4&newProject=try&newRevision=62a8bb155ce5ac893b774c82cfe2e56d7108c867&framework=4

Comment 12

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1562825 - Checking if puppet is active before initializing VR puppet command buffers.

Comment 13

[Daosheng Mu[:daoshengmu]]

The root cause is VRServiceHost::Shutdown() [1] will call PuppetRest(), then that will allocate the heap memory.

https://searchfox.org/mozilla-central/rev/2a10f4812f3f7c7645d253a4fe52f26bf58e20e8/gfx/vr/VRServiceHost.cpp#80

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b7469047904
Checking if puppet is active before initializing VR puppet command buffers. r=kip

Comment 15

[Cosmin Sabou [:CosminS]]

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

Comment 16

[Pascal Chevrel:pascalc]

Hi Daosheng, do you think this fix is a good candidate for an uplift to beta? If it is, please request the uplift, thanks.

Comment 17

[Daosheng Mu[:daoshengmu]]

Comment on attachment 9107085 [details]
Bug 1562825 - Checking if puppet is active before initializing VR puppet command buffers.

Beta/Release Uplift Approval Request

• User impact if declined: My patch fixes the issue of redundant heap allocation, if denying it, the heap will continue to be allocated.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It is happened at VRServiceTest that usually be run when doing smog tests. This heap allocation is happened when FF is doing shutdown. Therefore, for general users, they can get back this 3% heap allocation after applying this patch also with very low risk.
• String changes made/needed:

Comment 18

[Pascal Chevrel:pascalc]

Comment on attachment 9107085 [details]
Bug 1562825 - Checking if puppet is active before initializing VR puppet command buffers.

Fix for an awsy memory regression, patch is evaluated as low risk, uplift approved for 71 beta 10, thanks.

Comment 19

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a207a5eba543

Comment 20

[:kip (Kearwood Gilbert)]

Clearing NI