Ting-Yu Chou [:ting] (away) Reporter
	Description • 9 years ago

From a profile [1] I can see BrowserElementParent spends more than 90ms [930,1030] at _isAlive() [2], but this does not always happen. Wonder is it possible to do the same checking but not have a chance to trigger GC.

[1] http://people.mozilla.org/~bgirard/cleopatra/#report=e892e126caeed0df511f8678e780f922c0619f9d
[2] https://dxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.js#238

	Ting-Yu Chou [:ting] (away) Reporter
	Comment 1 • 9 years ago

Quote from bug 773980:

<quote>
The only tricky point is, how do we avoid having each BrowserElementParent register an inner-window-destroyed notification?  As dbaron points out in bug 770993 comment 18, this could lead to a lot more notifications than we want.
</quote>

	Ting-Yu Chou [:ting] (away) Reporter
	Comment 2 • 9 years ago

_isAlive() checks the state of defaultView which is the outer window, in that case is the quotation (comment 1) still valid?

	Ting-Yu Chou [:ting] (away) Reporter
	Comment 3 • 9 years ago

The same GC is also observed from the profile at bug 1110590 comment 13, but in nsFrameMessageManager::ReceiveMessage() for handling SystemMessageCache:RefreshCache.

	Ting-Yu Chou [:ting] (away) Reporter
	Comment 4 • 9 years ago

:bholley, is it possible to suppress the GC triggered by ~AutoEntryScript()? And if it is possible, will there be any drawbacks? Thanks.

	Bobby Holley (:bholley)
	Comment 5 • 9 years ago

