782802 - GC: make Reflect.parse support exact stack rooting

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Dave Herman [:dherman]]

Reflect.parse depends on conservative rooting. It needs to use the new exact rooting API.

Dave

Comment 1

[Dave Herman [:dherman]]

Notes from #jsapi:

jorendorff: dherman: mozilla-inbound
terrence: dherman: there are a few more handy tools that we want to get in to help with edge cases, but you can do everything that's needed with what's in gc/Root.h currently
terrence: dherman: speaking of wiki sprint days, the best docs are still in gc/Root.h
dherman: terrence: is there example code I could look to?
terrence: dherman: I don't know of any particular piece of code that I would look at as canonical: sfink, do you know of any specifically? ^^
sfink: let me look for something
terrence: dherman: you will need to configure a debug build with --enable-root-analysis
terrence: dherman: then you need to run with the analysis enabled by setting JS_GC_ZEAL=6 in your environment
sfink: dherman: no, that's just to get slapped if you do it wrong
sfink: dherman: it'll compile regardless
terrence: dherman: exactly... the Rooted doesn't actually root currently, it just ensures that when we flip the switch to use them instead of the conservative scanner nothing will break
sfink: dherman: js_FindClassObject in jsobj.cpp is an example of the main ways to use the stack rooting API
sfink: dherman: if you get stuck, you could read https://wiki.mozilla.org/Sfink/Draft_-_GC_Pointer_Handling to get even more stuck

Dave

Comment 2

[Dave Herman [:dherman]]

Also for my own reference, here's the old code I wrote to root with the auto-rooter API (never landed b/c conservative scanning landed in the meantime):

https://bug533874.bugzilla.mozilla.org/attachment.cgi?id=456405

Dave

Comment 4

[Nicholas Nethercote [inactive]]

I'm interested in this bug.  I configured a shell with --enable-root-analysis, and from within js/src/tests/ I ran this command:

  JS_GC_ZEAL=6 jstests.py ../r64/js js1_8_5/extensions/reflect-parse.js

I was expecting it to fall over in a heap, but it ran normally and passed.  Is this expected, or am I doing something wrong?

Comment 5

[Terrence Cole [:terrence]]

(In reply to Nicholas Nethercote [:njn] from comment #4)
> I was expecting it to fall over in a heap, but it ran normally and passed. 
> Is this expected, or am I doing something wrong?

Locally this test fails with the backtrace:

#0  0x000000000042710e in JSString::length (this=0x7fffda02cac0) at /home/terrence/moz/branch/surface_handlevalue/mozilla/js/src/vm/String.h:254
#1  0x0000000000464860 in js::StringBuffer::append (this=0x7fffffffc5b0, str=0x7fffda02cac0) at /home/terrence/moz/branch/surface_handlevalue/mozilla/js/src/vm/StringBuffer.h:102
#2  0x00000000006fd06c in NameResolver::resolveFun (this=0x7fffffffc880, pn=0xd079f0, prefix=0x7fffda02cac0) at /home/terrence/moz/branch/surface_handlevalue/mozilla/js/src/frontend/NameFunctions.cpp:192
#3  0x00000000006fd540 in NameResolver::resolve (this=0x7fffffffc880, cur=0xd079f0, prefix=0x7fffda02cac0) at /home/terrence/moz/branch/surface_handlevalue/mozilla/js/src/frontend/NameFunctions.cpp:282
#4  0x00000000006fd77c in NameResolver::resolve (this=0x7fffffffc880, cur=0xcbfc48, prefix=0x7fffda02cac0) at /home/terrence/moz/branch/surface_handlevalue/mozilla/js/src/frontend/NameFunctions.cpp:327
#5  0x00000000006fd77c in NameResolver::resolve (this=0x7fffffffc880, cur=0xcbfc00, prefix=0x7fffda02cac0) at /home/terrence/moz/branch/surface_handlevalue/mozilla/js/src/frontend/NameFunctions.cpp:327
#6  0x00000000006fd755 in NameResolver::resolve (this=0x7fffffffc880, cur=0xcbfb50, prefix=0x7fffda02cac0) at /home/terrence/moz/branch/surface_handlevalue/mozilla/js/src/frontend/NameFunctions.cpp:323
#7  0x00000000006fd77c in NameResolver::resolve (this=0x7fffffffc880, cur=0xcbfb08, prefix=0x0) at /home/terrence/moz/branch/surface_handlevalue/mozilla/js/src/frontend/NameFunctions.cpp:327
#8  0x00000000006fd623 in NameResolver::resolve (this=0x7fffffffc880, cur=0xde4430, prefix=0x0) at /home/terrence/moz/branch/surface_handlevalue/mozilla/js/src/frontend/NameFunctions.cpp:303
#9  0x00000000006fd80b in js::frontend::NameFunctions (cx=0xcbc590, pn=0xde4430) at /home/terrence/moz/branch/surface_handlevalue/mozilla/js/src/frontend/NameFunctions.cpp:338
#10 0x00000000006d89e7 in js::frontend::CompileScript (cx=0xcbc590, scopeChain=..., callerFrame=0x0, options=..., chars=0x7ffff5e1e010, length=0x11a43, source_=0x0, staticLevel=0x0)
    at /home/terrence/moz/branch/surface_handlevalue/mozilla/js/src/frontend/BytecodeCompiler.cpp:194
#11 0x000000000044f35b in JS::Compile (cx=0xcbc590, obj=..., options=..., chars=0x7ffff5e1e010, length=0x11a43) at /home/terrence/moz/branch/surface_handlevalue/mozilla/js/src/jsapi.cpp:5276
#12 0x000000000044f444 in JS::Compile (cx=0xcbc590, obj=..., options=..., 
    bytes=0x7ffff5e42010 "// |reftest| pref(javascript.options.xml.content,true) skip-if(!xulRuntime.shell)\n/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */\n/*\n * Any copyright is dedicated to th"..., 
    length=0x11a43) at /home/terrence/moz/branch/surface_handlevalue/mozilla/js/src/jsapi.cpp:5291
