1271854 - Allow setting multiple gc zeal values in GCRuntime::parseAndSetZeal

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[(no longer active)]

Right now we only support one gc zeal value there, but there's no good reason for that.  Can we extend that to support a syntax like |NN[,MM](;LL[,OO])*|?

Comment 1

[Terrence Cole [:terrence]]

(In reply to :Ehsan Akhgari (busy, don't ask for review please) from comment #0)
> Right now we only support one gc zeal value there, but there's no good
> reason for that.  Can we extend that to support a syntax like
> |NN[,MM](;LL[,OO])*|?

I had a patch for this like 4 years ago; we decided it wasn't worth the extra parsing code at the time. I think it's definitely worth resurrecting now that it's seeing much wider usage.

The requested syntax looks perfectly reasonable. Additionally, I'd like to give each of the numbers a proper name so that it's not just a pile of ints and can be read and remembered by a mere human.

Comment 2

[(no longer active)]

I'll take a stab at this.

Comment 3

[(no longer active)]

FWIW as I was implementing these changes I realized that my proposed format string is stupid since there is only one frequency number.  I implemented the simpler |level(;level)*[,freq]| syntax.

Comment 4

[(no longer active)]

Attachment: Part 1: Allow specifying multiple GC zeal levels

This affects both the environment variable JS_GC_ZEAL, and the --gc-zeal
argument to the JS shell.

Comment 5

[(no longer active)]

Attachment: Part 2: Allow specifying zeal modes by name as well

Comment 6

[(no longer active)]

Attachment: Part 3: Avoid saving the GC zeal string inside the JS shell

This will ensure that the GC zeal argument won't get truncated needlessly.

Comment 7

[Terrence Cole [:terrence]]

Comment on attachment 8751886 [details] [diff] [review]
Part 1: Allow specifying multiple GC zeal levels

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

::: js/src/jsgc.cpp
@@ +1267,5 @@
>  
> +            size_t offset = strspn(str, "0123456789");
> +            const char* p = str + offset;
> +            if (!*p || *p == ';')
> +                frequency = JS_DEFAULT_ZEAL_FREQ;

Add braces here: braces are required around all blocks in an if/else chain if any block in the chain requires braces.

@@ +1285,5 @@
> +            return false;
> +        }
> +    } while (!foundFrequency &&
> +             (str = strchr(str, ';')) != nullptr &&
> +             str++);

Parsing in C is the scariest thing ever. If this were enabled in non-DEBUG builds I would demand *so many* tests. As is, it's probably not worth the time.

@@ +1289,5 @@
> +             str++);
> +
> +    for (auto z : zeals) {
> +        setZeal(z, frequency);
> +    }

No braces on this for loop.

Comment 8

[Terrence Cole [:terrence]]

Comment on attachment 8751887 [details] [diff] [review]
Part 2: Allow specifying zeal modes by name as well

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

Awesome!

::: js/src/jsgc.cpp
@@ +277,5 @@
> +#define CHECK_ZEAL(name, value)                                                \
> +    static_assert(ZealMode::Limit >= ZealMode::name,                           \
> +                  "ZealMode::Limit shouldn't be smaller than " #name);
> +JS_FOR_EACH_ZEAL_MODE(CHECK_ZEAL)
> +#undef CHECK_ZEAL

Nice!

::: js/src/jsgc.h
@@ +1218,5 @@
> +            _(IncrementalMultipleSlices, 10)   \
> +            _(IncrementalMarkingValidator, 11) \
> +            _(ElementsBarrier, 12)             \
> +            _(CheckHashTablesOnMinorGC, 13)    \
> +            _(Compact, 14)

It is traditional in SpiderMonkey to use the name |D| for the callable here instead of |_|. I've been using |_| for parameters that I'm not expanding. This use should be hygenic, but let's stick with D for now regardless.

Comment 9

[Terrence Cole [:terrence]]

Comment on attachment 8751888 [details] [diff] [review]
Part 3: Avoid saving the GC zeal string inside the JS shell

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

::: js/src/shell/js.cpp
@@ +7057,5 @@
>      rt->profilingScripts = enableCodeCoverage || enableDisassemblyDumps;
>  
>  #ifdef JS_GC_ZEAL
> +    if (gZealBits && gZealFrequency) {
> +#define ZEAL_MODE(name, value)                     \

Use |_| instead of |name|, since you're not using it.

Comment 10

[(no longer active)]

(In reply to Terrence Cole [:terrence] from comment #7)
> @@ +1285,5 @@
> > +            return false;
> > +        }
> > +    } while (!foundFrequency &&
> > +             (str = strchr(str, ';')) != nullptr &&
> > +             str++);
> 
> Parsing in C is the scariest thing ever. If this were enabled in non-DEBUG
> builds I would demand *so many* tests. As is, it's probably not worth the
> time.

My hands were shaking so much that I could barely type when I was writing this. :-)

FWIW I did test everything that I could think of manually, but wasn't sure how I'd add tests for them.  I'd still be happy to maybe write a C++ tests verifying this if you'd like me to!

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e04dd37f4799
https://hg.mozilla.org/integration/mozilla-inbound/rev/13670b4bb7af
https://hg.mozilla.org/integration/mozilla-inbound/rev/327e1753d1f5

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e04dd37f4799
https://hg.mozilla.org/mozilla-central/rev/13670b4bb7af
https://hg.mozilla.org/mozilla-central/rev/327e1753d1f5