Closed Bug 1586220 Opened 1 year ago Closed 1 year ago

2.22 - 6.15% Explicit Memory / JS (linux64, macosx1014-64-shippable, windows10-64, windows7-32) regression on push c36eedede55882d7d5d2f811f448f824126612bf (Mon September 30 2019)

Categories

(Core :: DOM: Core & HTML, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox67 --- unaffected
firefox68 --- unaffected
firefox69 --- unaffected
firefox70 --- unaffected
firefox71 --- fixed

People

(Reporter: Bebe, Assigned: Gijs)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: perf, perf-alert, regression)

Attachments

(1 file)

We have detected an awsy regression from push:

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

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

Regressions:

6% JS linux64-stylo-sequential opt 110,447,397.25 -> 117,236,378.46
6% JS linux64-qr opt 110,508,336.38 -> 116,958,732.43
6% JS windows7-32-shippable opt 82,931,910.28 -> 87,726,957.86
6% JS windows10-64-shippable-qr opt 114,048,397.05 -> 120,629,997.00
6% JS windows7-32 opt 82,936,621.66 -> 87,687,871.74
6% JS linux64 opt 110,673,368.86 -> 116,956,189.38
6% JS windows10-64 opt 113,722,366.91 -> 120,180,391.14
6% JS windows10-64-shippable opt 113,681,569.38 -> 119,982,060.22
5% JS linux64 opt stylo tp6 204,993,863.95 -> 216,158,034.35
5% JS linux64-shippable opt 110,858,509.95 -> 116,883,430.25
5% JS linux64-shippable-qr opt 110,815,036.67 -> 116,771,814.47
5% JS windows10-64-qr opt 114,118,076.84 -> 120,229,315.16
5% JS windows7-32 opt stylo tp6 158,759,016.42 -> 167,212,324.18
5% JS macosx1014-64-shippable opt 112,621,371.19 -> 118,504,152.44
5% JS macosx1014-64-shippable opt stylo tp6 204,006,367.40 -> 214,578,913.87
5% JS windows10-64-shippable opt stylo tp6 205,698,942.77 -> 216,314,779.65
5% JS linux64-shippable opt stylo tp6 205,898,054.78 -> 216,288,569.25
5% JS windows7-32-shippable opt stylo tp6 158,549,858.12 -> 165,980,819.25
5% JS linux64-shippable-qr opt stylo tp6 206,893,152.99 -> 216,539,922.72
5% JS windows10-64-qr opt stylo tp6 210,013,857.64 -> 219,459,027.85
4% JS windows7-32-shippable opt stylo tp6 159,154,271.91 -> 166,266,645.98
4% JS linux64-qr opt stylo tp6 207,951,831.91 -> 216,465,852.49
2% Explicit Memory windows7-32-shippable opt stylo tp6 387,178,804.49 -> 396,675,585.66
2% Explicit Memory macosx1014-64-shippable opt stylo tp6 526,904,349.21 -> 538,594,244.43

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

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

Blocks: 1578356
Component: Performance → DOM: Core & HTML
Flags: needinfo?(gijskruitbosch+bugs)
Product: Testing → Core
Regressed by: 1345830
Version: Version 3 → unspecified

Delightful. It turns out the AWSY test relies on simulated shift presses being treated as user interaction - https://searchfox.org/mozilla-central/source/testing/awsy/awsy/awsy_test_case.py#374-394 .

After the patch in bug 1345830 we no longer treat modifier presses (or shortcuts, ie modifier+something else) as user interaction, which means this hack doesn't work, which may explain why the "settled" measures have all regressed, but the "after forced GC" measures have not. The confusing thing is... the comment claims that adding the keypresses avoids additional GCs, whereas it seems here the loss of the effect of "mark this user interactive" triggered no longer doing GCs, which regressed the metrics of the test.

