Closed Bug 763984 Opened 11 years ago Closed 11 years ago

GC: Add new GC zeal modes to exercise incremental collection

Categories

(Core :: JavaScript Engine, enhancement)

Product:
Core
Component:
JavaScript Engine
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla16

People

(Reporter: jonco, Assigned: jonco)

Details

(Whiteboard: [js:t])

Attachments

(2 files, 3 obsolete files)

Updated patch with review comments addressed #2
11.44 KB, patch
billm
: review+
Details | Diff | Splinter Review
Code simplification with comments addressed
4.60 KB, patch
billm
: review+
Details | Diff | Splinter Review 
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.
Assignee: general → jcoppeard
Attached patch Patch to add new GC zeal modes (obsolete) — — Splinter Review 
Adds new zeal mode options 6-8 as described in the bug.
Attachment #632285 - Flags: review?(wmccloskey)
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.
Attachment #632285 - Flags: review?(wmccloskey)
(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.
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
Attachment #632285 - Attachment is obsolete: true
Attachment #632684 - Flags: review?(wmccloskey)
Attached patch Simplify the code slightly (obsolete) — — Splinter Review 
Further to the previous patch, I made a couple of (slightly more invasive) changes that simplified the logic in IncrementalMarkSlice a little.
Attachment #632742 - Flags: review?(wmccloskey)
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);
Attachment #632684 - Flags: review?(wmccloskey) → review+
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.
Attachment #632742 - Flags: review?(wmccloskey)
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.
Attachment #632684 - Attachment is obsolete: true
Attachment #633996 - Flags: review?(wmccloskey)
It seems I didn't fully understand what was going on in BudgetIncrementalGC().  I've updated the patch.
Attachment #632742 - Attachment is obsolete: true
Attachment #634059 - Flags: review?(wmccloskey)
Comment on attachment 633996 [details] [diff] [review]
Updated patch with review comments addressed #2

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

Looks great, thanks.
Attachment #633996 - Flags: review?(wmccloskey) → review+
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.
Attachment #634059 - Flags: review?(wmccloskey) → review+
(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!
Whiteboard: [js:t]
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
https://hg.mozilla.org/mozilla-central/rev/f2a937c4fb99
https://hg.mozilla.org/mozilla-central/rev/59be51b3268d
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.