Closed Bug 767141 Opened 8 years ago Closed 8 years ago

moar rooting 1

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: sfink, Assigned: sfink)

References

Details

(Whiteboard: [js:t])

Attachments

(5 files, 1 obsolete file)

I'm running the rooting analysis, and it's failing all over the place during initialization. I manually went through all of jsapi.cpp, CTypes.cpp, and frontend/BytecodeCompiler.cpp and fixed them up. I also went through many other places that were affected or that I happened to hit in my simple test case. I originally intended to fix up everything necessary for my test file to run under |-m -n|, but eventually gave up on doing JM incrementally.

So anyway, this bug is for an arbitrary slice of the engine, with 100% coverage of the above mentioned 3 files.
This RAII guard is used to mark lexical regions that are believed to never trigger GC. It is mostly intended for documentation, but is also dynamically checked.
Attachment #635462 - Flags: review?(bhackett1024)
Given that AutoVectorRooter roots a vector of gcthings, it seems convenient to be able to access elements for passing into Handle-accepting functions without re-rooting.
Attachment #635465 - Flags: review?(bhackett1024)
Attached patch moar rootingSplinter Review
Believed to be complete rooting for jsapi.cpp, ctypes/CTypes.cpp, and frontend/BytecodeCompiler.cpp, and lots of rooting for additional things I encountered along the way
Attachment #635466 - Flags: review?(bhackett1024)
Whiteboard: [js:t]
Comment on attachment 635462 [details] [diff] [review]
Implement AssertRootingUnnecessary guard

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

