1257810 - 2-3% linux64 damp/sessionrestore regression on inbound (v.48) from push b20234ac6cf4 on March 8, 2016

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

Talos has detected a Firefox performance regression from push. As author of one of the patches included in that push, we need your help to address this regression.

This is a list of all known regressions and improvements related to the push:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=39f6551752dc&newProject=mozilla-inbound&newRevision=b20234ac6cf4&framework=1&filter=linux

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#sessionrestore.2Fsessionrestore_no_auto_restore

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 linux64 -u none -t other,other-e10s --rebuild 5  # add "mozharness: --spsProfile" to generate profile data

(we suggest --rebuild 5 to be more confident in the results)

To run the test locally and do a more in-depth investigation, first set up a local Talos environment:
https://wiki.mozilla.lorg/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 sessionrestore

(add --e10s to run tests in e10s mode)

Making a decision:
As the patch author we need your feedback to help us handle this regression.
*** Please let us know your plans by Monday, 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

[Joel Maher ( :jmaher ) (UTC -8)]

Hi Terrence, this is bug #2- sorry for the flood!  Can you take a look at this bug, it seems to affect linux64 only- all other signals seem to be noise.

Comment 2

[Terrence Cole [:terrence]]

"Talos has detected a Firefox performance regression from push."

What is the commit id I should be looking at?

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

oh, silly me!

it is this push:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b20234ac6cf4

Comment 4

[Terrence Cole [:terrence]]

Ah, I see that it is in the bug title and I simply can't read.

Comment 5

[Terrence Cole [:terrence]]

Ah, this is bug 1224038. Last time I landed, it regressed all the things, so it looks like the fix I applied mostly worked. There's one other thing we can try here to maybe eek out a bit more performance.

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

cool, glad that you fixed most of the regressions and have an idea to look at the others.  Thanks again for looking into this!

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

:terrence, did you have any luck trying something here?  Are we out of options?

Comment 8

[Terrence Cole [:terrence]]

Attachment: relax_global_atomic-v0.diff

The counter is a single word, so ReleaseAcquire should be good enough. Until we fixed the bugs in beginOffThreadParseTask, we were still seeing crashes, even with SequentiallyConsistent ordering, so we can be basically certain that those crashes were not caused by too-loose ordering guarantees.

Comment 9

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8739149 [details] [diff] [review]
relax_global_atomic-v0.diff

Review of attachment 8739149 [details] [diff] [review]:
-----------------------------------------------------------------

Seems safe to me. There's only one code path that looks at this value, and it's good as long as all threads see _nextCellUniqueId_ updates (which ReleaseAcquire will ensure.)

But was this expected to fix a perf regression? I thought that on x86 architectures, there was no difference in behavior between ReleaseAcquire and SequentiallyConsistent?

Comment 10

[Terrence Cole [:terrence]]

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

Comment 11

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/252686d8c6e5fae00cc0ee68470df6f8387919d5
Bug 1257810 - ReleaseAcquire should be adequate for a counter; r=sfink

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/252686d8c6e5