#13 0x000000000044f52d in JS::Compile (cx=0xcbc590, obj=..., options=..., fp=0xceae50) at /home/terrence/moz/branch/surface_handlevalue/mozilla/js/src/jsapi.cpp:5303
#14 0x00000000004503f2 in JS_CompileUTF8FileHandle (cx=0xcbc590, objArg=0x7fffdaf09060, filename=0x7fffffffe251 "js1_8_5/extensions/reflect-parse.js", file=0xceae50)
    at /home/terrence/moz/branch/surface_handlevalue/mozilla/js/src/jsapi.cpp:5511
#15 0x0000000000409228 in Process (cx=0xcbc590, obj_=0x7fffdaf09060, filename=0x7fffffffe251 "js1_8_5/extensions/reflect-parse.js", forceTTY=0x0) at /home/terrence/moz/branch/surface_handlevalue/mozilla/js/src/shell/js.cpp:429
#16 0x000000000041495f in ProcessArgs (cx=0xcbc590, obj_=0x7fffdaf09060, op=0x7fffffffdc60) at /home/terrence/moz/branch/surface_handlevalue/mozilla/js/src/shell/js.cpp:4724
#17 0x0000000000414c79 in Shell (cx=0xcbc590, op=0x7fffffffdc60, envp=0x7fffffffdea8) at /home/terrence/moz/branch/surface_handlevalue/mozilla/js/src/shell/js.cpp:4776
#18 0x0000000000415346 in main (argc=0xd, argv=0x7fffffffde38, envp=0x7fffffffdea8) at /home/terrence/moz/branch/surface_handlevalue/mozilla/js/src/shell/js.cpp:4952

Note the |this| pointer in the topmost frame: (this=0x7fff*da*02cac0).  The 0xda in byte four is the poisoning that occurred during the analysis.  The program has (correctly) broken at the next access to the unrooted thing, which is usually close enough for us to figure out what should have been rooted.

To be precise, the flags you need to configure with are:  --enable-root-analysis --enable-debug --enable-gczeal.  --enable-gczeal is implied by --enable-debug, and I am assuming that you built with --enable-debug because the purpose of triggering these crashes is to debug them.  If, on the other hand, the 'r' in r64 in your path above is for "release", then you just need to make a debug build for the analysis.  Steve checked in a patch yesterday to make --enable-root-analysis a configure error if you don't supply the right other flags.  I don't think I've perma-baked anything else important into my configure wrapper and subsequently forgotten about it, but if you pull, you can be sure.

Good luck!

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Start doing exact rooting in jsreflect.cpp.

This patch roots all the JSObjects in jsreflect.cpp, and a few other
knock-on ones.  All pretty straightforward.  I also added some missing 
MutableHandleFoo typedefs.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - More exact rooting in jsreflect.cpp.

This patch roots many, but not all, of the Values in jsreflect.cpp.  Sorry
the patch is so big, I couldn't get it much smaller and still have it
compile.

Basic stuff:

- Value locals --> RootedValue
- Value params --> HandleValue
- Value* params --> MutableHandleValue

Things worth looking at more closely:

- NodeBuilder objects only exist on the stack, AFAICT, hence the RootedValue
  within it.

