Closed
Bug 1453028
Opened 6 years ago
Closed 6 years ago
Add new zeal modes to test specific parts of incremental sweeping
Categories
(Core :: JavaScript: GC, enhancement)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: jonco, Assigned: jonco)
References
Details
Attachments
(4 files)
7.34 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
14.73 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
5.71 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
11.44 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•6 years ago
|
||
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)
Assignee | ||
Comment 2•6 years ago
|
||
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)
Assignee | ||
Comment 3•6 years ago
|
||
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)
Assignee | ||
Comment 4•6 years ago
|
||
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)
Assignee | ||
Comment 5•6 years ago
|
||
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 6•6 years ago
|
||
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 7•6 years ago
|
||
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 8•6 years ago
|
||
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 9•6 years ago
|
||
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+
Assignee | ||
Comment 10•6 years ago
|
||
(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.
Comment 11•6 years ago
|
||
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
Comment 12•6 years ago
|
||
(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.
Comment 13•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0f26c7b0aa7d https://hg.mozilla.org/mozilla-central/rev/12337456bdd9 https://hg.mozilla.org/mozilla-central/rev/de5cfd97ac49 https://hg.mozilla.org/mozilla-central/rev/c0f28a370935
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•