(In reply to Ting-Yu Chou [:ting] from comment #0)
> From a profile [1] I can see BrowserElementParent spends more than 90ms
> [930,1030] at _isAlive() [2], but this does not always happen. Wonder is it
> possible to do the same checking but not have a chance to trigger GC.

In general, you can't run script without the possibility of triggering a GC.

(In reply to Ting-Yu Chou [:ting] from comment #4)
> :bholley, is it possible to suppress the GC triggered by ~AutoEntryScript()?
> And if it is possible, will there be any drawbacks? Thanks.

We could add some sort of opt-out for the JS_MaybeGC call in ~AutoEntryScript, but I'm skeptical that that's the right thing. Script can always GC, and any work that we save by not GCing in ~AutoEntryScript will just be work we need to do later. Can you explain why you're trying to optimize that call specifically?

	Andrew McCreight [:mccr8]
	Comment 6 • 9 years ago

The comment in ~AutoEntryScript() makes it sound like poking the GC is just a hack, so it should be reasonable to not do it on an opt-in basis. But as Bobby said, you may still end up with GCs somewhere else during application startup.

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

What are the heuristics when MaybeGC actually triggers GC?

During an app startup we might want to have some flag set that JS Eng should just increase the heap size without any extra GCing, and then after startup run iGC.

	Ting-Yu Chou [:ting] (away) Reporter
	Comment 8 • 9 years ago

(In reply to Bobby Holley (:bholley) from comment #5)
> just be work we need to do later. Can you explain why you're trying to
> optimize that call specifically?

Because the GC happens on b2g process before newing application iframe, which delays application startup. GC while running JS is nature, but ~AutoEntryScript() triggers GC after script finishes running.

I am thinking to suppress the GC in a period, not in each AutoEntryScript instance because it's difficult to decide whether to suppress in some places, for instance the one [1] at bug description.

[1] https://dxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCWrappedJSClass.cpp#518

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

I agree that the heuristics for GCing should be different during app startup. 
That might in theory lead to killing background apps more likely, but we don't really have any
data about that.

So, I think when we're about to start a new app, JS engine should enter to some "less-likely-to-gc" mode which lasts only for couple of seconds or so.

	Jon Coppeard (:jonco) (PTO until 14th September)
	Comment 10 • 9 years ago

One approach is to change the GC tuning parameters during app startup to allow for more heap growth before triggering a GC.

GC trigger heuristics are sadly very complicated, but there's a great comment explaining how things work here:

https://dxr.mozilla.org/mozilla-central/source/js/src/gc/GCRuntime.h?from=gcruntime.h#195

Changing the parameters will likely require an element of trial-and-error to see what improves performance without causing problems elsewhere.

The parameters can be set by calling JS_SetGCParameter().  The parameter to change is given by a JSGCParamKey:

https://dxr.mozilla.org/mozilla-central/source/js/src/jsapi.h?from=jsapi.h#1756

The idea would be to increase the heap size threshold at which GC is triggered during the startup phase.  A couple of ideas:

1. If the zone is new and has not been GCed before the trigger threshold will be 45MiB (this is calculated from low frequency heap growth (default 1.5) times allocation threshold (default 30 MiB)).  This initial GC could be delayed by increasing the allocation threshold (JSGC_ALLOCATION_THRESHOLD).

2. If GCs are occurring more less than once a second then increasing the low frequency growth factor (JSGC_LOW_FREQUENCY_HEAP_GROWTH) from the default of 1.5 could help.

3. If GCs are occurring more than once a second then tuning the parameters related to high frequency heap grown (JSGC_HIGH_FREQUENCY_*) could help.  In general JSGC_HIGH_FREQUENCY_HEAP_GROWTH_MIN (default 1.5) and JSGC_HIGH_FREQUENCY_HEAP_GROWTH_MAX (default 3) could be increased.

Please ping me on IRC if you want more information.

	Ting-Yu Chou [:ting] (away) Reporter
	Comment 11 • 9 years ago

GCRuntime::maybeGC() compares gcBytes() with allocTrigger() [1], but |gcTriggerBytes_| and |gcHeapGrowthFactor_| are updated after GC, so there can still have a GC because the threshold is not updated after I change GC parameters.

How sholud I fix this?

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/jsgc.cpp?from=maybeGC&case=true#3146

	Ting-Yu Chou [:ting] (away) Reporter
	Comment 12 • 9 years ago

I prefer to avoid only the GC triggered by ~AutoEntryScript(), as it seems a hack and is not always needed. General GC should still happen IMO.

	Terrence Cole [:terrence]
	Comment 13 • 9 years ago

(In reply to Ting-Yu Chou [:ting] from comment #12)
> I prefer to avoid only the GC triggered by ~AutoEntryScript(), as it seems a
> hack and is not always needed. General GC should still happen IMO.

Actually, ~AutoEntryScript is the perfect time to GC because we know a script just ended and therefore that many objects are likely dead. A typical GC in ~AutoEntryScript should be faster and more effective than a typical GC elsewhere.

(In reply to Ting-Yu Chou [:ting] from comment #11)
> GCRuntime::maybeGC() compares gcBytes() with allocTrigger() [1], but
> |gcTriggerBytes_| and |gcHeapGrowthFactor_| are updated after GC, so there
> can still have a GC because the threshold is not updated after I change GC
> parameters.

This appears to be an existing bug. We should be propagating gc tunables to the zones automatically when updating the GC parameters. The attached patch should do it.

	Terrence Cole [:terrence]
	Comment 14 • 9 years ago

Try run, since this is poking GC scheduling:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=acefe5d0bcce

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

Comment on attachment 8631121 [details] [diff] [review]
update_zone_triggers_when_updating_gcparams-v0.diff

Review of attachment 8631121 [details] [diff] [review]:
-----------------------------------------------------------------

We'll end up doing the update every time if several parameters are set, but this is probably fine.  And much better than not doing it at all as previously.

	Ting-Yu Chou [:ting] (away) Reporter
	Comment 16 • 9 years ago

(In reply to Terrence Cole [:terrence] from comment #13)
> Actually, ~AutoEntryScript is the perfect time to GC because we know a
> script just ended and therefore that many objects are likely dead. A typical
> GC in ~AutoEntryScript should be faster and more effective than a typical GC
> elsewhere.

Thank you for explaining me and the provide the patch.

My plan is to change the GC parameters:

  "javascript.options.mem.gc_high_frequency_heap_growth_min", and
  "javascript.options.mem.gc_low_frequency_heap_growth"

to as high as "javascript.options.mem.gc_high_frequency_heap_growth_max" (300 in b2g.js) in Webapps.jsm when receive "Webapps:Launch" [1], and set it back after 1 second delay. How does this sound?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.jsm#1401

	Ting-Yu Chou [:ting] (away) Reporter
	Comment 17 • 9 years ago

(In reply to Ting-Yu Chou [:ting] from comment #16)
> My plan is to change the GC parameters:
> to as high as "javascript.options.mem.gc_high_frequency_heap_growth_max"
> (300 in b2g.js) in Webapps.jsm when receive "Webapps:Launch" [1], and set it

This seems not early enough, there're some (>10) other ~AutoEntryScript() invocations after homescreen calling launch() and before b2g process receiving Webapps:Launch.

	Ting-Yu Chou [:ting] (away) Reporter
	Comment 18 • 9 years ago

(In reply to Ting-Yu Chou [:ting] from comment #17)
> This seems not early enough, there're some (>10) other ~AutoEntryScript()
> invocations after homescreen calling launch() and before b2g process
> receiving Webapps:Launch.

The AutoEntryScript are mostly with reason "EventListener.handleEvent", the others are:

  "XPCWrappedJS QueryInterface", and
  "message manager handler"

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 19 • 9 years ago

I would expect that you'd want to tweak GC setting when we're about the launch a new app, and then
reset the settings to normal once the app has been launched (or launching has failed for whatever reason).

	Ting-Yu Chou [:ting] (away) Reporter
	Comment 20 • 9 years ago

(In reply to Olli Pettay [:smaug] from comment #19)
> I would expect that you'd want to tweak GC setting when we're about the
> launch a new app, and then
> reset the settings to normal once the app has been launched (or launching
> has failed for whatever reason).

The handler of |Webapps:Lauch| in Webapps.jsm is the earliest place I know in b2g process to understand we're about launching an app, however there's only a |content-start| event from b2g shell.js for success launching, I don't see an event for failed cases. And I think we only need to suppress GC for a short period to make sure it is not happened before creating application iframe, that's why I would like to use a timer.

How do you think?

	Ting-Yu Chou [:ting] (away) Reporter
	Comment 21 • 9 years ago

This wip is based on comment 16.

	Pulsebot
	Comment 22 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d11ac89bcd3

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/3d11ac89bcd3

	Ting-Yu Chou [:ting] (away) Reporter
	Comment 24 • 9 years ago

The bug hasn't been resolved yet, the patch just landed is to update zone triggers once gc parameter is changed.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 25 • 9 years ago

I still think comment 19 is the way to go. I guess we need to be able to mark a process
to be "in-startup" and then nsJSEnvironment heuristics and GC heuristics would use that to try to
prevent extra GCs.

	Terrence Cole [:terrence]
	Comment 26 • 8 years ago

I'm not working on this. I guess this bug isn't terribly relevant anymore in any case.