Cynthia Tang [:cynthiatang] Reporter
	Description • 8 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

	Cynthia Tang [:cynthiatang] Reporter
	Comment 1 • 8 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

	Andrew McCreight [:mccr8]
	Comment 2 • 8 years ago

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.

	Andrew McCreight [:mccr8]
	Comment 3 • 8 years ago

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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 4 • 8 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 5 • 8 years ago

IIRC we do that because of some really old historical reasons. So better to look for CVS blame.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 6 • 8 years ago

Is that still possible?  Bonsai is gone, IIRC.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 7 • 8 years ago

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

	Andrew McCreight [:mccr8]
	Comment 8 • 8 years ago

(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?

	Jerry Shih[:jerry] (UTC+8) (inactive)
	Comment 9 • 8 years ago

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.

	Jerry Shih[:jerry] (UTC+8) (inactive)
	Comment 10 • 8 years ago

from Kyle's hint, show the gc reason to profiler.

	Naveed Ihsanullah [:naveed]
	Comment 11 • 7 years ago

Should we try to land these patches Jon?

	Jon Coppeard (:jonco) (PTO until 14th September)
	Comment 12 • 7 years ago

I did a try run and it looks ok:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be0157e180a966e09101873389d205b0c7b44123&selectedJob=81631861

	Jon Coppeard (:jonco) (PTO until 14th September)
	Comment 13 • 7 years ago

smaug, do you think its OK to remove this?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 14 • 7 years ago

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.

	Jon Coppeard (:jonco) (PTO until 14th September)
	Comment 15 • 7 years ago

(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.

	Pulsebot
	Comment 16 • 7 years ago

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

	Carsten Book [:Tomcat]
	Comment 17 • 7 years ago
bugherder

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