Closed Bug 1557771 Opened 6 months ago Closed 6 months ago

Lots of DOCSHELL GCs triggered for yahoo news raptor test

Categories

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

defect

Tracking

()

RESOLVED WORKSFORME
mozilla69

People

(Reporter: jonco, Assigned: jonco)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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.

Attached file raptor-linux-yahoo.txt

Test log with JS_GC_PROFILE enabled to show GCs.

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.

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

Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec4aac5a6c9f
Trigger fewer GCs via nsJSContext::MaybeRunNextCollectorSlice r=smaug

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(jcoppeard)
Blocks: GCScheduling
Flags: needinfo?(jcoppeard)
Priority: -- → P2
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Assignee: nobody → jcoppeard

== 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

Regressions: 1565486

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.

Attached patch backout1557771Splinter Review

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

Resolution: FIXED → WORKSFORME
Attachment #9073259 - Attachment is obsolete: true

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
Flags: needinfo?(jcoppeard)
Attachment #9077689 - Flags: approval-mozilla-beta?

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
Flags: needinfo?(jcoppeard)
Attachment #9077689 - Flags: approval-mozilla-beta?
Comment on attachment 9077689 [details] [diff] [review]
backout1557771

Approved for 69.0b6 to fix the regression before it ships.
Attachment #9077689 - Flags: approval-mozilla-beta?
Attachment #9077689 - Flags: approval-mozilla-beta+

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

Flags: needinfo?(aryx.bugmail)

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 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.
Attachment #9077689 - Flags: approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.