764440 - GC: Add option to enable use of exact stack rooting.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: v0

Now that we have handles in most of our engine, this is trivial to turn on.  This still fails when enabled because of known rooting violations, but is going to be needed soon to allow us to optimize the performance of the Handles.

Comment 1

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

Comment on attachment 632727 [details] [diff] [review]
v0

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

::: js/src/configure.in
@@ +3882,5 @@
>  
>  dnl ========================================================
> +dnl = Use exact stack rooting for GC
> +dnl ========================================================
> +JSGC_USE_EXACT_ROOTING=1

What is this line for?

@@ +3888,5 @@
> +[  --enable-exact-rooting Use exact stack roots for GC],
> +    JSGC_USE_EXACT_ROOTING=1,
> +    JSGC_USE_EXACT_ROOTING= )
> +if test -n "$JSGC_USE_EXACT_ROOTING"; then
> +    AC_DEFINE(JSGC_ROOT_ANALYSIS)

I'd rather keep these options separate so that you can run with exact rooting but without rooting analysis. Otherwise it will be hard to do performance testing.

Comment 2

[Terrence Cole [:terrence]]

Attachment: v1

This is unfortunately quite confusing, but actually covers all of our use cases.  Here are the possible states allowed after this patch and where they should be used:

State: ROOT_ANALYSIS && !ZEAL && !USE_EXACT_ROOTS
Does:  Always runs rooting analysis, but uses normal GC.
Used:  To run the rooting analysis on TBPL.

State: ROOT_ANALYSIS && ZEAL
Does:  Only runs rooting analysis at zeal 6 and 7.
Used:  To optimize handles before exact rooting is ready.

State: USE_EXACT_ROOTS (implies ROOT_ANALYSIS) && !ZEAL
Does:  Always runs rooting analysis and then uses roots for GC.
Used:  Probably useless.

State: USE_EXACT_ROOTS (implies ROOT_ANALYSIS) && ZEAL
Does:  Runs rooting analysis only with zeal and uses roots for GC.
Used:  To optimize rooting after exact rooting is stable.

Comment 3

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

Comment on attachment 632891 [details] [diff] [review]
v1

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

::: js/src/jsapi.cpp
@@ +6646,5 @@
>                     "  3: GC when the window paints (browser only)\n"
>                     "  4: Verify write barriers between instructions\n"
> +                   "  5: Verify write barriers between paints\n"
> +                   "  6: Verify rooting analysis (ignoring XML and Reflect)\n"
> +                   "  7: Verify rooting analysis (all roots)\n");

Instead of "Verify rooting analysis", please say "Verify stack rooting". Also, I think you need to update the help in TestingFunctions.cpp.

::: js/src/jsgc.cpp
@@ +4191,5 @@
>  
> +#ifdef JS_GC_ZEAL
> +    if (rt->gcZeal_ != 6 && rt->gcZeal_ != 7)
> +        return;
> +    if (rt->gcZeal_ == 6 && !rt->gcExactScanningEnabled)

Instead of 6 and 7, please add names for these in jsgc.h (similar to ZealFrameVerifierValue).

Comment 4

[Terrence Cole [:terrence]]

Okay, so I didn't realize that JS_GC_ZEAL was forcibly set when we turn on DEBUG.  Worse, it was forced on in jsapi.h, not in configure.in, so it got seen in jsgc.cpp, but not in gc/Root.h.  This resulted in some very frustrated debugging.

The upshot is that the dichotomy I laid out above will not work.  Instead, I had to go and implement this the right way:

--enable-root-analysis
Enables tracking the stack roots in the context, but does not use them for rooting.  This will be useful for optimization before we finish getting the rooting stable.

--enable-exact-rooting
Enables tracking the stack roots in the context and uses them for rooting, for once we have the rooting stable.

--enable-root-analysis && --enable-debug && --enable-gczeal && --disable-threadsafe
Allows one to use the root analysis, but does not switch it on by default.  Use the new JS_GC_ZEAL modes to switch it on.  This tracks skipRoots in addition to the stack roots, so it will be a bit slower that a build with just root-analysis, but allows you to use the verifier without rebuilding.  This will be useful for generational gc, which needs the roots, wants to test quickly, and would like to occasionally inspect crashes or run the full rooting analysis.

Running on try at: https://tbpl.mozilla.org/?tree=Try&rev=7ba47e705b0c

Comment 5

[Terrence Cole [:terrence]]

LGTM, pushing:
https://hg.mozilla.org/integration/mozilla-inbound/rev/885e190cfad6

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/885e190cfad6