This all seems... terrible. I have three questions:

  1. can we fix the tests to do something - anything - better/more reliable than this
  2. perhaps more importantly, what is/was the exact connection between user interaction and GC here? It doesn't look like the effect the comment describes is actually what's happening. Are we now just never becoming active and does that cause us to never GC until forced to? Or something else? Should we accept the regression because this is really what we want to measure, or something (but then, should we remove the bogus key simulation code)?
  3. should we reconsider the activity requirement so that GC logic is unaffected, either by coupling the GC logic to something different to what we use for beforeunload, or by changing the requirement for chrome documents (assuming activity in the chrome doc influences whether we GC in the content process, which I'm not sure about)?

(ahal, I'm hoping you know about AWSY based on hg log/blame; looks like erahm is out - please redirect if there's someone more appropriate)

Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(bugs)
Flags: needinfo?(ahal)

I don't know what is terrible. We don't want to run slow GCs when using is active.
https://searchfox.org/mozilla-central/rev/2f29d53865cb895bf16c91336cc575aecd996a17/dom/base/nsJSEnvironment.cpp#411-421

It is surprising that we see memory regression, I would have expected better memory use, since we should be able to do shrinking GCs more often.
Is it possible that we don't now ever run shrinking GC since we never consider user being active, so we never change state to inactive.

We could move https://searchfox.org/mozilla-central/rev/2f29d53865cb895bf16c91336cc575aecd996a17/dom/events/EventStateManager.cpp#498-499 outside the new key event specific checks.

Flags: needinfo?(bugs)

I'm not familiar with it, my name is only in the blame for lint fixes. I think :bc has worked with AWSY before.

Flags: needinfo?(ahal) → needinfo?(bob)

(In reply to Olli Pettay [:smaug] from comment #2)

I don't know what is terrible. We don't want to run slow GCs when using is active.
https://searchfox.org/mozilla-central/rev/2f29d53865cb895bf16c91336cc575aecd996a17/dom/base/nsJSEnvironment.cpp#411-421

Yes, this makes perfect sense. The fact that we're faking keypresses in the perf tests in order to get a particular GC behaviour is what I find concerning.

It is surprising that we see memory regression, I would have expected better memory use, since we should be able to do shrinking GCs more often.
Is it possible that we don't now ever run shrinking GC since we never consider user being active, so we never change state to inactive.

I expect this is it, also based on what the comment in the test says. In that case, is that something we should fix more generally? That is, presumably this means scripts or other automated tools that use automation to interact with the browser might also "never" do these GCs... Anyway, perhaps we shouldn't worry about that for the immediate case...

We could move https://searchfox.org/mozilla-central/rev/2f29d53865cb895bf16c91336cc575aecd996a17/dom/events/EventStateManager.cpp#498-499 outside the new key event specific checks.

Yeah, I guess given that otherwise, shortcut presses wouldn't interrupt interruptable GCs (or prevent them from occurring), for UX reasons it's probably best to fix this this way, though I'm still a bit concerned about how fragile the test's way of interacting with GC really is...

Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(bob)
Priority: -- → P2
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/613ae311e665
still treat shortcut/modifier keypresses as interaction for the purposes of GCs/timers and so on, just not for the website, r=smaug
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

== Change summary for alert #23399 (as of Wed, 09 Oct 2019 16:44:05 GMT) ==

Improvements:

7% JS windows10-64 opt stylo tp6 222,414,409.02 -> 207,241,327.43
6% JS linux64-shippable-qr opt 117,569,342.93 -> 110,322,216.86
6% JS windows10-64-shippable-qr opt 120,695,995.16 -> 113,401,420.42
6% JS macosx1014-64-shippable opt 118,517,634.45 -> 111,585,114.39
6% JS windows7-32 opt 87,345,009.72 -> 82,348,348.20
6% JS linux64-qr opt 116,935,806.93 -> 110,430,790.54
6% JS windows7-32-shippable opt 87,548,656.43 -> 82,732,263.86
5% JS linux64 opt 116,773,622.48 -> 110,466,986.82
5% JS windows10-64 opt 119,540,058.55 -> 113,142,354.60
5% JS windows10-64-shippable opt 119,488,012.84 -> 113,110,468.06
5% JS linux64-shippable opt 116,351,043.01 -> 110,269,004.13
5% JS windows7-32-shippable opt stylo tp6 165,967,840.89 -> 157,434,804.09
5% JS linux64-stylo-sequential opt 116,795,165.84 -> 110,805,284.23
5% JS windows10-64-qr opt 119,841,094.91 -> 113,840,323.30
5% JS linux64-qr opt stylo tp6 215,401,066.93 -> 204,918,500.93
5% JS macosx1014-64-shippable opt stylo tp6 213,543,901.43 -> 203,192,579.83
5% JS windows7-32 opt stylo tp6 165,519,342.94 -> 157,633,503.59
5% JS linux64-shippable-qr opt stylo tp6 215,952,096.83 -> 205,731,416.85
5% JS linux64-shippable opt stylo tp6 215,039,249.10 -> 204,941,557.56
4% JS windows10-64-qr opt stylo tp6 219,400,488.50 -> 209,652,335.81
4% JS linux64 opt stylo tp6 215,024,322.68 -> 205,806,003.93
4% JS windows10-64-shippable-qr opt stylo tp6 219,990,845.14 -> 211,595,274.99
3% Explicit Memory windows10-64 opt stylo tp6 488,636,678.46 -> 472,255,369.14
3% Explicit Memory linux64-shippable opt stylo tp6 525,994,131.14 -> 510,068,224.66
3% Explicit Memory windows10-64-shippable-qr opt 378,562,832.61 -> 368,619,735.29
2% Explicit Memory linux64 opt stylo tp6 523,071,485.68 -> 510,542,193.68
2% Explicit Memory macosx1014-64-shippable opt stylo tp6 548,904,758.68 -> 536,572,716.21
0.26% Base Content JS windows7-32 opt 3,228,032.00 -> 3,219,684.00
0.26% Base Content JS windows7-32-shippable opt 3,228,032.00 -> 3,219,684.00

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=23399

No longer blocks: 1592626
You need to log in before you can comment on or make changes to this bug.