- NodeBuilder::setProperty() changed a bit.

- NodeBuilder::newNodeLoc() changed quite a bit.  Note that |val| get
  re-assigned multiple times -- is that ok?

- I used address() in all the overloadings of callback() so that I didn't
  have to modify Invoke()'s signature, which would have caused 100s of
  knock-on changes all over the engine.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Yet more exact rooting in jsreflect.cpp, enough to turn on exact scanning with the root analysis.

Some things to think about carefully...

- I sure hope AutoValueArray is the right way to deal with Value arrays.  I
  had to use it in several places and it fixed problems, so I figure I'm
  doing something right.

- I'm returning a HandleValue in opt() which is against the usual rules (as
  I understand them) but I think it's ok in this case.  I wrote a comment
  about this.  Let me know if there's a better way.
  
- When I turned on exact rooting in reflect_parse(), the analysis of 
  reflect-parse.js went from taking a few seconds to 3 minutes.  Is this 
  expected?

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Still more exact rooting in jsreflect.cpp.

This one ferrets out some remaining JSAtom pointers.

Comment 10

[Terrence Cole [:terrence]]

Comment on attachment 660622 [details] [diff] [review]
(part 2) - More exact rooting in jsreflect.cpp.

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

(In reply to Nicholas Nethercote [:njn] from comment #7)
> Things worth looking at more closely:

Thanks for the list, it helped tremendously with the review!
 
> - NodeBuilder::newNodeLoc() changed quite a bit.  Note that |val| get
>   re-assigned multiple times -- is that ok?

Reassignment to an already rooted stack location is much faster than creating a new root.  I don't think it's *preferred* as such, but this particular usage is optimal.  

It's a shame the code is so much less compact now, but it's probably clearer given the use of the comma operator in the prior version.  It would be nice if we could have some way to put the val.setFoo() into the calls as they were before, but I can't see anything that's not a terrible idea for other reasons.  In short, I think this is fine.

> - I used address() in all the overloadings of callback() so that I didn't
>   have to modify Invoke()'s signature, which would have caused 100s of
>   knock-on changes all over the engine.

Perfect!  That's basically what we've been doing to split up the work.

::: js/src/jsreflect.cpp
@@ +422,5 @@
>  
> +        /* Represent "no node" as null and ensure users are not exposed to magic values. */
> +        RootedValue nullVal(cx, NullValue());
> +        return JSObject::defineProperty(cx, obj, atom->asPropertyName(),
> +                                        val.isMagic(JS_SERIALIZE_NO_NODE) ? nullVal : val);

I like this better than the if-stmt I suggested.  However, if you have to create a new Root anyway, maybe it would be even better as:

RootedValue v(cx, val.isMagic(JS_SERIALIZE_NO_NODE) ? NullValue() : val);
return JSObject::defineProperty(cx, obj, atom->asPropertyName(), v);

Comment 11

[Terrence Cole [:terrence]]

Comment on attachment 660723 [details] [diff] [review]
(part 3) - Yet more exact rooting in jsreflect.cpp, enough to turn on exact scanning with the root analysis.

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

(In reply to Nicholas Nethercote [:njn] from comment #8)
> Created attachment 660723 [details] [diff] [review]
> (part 3) - Yet more exact rooting in jsreflect.cpp, enough to turn on exact
> scanning with the root analysis.

Woot!
 
> Some things to think about carefully...
> 
> - I sure hope AutoValueArray is the right way to deal with Value arrays.  I
>   had to use it in several places and it fixed problems, so I figure I'm
>   doing something right.

At the moment, AutoFooArray contains a SkipRoot for the span of values on the stack.  This has the effect of suppressing the poisoning, but nothing else.  With our new rooting infrastructure, it will be easy to add a RootedArray<T> (painted whatever color we decide on eventually) that does exactly what AutoArray does now, but 100x faster.  We just haven't gotten around to it yet.
 
> - When I turned on exact rooting in reflect_parse(), the analysis of 
>   reflect-parse.js went from taking a few seconds to 3 minutes.  Is this 
>   expected?

Uhg. Yes, that's annoying but not unreasonable.  Most tests are short enough that a 100x slowdown doesn't trigger any timeouts on TBPL.  Of the ones that have timed out, most are either stupid tests or tests that are also benchmarks.  In these cases we've slapped a relaxRootChecks call at the top to suppress most of the stack checking.  We still do the stack checking in "relaxed" mode, except only when GC's do occur, not in all paths where they might occur.  3 minutes may still be fast enough.  We'll have to run it on try a few times to see.  We can always split it up or relax the checks later if it becomes an orangefactor.

::: js/src/jsapi.cpp
@@ +7168,5 @@
>                     "  2: GC every F allocations (default: 100)\n"
>                     "  3: GC when the window paints (browser only)\n"
>                     "  4: Verify pre write barriers between instructions\n"
>                     "  5: Verify pre write barriers between paints\n"
> +                   "  6: Verify stack rooting (ignoring XML)\n"

\o/

Which reminds me: 3.5 weeks until E4X dies!  I should start shopping for party supplies.

::: js/src/jsreflect.cpp
@@ +141,5 @@
>      RootedValue srcval;                /* source filename JS value or null      */
>      Value       callbacks[AST_LIMIT];  /* user-specified callbacks              */
> +    AutoValueArray callbacksRoots;     /* for rooting |callbacks|               */
> +    RootedValue userv;                 /* user-specified builder object or null */
> +    RootedValue undefinedVal;          /* a rooted undefined val, used by opt() */

We have quite a few of these RootedConst laying about.  It's fine for now, but I've opened bug 791022 to address this in a more general way.

@@ +293,5 @@
>  
> +    // WARNING: Returning a Handle is non-standard, but it works in this case
> +    // because both |v| and |undefinedVal| are definitely rooted on a previous
> +    // stack frame (i.e. we're just choosing between two already-rooted
> +    // values).

Nice.  I think the comment makes sense here.  We return a Handle for the GlobalObject and for the stack values in CallReceiver without a comment, but those cases are more obvious.

@@ -3364,5 @@
>  
>  static JSBool
>  reflect_parse(JSContext *cx, uint32_t argc, jsval *vp)
>  {
> -    cx->runtime->gcExactScanningEnabled = false;

\o/

Comment 12

[Terrence Cole [:terrence]]

Comment on attachment 660725 [details] [diff] [review]
(part 4) - Still more exact rooting in jsreflect.cpp.

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

Rooting of atoms is a weird edge case.  (1) Atoms are always held live, so we don't need exact roots to keep them from getting thrown away.  (2) Generational GC is never going to store atoms, so we won't need exact roots to update their address.  (3) We would like to compact the atoms compartment, _but_ we can just make compaction such that it can only happen when the stack is empty -- i.e. we know there are no Atoms on the stack to move.

That said, I personally am in favor of Root All The Things: in my mind, the flexibility and removal of the above awkward, full-engine constraints totally outweighs an as-yet-unmeasured speedup.  I think if we do eventually Unroot All The Atoms, it will be constrained to the frontend, which deals heavily with atoms and is very performance sensitive.

::: js/src/jsreflect.cpp
@@ +1695,5 @@
>      Parser              *parser;
>      NodeBuilder         builder;
>      DebugOnly<uint32_t> lineno;
>  
> +    RawValue unrootedAtomContents(RawAtom atom) {

Nice.

Comment 13

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

Atoms are not always kept alive. Interned atoms are always kept alive, but most atoms are not interned. During parsing we keep all atoms alive, but the reflect code doesn't run during parsing, as far as I know.

Comment 14

[Terrence Cole [:terrence]]

Well, I guess that makes this easy then. :-)

Comment 15

[Dave Herman [:dherman]]

Comment on attachment 660622 [details] [diff] [review]
(part 2) - More exact rooting in jsreflect.cpp.

Good heavens.

Comment 16

[Dave Herman [:dherman]]

(In reply to Bill McCloskey (:billm) from comment #13)
> the reflect code doesn't run during parsing, as far as I know.

That's correct.

Dave

Comment 17

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d58ca3879862
https://hg.mozilla.org/integration/mozilla-inbound/rev/24da8a732dcb
https://hg.mozilla.org/integration/mozilla-inbound/rev/70370ccbf9d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a5c6e070367

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d58ca3879862
https://hg.mozilla.org/mozilla-central/rev/24da8a732dcb
https://hg.mozilla.org/mozilla-central/rev/70370ccbf9d9
https://hg.mozilla.org/mozilla-central/rev/2a5c6e070367

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 660723 [details] [diff] [review]
(part 3) - Yet more exact rooting in jsreflect.cpp, enough to turn on exact scanning with the root analysis.

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

::: js/src/jsreflect.cpp
@@ +202,5 @@
>              RootedValue loc(cx);
>              if (!newNodeLoc(pos, &loc))
>                  return false;
>              Value argv[] = { loc };
> +            AutoValueArray ava(cx, argv, 1);

Passing the length manually seems error-prone... Can we have an AutoValueArray constructor that uses the template trick (<http://whereswalden.com/2011/10/20/implementing-mozillaarraylength-and-mozillaarrayend-and-some-followup-work/>)?