1193428 - Rename the |incremental| parameter to be more meaningful

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: 3_rename_incremental_param-v0.diff

The |incremental| parameter confused me greatly (and still does to some extent). We use it to make some incremental decisions, but certainly not all of them. And the decisions we make do not seem to have much in common.

What it seems to really mean, at least in gcCycle is "this SliceBudget::unlimited GC event was triggered by an API that expects !incremental behavior." Specifically, this is GCRuntime::gc -- that's the only caller that sets |incremental| to false.

In collect it controls whether we will redo the non-incremental GC for dead zones. I think this second usage would be better done by checking if the slice budget is unlimited so that it happens not just via gc(). In this case we set the incremental parameter to false to prevent us from doing this multiple times.

I think both of these uses would be much better done in GCRuntime::gc, allowing us to remove the parameter altogether, but I'm not sure how feasible this is. It is going to take some extensive experimentation. The first step though is to just make the name more coherent. I think incrementalByAPI is better than incremental.

Comment 1

[Jon Coppeard (:jonco)]

Comment on attachment 8646477 [details] [diff] [review]
3_rename_incremental_param-v0.diff

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

(In reply to Terrence Cole [:terrence] from comment #0)
This is fine, but I think we really want to invert the sense of it and call it nonIncrementalRequested (or nonIncrementalByAPI).

As you say, the parameter is necessary for gcCycle() to distinguish between being asked to continue an incremental GC with unlimited budget and start a new GC with unlimited budget.  So it's not sufficient to make a GC incremental by itself, whereas it is sufficient to make a GC non-incremental.

The dead zones check only needs to happen for incremental GCs so I think this should check isIncremental instead and set the slice budget to unlimited.

And yes, we should move the reset and the dead zone check up to collect().

Comment 2

[Terrence Cole [:terrence]]

(In reply to Jon Coppeard (:jonco) from comment #1)
> Comment on attachment 8646477 [details] [diff] [review]
> 3_rename_incremental_param-v0.diff
> 
> Review of attachment 8646477 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Terrence Cole [:terrence] from comment #0)
> This is fine, but I think we really want to invert the sense of it and call
> it nonIncrementalRequested (or nonIncrementalByAPI).
> 
> As you say, the parameter is necessary for gcCycle() to distinguish between
> being asked to continue an incremental GC with unlimited budget and start a
> new GC with unlimited budget.  So it's not sufficient to make a GC
> incremental by itself, whereas it is sufficient to make a GC non-incremental.

That's a great way to think about it!

> The dead zones check only needs to happen for incremental GCs so I think
> this should check isIncremental instead and set the slice budget to
> unlimited.
> 
> And yes, we should move the reset and the dead zone check up to collect().

My initial attempt to move the reset up led to JP failures, so I split both of these out to a separate bug.

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f772e7820f5f

Comment 4

[Carsten Book [:Tomcat]]

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