763984 - GC: Add new GC zeal modes to exercise incremental collection

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jon Coppeard (:jonco)]

Currently it's difficult to test incremental collection because it doesn't always happen that often.  We can improve this by adding some new GC zeal modes, which would periodically trigger on allocation.  These would:

 1. Do only root marking in the first slice (as if the budget is zero) and then finish the GC in the second slice.

 2. Do all marking in the first slice and then leave before doing any sweeping. In the second slice, do any new marking work that write barriers might have created and then finish the GC.

 3. Run multiple slices, limiting the collection to X objects where X is doubled every slice until all objects have been marked.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Patch to add new GC zeal modes

Adds new zeal mode options 6-8 as described in the bug.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 632285 [details] [diff] [review]
Patch to add new GC zeal modes

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

This looks good to me. Sorry for all the style nits. Could you post a new patch with the changes and ask for review again?

Also, I noticed that the patch has a lot of extra whitespace at the end of lines. We try to avoid that (I think vi people get bugged by it for some reason). I set show-trailing-whitespace to true in emacs so that it shows up in red.

::: js/src/jscntxt.h
@@ +600,5 @@
>      int                 gcZeal_;
>      int                 gcZealFrequency;
>      int                 gcNextScheduled;
>      bool                gcDeterministicOnly;
> +    int                 gcIncrementalLimit;

We usually initialize fields like this in the JSRuntime constructor, even if it's not necessary.

