[Perf][google suite][google docs] Try to prevent the GC before first paint event

RESOLVED FIXED in Firefox 55

Status

()

Core
JavaScript: GC
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: cynthiatang, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(platform-rel -, firefox55 fixed)

Details

(Whiteboard: [qf:p3][platform-rel-Google][platform-rel-GoogleSuite][platform-rel-GoogleDocs])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
In profiler data, we could see the heavy GC task before the first paint event. We are trying to improve our webpage startup time. Maybe we could have some heuristic rules for GC module that do not do the GC before the first paint.

Profiler: https://cleopatra.io/#report=37b1b314384bfff047553f1995a62b608457f830&filter=%5B%7B%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A60317,%22end%22%3A60881%7D%5D&selection=0,3902
(Reporter)

Comment 1

2 years ago
Steps to Reproduce:
 1. Launch browser
 2. Open a google doc with 200+ pages 
    https://docs.google.com/document/d/1EPSmGqm2r4Qq42B4t1VOYacjTlL0JVuC8JSlUvoIhss/edit?usp=sharing
Summary: Try to prevent the GC before first paint event → [Perf][google docs] Try to prevent the GC before first paint event
(Reporter)

Updated

2 years ago
Blocks: 1264536

Updated

2 years ago
Component: JavaScript: GC → General
I think the way this works now is that when we set a new document or something we call nsJSContext::PokeGC(), which triggers a GC 4 seconds later. The idea is that this should happen after a page loads, but it looks like this page load takes around 9 seconds, so we end up GCing while the page is still loading.
If you set javascript.options.mem.log to true in about:config, and then look at the browser console, it will display the reason for the GC. It will look something like "GC Slice 0 - Pause: 10.631ms of 10ms budget (@ 0.000ms); Reason: CC_WAITING". It would be good to know what the reason is for this GC.
(Reporter)

Updated

2 years ago
Component: General → JavaScript: GC
We looked at this in Taipei last week, the GCReason is SET_NEW_DOCUMENT.  Why do we GC there?  That seems like it's always going to be in the page load path ...
Flags: needinfo?(continuation)
IIRC we do that because of some really old historical reasons. So better to look for CVS blame.
Is that still possible?  Bonsai is gone, IIRC.
FWIW, I can see the relevant GC call in http://52.25.115.98/viewvc/main/mozilla/dom/src/base/nsGlobalWindow.cpp?revision=1.1&view=markup#l277
Thu Jul 16 01:16:25 1998 UTC
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #4)
> We looked at this in Taipei last week, the GCReason is SET_NEW_DOCUMENT. 
> Why do we GC there?

It doesn't GC immediately, it GCs in 4 seconds, so usually it will fire after the document is loaded, I'd think. We also poke the GC in nsDocumentViewer::LoadComplete() (which is a LOAD_END GC), so maybe that's enough and we can just drop SET_NEW_DOCUMENT?
Flags: needinfo?(continuation)
Created attachment 8750050 [details] [diff] [review]
skip the GC in nsGlobalWindow::SetNewDocument().

As comment 8, this patch try to skip the gc during the page loading. It's worth to test with this change.
The GC module observes the memory-pressure event, so I think we sill don't hit the OOM problem.
Created attachment 8750051 [details] [diff] [review]
show GC reason in gecko profiler

from Kyle's hint, show the gc reason to profiler.
platform-rel: --- → ?
Whiteboard: [platform-rel-Google][platform-rel-GoogleSuite][platform-rel-GoogleDocs]

Updated

a year ago
Summary: [Perf][google docs] Try to prevent the GC before first paint event → [Perf][google suite][google docs] Try to prevent the GC before first paint event
Blocks: 1260981
platform-rel: ? → -
Should we try to land these patches Jon?
Flags: needinfo?(jcoppeard)
Whiteboard: [platform-rel-Google][platform-rel-GoogleSuite][platform-rel-GoogleDocs] → [qf:p3][platform-rel-Google][platform-rel-GoogleSuite][platform-rel-GoogleDocs]
I did a try run and it looks ok:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be0157e180a966e09101873389d205b0c7b44123&selectedJob=81631861
Flags: needinfo?(jcoppeard)
Created attachment 8843572 [details] [diff] [review]
bug1270056-remove-poke-gc

smaug, do you think its OK to remove this?
Attachment #8750050 - Attachment is obsolete: true
Attachment #8750051 - Attachment is obsolete: true
Attachment #8843572 - Flags: review?(bugs)
Comment on attachment 8843572 [details] [diff] [review]
bug1270056-remove-poke-gc

ah, this might be fine, given that there is http://searchfox.org/mozilla-central/rev/8dcf6f318c3b0d8298d88b43bd6815405227dc93/layout/base/nsDocumentViewer.cpp#1088

So, I'm ok to try this. Please push to try with all the platforms and debug and opt builds.
Attachment #8843572 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] (high review load) from comment #14)

Try build here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1c16273355c538d686801f0496ae78be2340271&selectedJob=81886118

This mostly green bar a few failures that don't seem related to GC.

Comment 16

9 months ago
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f175c0117072
skip the GC in nsGlobalWindow::SetNewDocument() r=smaug

Comment 17

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f175c0117072
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.