1548185 - use a smarter heuristic for when to run the next iGC slice

Status: RESOLVED INVALID

(Core :: JavaScript: GC, enhancement, P5)

Description

[Paul Bone [:pbone]]

In Bug 1548182 it seems I can improve performance by scheduing GC slices more often. Which makes sense because we'll collect more garbage (keeping memory usage lower) with shorter GCs and have barriers enabled for a shorter duration.

As far as I'm aware, (NI jonco, is this correct?), we only use how much as been allocated as a signal for when to run the next slice. I don't think we use time. However a GC may run for a long time if the mutator doesn't allocate much memory. It may be useful to at least add time as another cue for when to run another slice. Some other ideas are:

• Use time as above.
• Vary the time/size between slices depending on the duration of the
  previous slice, to try to keep a reasonable ratio between mutator and GC
  time.
• Apparently we check whether we should GC at the main loop, but I wasn't
  able to find that code. One other option is to run this in idle time. We
  currently collect the nursery in idle time, we should check for iGC slices
  before checking if we should collect the nursery and only do one of
  them.

Comment 1

[Paul Bone [:pbone]]

NI jonco for a question in comment 0

Comment 2

[Daniel Holbert [:dholbert] (PTO until 2/24)]

(Triaging this as a qf:p2 responsiveness bug, since GCs show up frequently in profiles, so we're eager to see them be shorter/smarter. Feel free to downgrade to p3 if that seems appropriate.)

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(GCs don't show up in the profiles too often)

Most of the GC slices are scheduled in nsJSEnvironment. By far the most common case is idle time, but
if there isn't any idle time, we fall back to timers (via IdleTaskRunner).

Looks like the recent changes to slice scheduling have even improved idle usage.
https://telemetry.mozilla.org/new-pipeline/evo.html#!aggregates=Median!Mean!5th%2520percentile!25th%2520percentile!75th%2520percentile!95th%2520percentile&cumulative=0&end_date=2019-03-05&include_spill=0&keys=!XHR!URL%2520%253A%253A%2520getter!UserAgent%2520getter&max_channel_version=nightly%252F68&measure=GC_SLICE_DURING_IDLE&min_channel_version=nightly%252F67&processType=content&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2019-01-28&trim=1&use_submission_date=0

(There were 3 different patches, latter two fixing issues caused by the first one)

https://searchfox.org/mozilla-central/rev/7944190ad1668a94223b950a19f1fffe8662d6b8/dom/base/nsJSEnvironment.cpp#1793-1801
https://searchfox.org/mozilla-central/rev/7944190ad1668a94223b950a19f1fffe8662d6b8/dom/base/nsJSEnvironment.cpp#2260-2269

Comment 4

[Paul Bone [:pbone]]

I had a chat with jonco last night and he said that zoneAllocDelayBytes is only used during long-executing JS tasks (or tasks that allocate a lot) and more is done in idle time. So it won't have the kind of impact I was thinking of. And the tests I ran improved performance at a cost to responsiveness. Maybe there's still some opportunity to improve performance by changing how often the idle timers fire.