::: js/src/gc/Root.h
@@ +319,5 @@
>  
> +#if defined(JSGC_ROOT_ANALYSIS) && defined(DEBUG) && !defined(JS_THREADSAFE)
> +bool IsRootingUnnecessaryForContext(JSContext *cx);
> +void SetRootingUnnecessaryForContext(JSContext *cx, bool value);
> +class AssertRootingUnnecessary {

I think it would be better if the class definition was outside the #ifdef, so it only has to appear once, and you can use the GUARD_OBJECT_NOTIFIER stuff in all cases without duplicate boilerplate.

I also think you can just make the contents of this class #ifdef DEBUG, and assert !cx->rootingUnnecessary within MaybeCheckStackRoots in any debug build.

This class is a great idea and it would be good to push the assertion checking even further.  r+ as is, but it would be great if you could make these changes and post an updated patch.
Attachment #635462 - Flags: review?(bhackett1024) → review+
Attachment #635465 - Flags: review?(bhackett1024) → review+
Comment on attachment 635466 [details] [diff] [review]
moar rooting

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

Thanks!

::: js/src/ctypes/CTypes.cpp
@@ +2362,5 @@
>  // Convert jsval 'val' to a C binary representation of CType 'targetType',
>  // storing the result in 'buffer'. This function is more forceful than
>  // ImplicitConvert.
>  JSBool
> +ExplicitConvert(JSContext* cx, jsval val, JSObject* targetType_, void* buffer)

Is there a reason for taking a bare JSObject* here and not a handle?  Is this used as a function pointer (can also change the function pointer signature)?

Generally these should be replaced with handles and rooting within the callee avoided, mostly so that C++ will type check the caller to see that it uses roots too.

Ditto for other instances of this below.

jsapi.cpp does this a lot but that will change pretty soon once the handle based API is done.
Attachment #635466 - Flags: review?(bhackett1024) → review+
Comment on attachment 635466 [details] [diff] [review]
moar rooting

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

::: js/src/frontend/BytecodeCompiler.h
@@ +13,5 @@
>  namespace js {
>  namespace frontend {
>  
>  bool
> +CompileFunctionBody(JSContext *cx, HandleFunction fun,

There was some quasi-consensus on the js-internals mailing list that the Handle<JSFunction*> form was preferable...

::: js/src/jsscript.h
@@ +567,5 @@
>    public:
>      static JSScript *Create(JSContext *cx, bool savedCallerFun,
>                              JSPrincipals *principals, JSPrincipals *originPrincipals,
>                              bool compileAndGo, bool noScriptRval,
> +                            js::GlobalObject* globalObject, JSVersion version,

ORLY?  No need to change this, plz.
(In reply to Nicholas Nethercote [:njn] from comment #6)
> Comment on attachment 635466 [details] [diff] [review]
> moar rooting
> 
> Review of attachment 635466 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/frontend/BytecodeCompiler.h
> @@ +13,5 @@
> >  namespace js {
> >  namespace frontend {
> >  
> >  bool
> > +CompileFunctionBody(JSContext *cx, HandleFunction fun,
> 
> There was some quasi-consensus on the js-internals mailing list that the
> Handle<JSFunction*> form was preferable...

I'll raise this in the GC meeting today, but I'm kind of expecting a full trivial rewrite pass of some sort once the naming bikeshedding is complete, so I don't much care. And the discussion on the mailing list wasn't close enough to a consensus yet (as in, there were fairly strong opinions for both sides).

> ::: js/src/jsscript.h
> @@ +567,5 @@
> >    public:
> >      static JSScript *Create(JSContext *cx, bool savedCallerFun,
> >                              JSPrincipals *principals, JSPrincipals *originPrincipals,
> >                              bool compileAndGo, bool noScriptRval,
> > +                            js::GlobalObject* globalObject, JSVersion version,
> 
> ORLY?  No need to change this, plz.

Doh! Thanks. I think the ctypes convention is the opposite and that got stuck in my head. Though I have a suspicion that current-day JS developers may actually prefer |type* var|, and the Handle-ification of the API wouldn't be the worst possible time for a flag day... nah, never mind.
(In reply to Brian Hackett (:bhackett) from comment #5)
> Comment on attachment 635466 [details] [diff] [review]
> moar rooting
> 
> Review of attachment 635466 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks!
> 
> ::: js/src/ctypes/CTypes.cpp
> @@ +2362,5 @@
> >  // Convert jsval 'val' to a C binary representation of CType 'targetType',
> >  // storing the result in 'buffer'. This function is more forceful than
> >  // ImplicitConvert.
> >  JSBool
> > +ExplicitConvert(JSContext* cx, jsval val, JSObject* targetType_, void* buffer)
> 
> Is there a reason for taking a bare JSObject* here and not a handle?  Is
> this used as a function pointer (can also change the function pointer
> signature)?

Because I wasn't changing public API, and ExplicitConvert is defined in CTypes.h so I assume that it is part of the API.

On the other hand, I also didn't convert a bunch of the static functions defined in a collection of namespaces at the top of CTypes.cpp, and there's no reason not to.

> Generally these should be replaced with handles and rooting within the
> callee avoided, mostly so that C++ will type check the caller to see that it
> uses roots too.
> 
> Ditto for other instances of this below.
> 
> jsapi.cpp does this a lot but that will change pretty soon once the handle
> based API is done.

I think I'll land this now and do the APIs together. (Or almost "this" -- I also switched the static functions to handles, too.)
Still trying to get this through the try server.

This patch does some especially ugly juggling to root a Type used as a key. Please give me a simple alternative to avoid this. :)
Attachment #637745 - Flags: review?(bhackett1024)
Attached patch null check for AutoGCVector (obsolete) — Splinter Review
I'm not sure about the perf implications of this patch, so I wanted to run it by billm.

The problem is when I use an auto vector rooter, the vector can contain NULLs, and those get passed into MarkInternal, which barfs on them. Sorry; I should have kept a stack trace. I don't remember the exact names. But it's a vector of JSObject* and it's doing something like MarkRange, which just goes over every entry and passes in the address of each. The addresses are of course non-NULL, but the thing=*thingp can be NULL.
Attachment #637746 - Flags: review?(wmccloskey)
Comment on attachment 637745 [details] [diff] [review]
nasty Type rooting

Eh, this looks fine to me.  Bug 767223 makes GCs during AutoEnterTypeInference impossible, which would remove the need for this root, but I suspect there are other places Types may need to be rooted even with that change.
Attachment #637745 - Flags: review?(bhackett1024) → review+
Comment on attachment 637746 [details] [diff] [review]
null check for AutoGCVector

Talked to billm over IRC; moved the null check to the range rooter and folded into the rooting patch.
Attachment #637746 - Attachment is obsolete: true
Attachment #637746 - Flags: review?(wmccloskey)
In addition to comment 15, this turned mozilla-inbound_linux64-debug_spidermonkey-rootanalysis from orange (https://tbpl.mozilla.org/php/getParsedLog.php?id=13122160&tree=Mozilla-Inbound) to red (https://tbpl.mozilla.org/php/getParsedLog.php?id=13124032&tree=Mozilla-Inbound). It is hidden on TBPL, so not something that anything would be backed out over - but just thought I'd mention :-)
It compiled successfully after I rebased; how could it possibly be wrong? :)
You need to log in before you can comment on or make changes to this bug.