1557771 - Lots of DOCSHELL GCs triggered for yahoo news raptor test

Status: RESOLVED WORKSFORME

(Core :: JavaScript: GC, defect, P2)

Description

[Jon Coppeard (:jonco)]

I'm seeing a lot of DOCSHELL GCs when I run the yahoo news raptor tests with:

./mach raptor-test --test raptor-tp6-yahoo-news-firefox

I had a look at how this works and it seems that nsJSContext::MaybeRunNextCollectorSlice will start a new GC if the GC timer is pending (sGCTimer) as well as just running a slice.

I think the timer gets started when we close a page, so during benchmarks (and heavy use) this will often be the case. I think this may end up with us doing more GCs than necessary.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: raptor-linux-yahoo.txt

Test log with JS_GC_PROFILE enabled to show GCs.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1557771 - Trigger fewer GCs via nsJSContext::MaybeRunNextCollectorSlice r=smaug?

This patch changes MaybeRunNextCollectorSlice so that it doesn't start a new GC/CC if one is not running already.

This reduces the number of GC slices when running the benchmark from ~400 to ~230.

Olli, what do you think? It seems to me that this is closer to the original intention of the code. I haven't run this through perfherder because I haven't worked out what try command I need to do this, but I figure that it won't make things worse.

Comment 3

[Jon Coppeard (:jonco)]

Speedometer results seem unaffected on windows platforms. I'll try pushing this.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec4aac5a6c9f
Trigger fewer GCs via nsJSContext::MaybeRunNextCollectorSlice r=smaug

Comment 5

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

The priority flag is not set for this bug.
:jonco, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 6

[Daniel Varga [:dvarga]]

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

Comment 7

[Florin Strugariu [:Bebe]]

== Change summary for alert #21587 (as of Tue, 25 Jun 2019 14:26:11 GMT) ==

Improvements:

3% raptor-stylebench-firefox windows10-64-shippable opt 44.40 -> 45.58

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

Comment 8

[Jon Coppeard (:jonco)]

The following performance regressions have been reported in bug 1565486:

2% raptor-sunspider-firefox linux64-shippable-qr opt 213.24 -> 218.23
2% raptor-sunspider-firefox linux64-shippable-qr opt 213.50 -> 217.40
3% raptor-stylebench-firefox windows10-64-shippable opt 44.40 -> 45.58
7% raptor-assorted-dom-firefox linux64-shippable-qr opt 26.71 -> 28.69
6% raptor-assorted-dom-firefox linux64-shippable opt 26.93 -> 28.47
5% raptor-assorted-dom-firefox linux64-shippable opt 27.13 -> 28.45

Based on that I'm going to back this out.

Comment 9

[Jon Coppeard (:jonco)]

Backout:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1fbc711925ba8c05f021f40aad329483413d38f6

Comment 10

[Jon Coppeard (:jonco)]

Attachment: backout1557771

I guess we probably want to back this out of beta too.

Comment 11

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

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

Please nominate for beta.

Comment 12

[Jon Coppeard (:jonco)]

Comment on attachment 9077689 [details] [diff] [review]
backout1557771

Beta/Release Uplift Approval Request

• User impact if declined: Minor performance regressions.
• Is this code covered by automated tests?: Yes
• 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): This is a backout of a simple change and just returns the code to its original state.
• String changes made/needed: None

Comment 13

[Jon Coppeard (:jonco)]

Comment on attachment 9077689 [details] [diff] [review]
backout1557771

Beta/Release Uplift Approval Request

• User impact if declined: Minor performance regressions.
• Is this code covered by automated tests?: Yes
• 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): This is a backout of a simple change and just returns the code to its original state.
• String changes made/needed: None

Beta/Release Uplift Approval Request

• User impact if declined: Minor performance regressions.
• Is this code covered by automated tests?: Yes
• 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): This is a backout of a simple change and just returns the code to its original state.
• String changes made/needed: None

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9077689 [details] [diff] [review]
backout1557771

Approved for 69.0b6 to fix the regression before it ships.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

NI Aryx to make sure this shows up on the radar for the next round of uplifts.

Comment 16

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

Not sure this is relevant, but the backout caused those changes:
== Change summary for alert #21888 (as of Mon, 15 Jul 2019 03:35:35 GMT) ==

Regressions:

3% raptor-stylebench-firefox windows10-64-shippable opt 46.04 -> 44.63

Improvements:

1% raptor-sunspider-firefox linux64-shippable opt 210.23 -> 207.64

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

Comment 17

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

https://hg.mozilla.org/releases/mozilla-beta/rev/4829e3f23d9d

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9077689 [details] [diff] [review]
backout1557771

Clearing the approval flag to get this off the needs-uplift radar now that the backout has landed.