@@ +610,5 @@
>      bool needZealousGC() {
>          if (gcNextScheduled > 0 && --gcNextScheduled == 0) {
> +            if (gcZeal() == js::gc::ZealAllocValue ||
> +                (gcZeal() >= js::gc::ZealIncrementalRootsThenFinish &&
> +                 gcZeal() <= js::gc::ZealIncrementalMultipleSlices))

For multi-line conditions, we use braces. And, unlike the braces for a single-line condition, the opening brace goes on its own line to separate the condition from the code. As in:

if (X &&
    Y)
{
    do_something();
}

::: js/src/jsgc.cpp
@@ +3508,5 @@
>      gc::State initialState = rt->gcIncrementalState;
>  
> +    int zeal = 0;
> +#ifdef JS_GC_ZEAL
> +    if (reason == gcreason::DEBUG_GC)

Is there a reason we should only do this for DEBUG_GC GCs? If so, please add a comment.

@@ +3529,1 @@
>      *shouldSweep = false;

Could you move this assignment to the top of the function so that it happens even if we return early?

@@ +3534,5 @@
>          if (!rt->gcMarker.hasBufferedGrayRoots())
>              sliceBudget.reset();
>  
> +        if (zeal == ZealIncrementalRootsThenFinish ||
> +            zeal == ZealIncrementalMarkAllThenFinish)

Need braces here too.

@@ +3558,5 @@
> +
> +            if (!rt->gcLastMarkSlice &&
> +                ((initialState == MARK && budget != SliceBudget::Unlimited) ||
> +                 zeal == ZealIncrementalMarkAllThenFinish) &&
> +                zeal != ZealIncrementalRootsThenFinish) {

The brace should go on its own line here.

@@ +3563,3 @@
>                  rt->gcLastMarkSlice = true;
> +            }
> +            else {

We use "} else {".

@@ +4058,5 @@
>  void
>  RunDebugGC(JSContext *cx)
>  {
>  #ifdef JS_GC_ZEAL
> +    JSRuntime *rt = cx->runtime;

I think we need the AutoKeepAtoms from RunLastDitchGC here.

@@ +4065,5 @@
> +
> +    int type = rt->gcZeal();
> +    if (type == ZealIncrementalRootsThenFinish ||
> +        type == ZealIncrementalMarkAllThenFinish ||
> +        type == ZealIncrementalMultipleSlices) {

Brace on its own line.

@@ +4076,5 @@
> +            else
> +                rt->gcIncrementalLimit *= 2;
> +            budget = SliceBudget::WorkBudget(rt->gcIncrementalLimit);
> +        }
> +        else {

} else {

@@ +4083,5 @@
> +        }
> +        Collect(rt, true, budget, GC_NORMAL, gcreason::DEBUG_GC);
> +    }
> +    else
> +        RunLastDitchGC(cx, gcreason::DEBUG_GC);

Let's just always use Collect no matter what and stop using RunLastDitchGC. I think that will be cleaner. Then the call can go outside the branch.

Comment 3

[Terrence Cole [:terrence]]

(In reply to Bill McCloskey (:billm) from comment #2)
> Also, I noticed that the patch has a lot of extra whitespace at the end of
> lines. We try to avoid that (I think vi people get bugged by it for some
> reason). I set show-trailing-whitespace to true in emacs so that it shows up
> in red.

I have the following stanza in my .vimrc.

" Highlight whitespace at ends of lines
:highlight ExtraWhitespace ctermbg=red guibg=red
:match ExtraWhitespace /\s\+\%#\@<!$/
:match
:au InsertEnter * match ExtraWhitespace /\s\+\%#\@<!$/
:au InsertLeave * match ExtraWhitespace /\s\+$/

Another nit: if we need to brace any one block of an if/elseif/else, we brace every block.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Updated patch with review comments addressed

Thanks for the review comments.  

The reason for the following:

    int zeal = 0;
#ifdef JS_GC_ZEAL
    if (reason == gcreason::DEBUG_GC)
        zeal = rt->gcZeal();

is because I assumed we only want to do one of our special zeal mode collections if the GC was triggered by the zeal periodic settings in the first place, not for example in response to being actually out of memory (the zeal modes incremental collection types won't actually free any memory in the first slice).  Do you you think that's the right approach?  I added a comment to this affect anyway.

Adding AutoKeepAtoms in RunDebugGC has the effect of disabling incremental GC, because IsIncrementalGCSafe() indicates that incremental GC is not safe if rt->gcKeepAtoms is set, so I left this out.

I think I've addressed all the other comments in the updated patch.

Let me know what you think!

Jon

Comment 5

[Jon Coppeard (:jonco)]

Attachment: Simplify the code slightly

Further to the previous patch, I made a couple of (slightly more invasive) changes that simplified the logic in IncrementalMarkSlice a little.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 632684 [details] [diff] [review]
Updated patch with review comments addressed

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

The thing about using the special zeal stuff only for debug GCs makes sense. And I realize you're right about the AutoKeepAtoms call. I took that call out and ran all our tests, and it didn't seem to make any difference. I think it was probably a historical relic.

All my comments are just syntax nits. Please post an updated patch and we can get it checked in.

::: js/src/jsgc.cpp
@@ +3538,5 @@
> +    int zeal = 0;
> +#ifdef JS_GC_ZEAL
> +    if (reason == gcreason::DEBUG_GC) {
> +        // do the collection type specified by zeal mode only if the collection
> +        // was triggered by RunDebugGC

Comments should be complete sentences, with proper capitalization and a period at the end.

@@ +4108,5 @@
> +            else
> +                rt->gcIncrementalLimit *= 2;
> +            budget = SliceBudget::WorkBudget(rt->gcIncrementalLimit);
> +        }
> +        else {

Should be on one line: "} else {".

@@ +4109,5 @@
> +                rt->gcIncrementalLimit *= 2;
> +            budget = SliceBudget::WorkBudget(rt->gcIncrementalLimit);
> +        }
> +        else {
> +            // this triggers incremental GC but is actually ignored by IncrementalMarkSlice

Capitalization and period.

@@ +4114,5 @@
> +            budget = SliceBudget::WorkBudget(1);
> +        }
> +        Collect(rt, true, budget, GC_NORMAL, gcreason::DEBUG_GC);
> +    }
> +    else {

} else {

@@ +4115,5 @@
> +        }
> +        Collect(rt, true, budget, GC_NORMAL, gcreason::DEBUG_GC);
> +    }
> +    else {
> +        RunLastDitchGC(cx, gcreason::DEBUG_GC);

I'd still rather if we used the same call here regardless of the zeal mode. Can you just change this one to the following?
  Collect(rt, false, SliceBudget::Unlimited, GC_NORMAL, gcreason::DEBUG_GC);

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 632742 [details] [diff] [review]
Simplify the code slightly

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

I like the overall idea here, but it seems to change semantics in undesirable ways. For example, if we're in the middle of an incremental GC and we start a new slice where rt->isTooMuchMalloc() is true, then we want to immediately finish the current incremental GC. We do this right now by setting the budget to unlimited. With the patch, we would set the incremental variable to false. However, since rt->gcIncrementalState is MARK (and not NO_INCREMENTAL), we'll still do a normal incremental slice.

Can you come up with a way to fix this? Maybe it would be best to finally eliminate NonIncrementalMark.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: Updated patch with review comments addressed #2

Thanks for the comments.  I've fixed the syntax issues, and done an update which has merged the changes for Bug 764440 which also affects the zeal options.

Comment 9

[Jon Coppeard (:jonco)]

Attachment: Code simplification with comments addressed

It seems I didn't fully understand what was going on in BudgetIncrementalGC().  I've updated the patch.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 633996 [details] [diff] [review]
Updated patch with review comments addressed #2

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

Looks great, thanks.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 634059 [details] [diff] [review]
Code simplification with comments addressed

This seems safe. I'll push both these patches to tryserver today to make sure everything is okay.

Comment 12

[Jon Coppeard (:jonco)]

(In reply to Bill McCloskey (:billm) from comment #11)
> This seems safe. I'll push both these patches to tryserver today to make
> sure everything is okay.

Awesome, thank you!

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Sorry, I kinda spaced on this. Here's the try link:

https://tbpl.mozilla.org/?tree=Try&rev=f2c8a1fc7373

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I had to change the patch, since gcIncrementalMode != NO_INCREMENTAL no longer implies that we're doing an incremental collection. Here's a new try link:

https://tbpl.mozilla.org/?tree=Try&rev=1dd0a59258b2

Comment 15

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2a937c4fb99
https://hg.mozilla.org/integration/mozilla-inbound/rev/59be51b3268d

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f2a937c4fb99
https://hg.mozilla.org/mozilla-central/rev/59be51b3268d