Last Comment Bug 764440 - GC: Add option to enable use of exact stack rooting.
: GC: Add option to enable use of exact stack rooting.
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla16
Assigned To: Terrence Cole [:terrence]
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: ExactRooting
  Show dependency treegraph
Reported: 2012-06-13 09:12 PDT by Terrence Cole [:terrence]
Modified: 2012-06-16 06:50 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

v0 (5.50 KB, patch)
2012-06-13 09:12 PDT, Terrence Cole [:terrence]
wmccloskey: review+
Details | Diff | Splinter Review
v1 (7.20 KB, patch)
2012-06-13 14:48 PDT, Terrence Cole [:terrence]
wmccloskey: review+
Details | Diff | Splinter Review

Description User image Terrence Cole [:terrence] 2012-06-13 09:12:05 PDT
Created attachment 632727 [details] [diff] [review]

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 User image Bill McCloskey (:billm) 2012-06-13 10:24:17 PDT
Comment on attachment 632727 [details] [diff] [review]

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

::: js/src/
@@ +3882,5 @@
>  dnl ========================================================
> +dnl = Use exact stack rooting for GC
> +dnl ========================================================

What is this line for?

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

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 User image Terrence Cole [:terrence] 2012-06-13 14:48:03 PDT
Created attachment 632891 [details] [diff] [review]

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:

Does:  Always runs rooting analysis, but uses normal GC.
Used:  To run the rooting analysis on TBPL.

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

Does:  Always runs rooting analysis and then uses roots for GC.
Used:  Probably useless.

Does:  Runs rooting analysis only with zeal and uses roots for GC.
Used:  To optimize rooting after exact rooting is stable.
Comment 3 User image Bill McCloskey (:billm) 2012-06-13 14:57:06 PDT
Comment on attachment 632891 [details] [diff] [review]

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 User image Terrence Cole [:terrence] 2012-06-15 15:35:43 PDT
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, 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:

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.

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:
Comment 5 User image Terrence Cole [:terrence] 2012-06-15 18:04:39 PDT
LGTM, pushing:
Comment 6 User image Ryan VanderMeulen [:RyanVM] 2012-06-16 06:50:27 PDT

Note You need to log in before you can comment on or make changes to this bug.