Closed Bug 1453028 Opened 2 years ago Closed 2 years ago

Add new zeal modes to test specific parts of incremental sweeping

Categories

(Core :: JavaScript: GC, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(4 files)

There are several sub-phases to incremental sweeping, and we should add zeal modes to test them specifically.  This should help the fuzzers discover any latent bugs.
I was originally going to add an extra parameter to the zeal mode string so I rewrote the parsing to make it easier for me to understand.  I didn't do that in the end, but I think this is more readable.
Attachment #8966962 - Flags: review?(sphink)
This renames the zeal modes that do incremental GC in two slices to make it more obvious which part of the GC they are designed to test.

For example, IncrementalRootsThenFinish is renamed to YieldBeforeMarking to indicate that it cans show up problems with incremental marking.

I refactored the code in GCRuntime::runDebugGC() to split the IncrementalMultipleSlices case out from the code that handles these modes.
Attachment #8966963 - Flags: review?(sphink)
This adds a sweep action that can yield before performing another action if a specified zeal mode is enabled, and uses it to implement the YieldBeforeSweepingAtoms mode.
Attachment #8966964 - Flags: review?(sphink)
Finally, this adds a bunch of new zeal modes to target different parts of incremental sweeping.

This clutters up GCRuntime::initSweepActions() a bit, but was the best way I could find to do it so far.
Attachment #8966968 - Flags: review?(sphink)
I didn't find any problems running the JS engine tests with these new modes.  I haven't tried testing the browser with them yet.
Comment on attachment 8966962 [details] [diff] [review]
bug1453028-1-refactor-parsing

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

Very nice! I'd never read the previous parsing, and I'm glad I didn't.

::: js/src/gc/GC.cpp
@@ +1118,5 @@
> +            memcmp(text.begin().get(), mode.name, mode.length) == 0)
> +        {
> +            *modeOut = mode.value;
> +            return true;
> +        }

Doesn't this disallow numeric zeal modes? Is that what you wanted?

@@ +1125,5 @@
> +    return false;
> +}
> +
> +static bool
> +ParseZealModeParam(CharRange text, uint32_t* paramOut)

Rename to ParseZealModeNumericParam.

@@ +1136,4 @@
>              return false;
> +    }
> +
> +    *paramOut = atoi(text.begin().get());

char* end;
  long val = strtol(text.begin().get(), &end, 10);
  if (end != text.end())
    return false;
  *paramOut = val;
  return true;

would probably be more correct (though I've never used CharRange, so I'm not sure that's right.) But I guess it doesn't matter much.

@@ +1167,5 @@
> +GCRuntime::parseAndSetZeal(const char* str)
> +{
> +    auto text = CharRange(str, strlen(str));
> +
> +    CharRangeVector parts;

Maybe comment somewhere

  // The format is one or more levels, separated by ';', followed by an optional ",N" giving the frequency.

I didn't know you could do multiple levels. Our format is weird.
Attachment #8966962 - Flags: review?(sphink) → review+
Comment on attachment 8966963 [details] [diff] [review]
bug1453028-2-rename-zeal-modes

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

You're right, that works out a lot better.
Attachment #8966963 - Flags: review?(sphink) → review+
Comment on attachment 8966964 [details] [diff] [review]
bug1453028-3-add-yield-action

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

Nice!
Attachment #8966964 - Flags: review?(sphink) → review+
Comment on attachment 8966968 [details] [diff] [review]
bug1453028-4-add-new-modes

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

Heh. I see why you were thinking of changing the parsing. But this seems more usable for now.
Attachment #8966968 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] [:s:] (PTO Apr9-12) from comment #6)
> Doesn't this disallow numeric zeal modes? Is that what you wanted?

This calls ParseZealModeNumericParam for the mode if ParseZealModeName fails, so both are allowed.

> char* end;
>   long val = strtol(text.begin().get(), &end, 10);
>   if (end != text.end())
>     return false;
>   *paramOut = val;
>   return true;
> 
> would probably be more correct (though I've never used CharRange, so I'm not
> sure that's right.) But I guess it doesn't matter much.

I looked up what these functions do and both accept leading whitespace which I didn't want to allow, so I'm going to leave it like it is with the manual digit check first.
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f26c7b0aa7d
Refactor the way we parse zeal mode strings r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/12337456bdd9
Rename GC zeal modes that run in two slices and refactor r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/de5cfd97ac49
Add a new sweep action to yield in a specified zeal mode r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0f28a370935
Add new zeal modes to test the different parts of incremental sweeping r=sfink
(In reply to Jon Coppeard (:jonco) from comment #10)
> (In reply to Steve Fink [:sfink] [:s:] (PTO Apr9-12) from comment #6)
> > Doesn't this disallow numeric zeal modes? Is that what you wanted?
> 
> This calls ParseZealModeNumericParam for the mode if ParseZealModeName
> fails, so both are allowed.

Ok, I missed that.

> > char* end;
> >   long val = strtol(text.begin().get(), &end, 10);
> >   if (end != text.end())
> >     return false;
> >   *paramOut = val;
> >   return true;
> > 
> > would probably be more correct (though I've never used CharRange, so I'm not
> > sure that's right.) But I guess it doesn't matter much.
> 
> I looked up what these functions do and both accept leading whitespace which
> I didn't want to allow, so I'm going to leave it like it is with the manual
> digit check first.

atoi does the same thing. atoi(s) is equivalent to strtol(s, NULL, 10).

But you're right; I hadn't noticed that you check whether *all* of the digits are numeric first. In that case, atoi seems fine. (I just have a rule burned in my head that you should always use strtol in favor of atoi, because of error checking, but you're doing the error checking separately so it seems fine.)

Of course, none of these do a range check. -z 9999999999 will give you an assert in debug builds, I don't know what in opt builds. But I can't see that mattering all that much.

Ugh -- I just tried things out and sadly if you have a zeal syntax error you'll get the help message followed by the infamous "WARNING: YOU ARE LEAKING THE WORLD" message. Oh well.
You need to log in before you can comment on or make changes to this bug.