1570883 - 2.13 - 2.15% Base Content Heap Unclassified (windows10-64-shippable-qr) regression on push 0eff8ea5dbf102b07f111fabac7ffe63d6eb9502 (Mon July 29 2019)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Marian Raiciof [:marauder]]

We have detected an awsy regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=0eff8ea5dbf102b07f111fabac7ffe63d6eb9502

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

Regressions:

2% Base Content Heap Unclassified windows10-64-shippable-qr opt 1,576,820.67 -> 1,610,772.00
2% Base Content Heap Unclassified windows10-64-shippable-qr opt 1,576,743.33 -> 1,610,335.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=22243

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

[Jan de Mooij [:jandem]]

Maybe we can double check the "periodically free JSContext's temp LifoAlloc" code is still working as expected.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The component has been changed since the backlog priority was decided, so we're resetting it.
For more information, please visit auto_nag documentation.

Comment 3

[Kris Wright :KrisWright]

(In reply to Jan de Mooij [:jandem] from comment #1)

Maybe we can double check the "periodically free JSContext's temp LifoAlloc" code is still working as expected.

I checked to see if we are entering the routine to free the tempLifoAlloc. I'm not sure if any other implementation details of the LifoAlloc change by using a global list, but I do know that we are periodically calling tempLifoAlloc().freeAll() so assuming that routine still works, it should be freeing unused memory. Since the call only happens after a context is used, it's possible that there is a cx at the end of the list which is used less, and therefore may only reach freeAll() after GlobalHelperThreadState sets freeUnusedMemory several times or not at all. Could it be that we need to try calling it more frequently, or maybe make it GlobalHelperThreadState's responsibility to call freeAll() on contexts that are not currently in use after setting freeUnusedMemory?

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Kris Wright :KrisWright from comment #3)

or maybe make it GlobalHelperThreadState's responsibility to call freeAll() on contexts that are not currently in use after setting freeUnusedMemory?

Good point. Ideally we'd free off-thread so how about this: if we try to set the freeUnusedMemory flag, but we notice the flag is already set (meaning the context hasn't been active since last time we set it) then we do it on the main thread.

Comment 5

[Kris Wright :KrisWright]

Attachment: Bug 1570883 - GlobalHelperThreadState should clear the LifoAlloc on JSContexts that haven't been used since the last free event.

In the event of a JSContext having gone unused between the last triggerFreeUnusedMemory() and the current one, GlobalHelperThreadState should free the temp LifoAlloc from the main thread. This isn't safe for ContextData and requires tempLifoAlloc to take a different type.

Comment 6

[Kris Wright :KrisWright]

(In reply to Jan de Mooij [:jandem] from comment #4)

Good point. Ideally we'd free off-thread so how about this: if we try to set the freeUnusedMemory flag, but we notice the flag is already set (meaning the context hasn't been active since last time we set it) then we do it on the main thread.

I realize now that the temp LifoAlloc was ContextData, so freeing it from the main thread causes the protected data checks to fail unless the context itself is set to the thread. I'm not so sure of the idea of making it a raw pointer or a piece of unprotected data, but that is what I did for now. Is there a better way to address it?

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Kris Wright :KrisWright from comment #6)

I'm not so sure of the idea of making it a raw pointer or a piece of unprotected data, but that is what I did for now. Is there a better way to address it?

(Commented in Phabricator, you can use refNoCheck.)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d463eca94a85
GlobalHelperThreadState should clear the LifoAlloc on JSContexts that haven't been used since the last free event. r=jandem

Comment 9

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/d463eca94a85