714647 - Add rooters to get rooting analysis to pass sunspider/v8bench

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch (10f831bfaf08)

Add uses of rooter and handle classes to get the sunspider and v8bench tests in jit-tests to pass (along with a lot of other jit-tests).

Comment 1

[Paul Wright]

Is this bug still needed?

Comment 2

[Brian Hackett [Laid off!]]

Yes, this is necessary for generational GC.  Landing it immediately isn't critical, but I'd like it to stop rotting.  Bill, can you review this pretty soon?  (after incremental GC lands).  It is large but very straightforward and should just need skimming.

Comment 3

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

Comment on attachment 585295 [details] [diff] [review]
patch (10f831bfaf08)

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

Sorry again for the delay. This looks nice.

I guess we have to figure out what to do for char* pointers to inlined strings. Maybe we should have a special kind of rooter for those.

::: js/src/frontend/Parser.cpp
@@ +1306,5 @@
>  #endif /* JS_HAS_DESTRUCTURING */
>  
>                case TOK_NAME:
>                {
> +                RootedVar<PropertyName*> name(context, tokenStream.currentToken().name());

RootedVarPropertyName?

::: js/src/frontend/TokenStream.h
@@ +832,5 @@
>      void updateLineInfoForEOL();
>      void updateFlagsForEOL();
>  
>      Token               tokens[ntokens];/* circular token buffer */
> +    CheckRoot           tokensRoot;     /* prevent overwriting of token buffer */

Please explain this. Maybe we don't need to root this because it only contains atoms and because we never trace through it during GC?

::: js/src/jsapi.cpp
@@ +3577,4 @@
>                   JSPropertyOp getter, JSStrictPropertyOp setter, uintN attrs)
>  {
>      CHECK_REQUEST(cx);
> +    RootObject obj(cx, &obj_);

Can you use RootedVarObject here?

@@ +3649,4 @@
>                   const Value &value, PropertyOp getter, StrictPropertyOp setter, uintN attrs,
>                   uintN flags, intN tinyid)
>  {
> +    RootObject obj(cx, &obj_);

and here?

@@ +3978,3 @@
>          return JS_FALSE;
>      if (objp)
>          *objp = obj;

Couldn't obj have been poisoned at this point?

::: js/src/jscntxt.h
@@ +1589,4 @@
>  typedef Root<Value>              RootValue;
>  
>  /* Mark a stack location as a root for a rooting analysis. */
>  class CheckRoot

I realize I should have mentioned this in the previous patch, but the name CheckRoot is confusing. Wouldn't SkipRoot be more appropriate?

::: js/src/jsfun.cpp
@@ +428,5 @@
>      if (!obj->isStrictArguments())
>          return true;
>  
> +    RootedVar<StrictArgumentsObject*> argsobj(cx, obj->asStrictArguments());
> +    RootId idRoot(cx, &id);

Can you use RootedVarId?

::: js/src/jsinferinlines.h
@@ +1327,5 @@
>  inline bool
>  JSScript::ensureRanAnalysis(JSContext *cx, JSObject *scope)
>  {
>      JSScript *self = this;
> +    js::CheckRoot root(cx, &self);

Why don't we just root self? I asked the same question last time and I can't find an answer.

::: js/src/jsinterp.cpp
@@ +3198,5 @@
>                      JS_ASSERT(entry->kshape != entry->pshape);
>                      JS_ASSERT(!shape->hasSlot());
>                  }
>  #endif
> +                */

I'm not familiar with this code, but these assertions seem kind of useful. Why do they have to go?

::: js/src/jspropertycache.cpp
@@ -59,5 @@
> -     */
> -    if (!pobj->nativeContains(cx, *shape)) {
> -        PCMETER(oddfills++);
> -        return JS_NO_PROP_CACHE_FILL;
> -    }

Again, I don't understand what's happening here.

::: js/src/jsscope.cpp
@@ +312,5 @@
>      JS_ASSERT(!inDictionary());
>  
>      Shape *shape = JS_PROPERTY_TREE(cx).getChild(cx, this, numFixedSlots(), child);
>      if (shape) {
> +        //JS_ASSERT(shape->parent == this);

Is this not worth keeping?

::: js/src/jsscope.h
@@ -710,5 @@
>      /* Copy constructor disabled, to avoid misuse of the above form. */
> -    Shape(const Shape &other);
> -
> -    /* Not defined: Shapes must not be stack allocated. */
> -    ~Shape();

Why this change?

::: js/src/vm/StringObject-inl.h
@@ +56,5 @@
>  StringObject::init(JSContext *cx, JSString *str)
>  {
>      JS_ASSERT(gc::GetGCKindSlots(getAllocKind()) == 2);
>  
> +    setStringThis(str);

Moving this up means changes semantics if we OOM. Can we just root str?

Comment 4

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4c82a6b298c

According to Try this dings dromaeo-v8 and dromaeo-basics by a couple %.  Other dromaeo tests, including DOM and CSS, are unaffected.  Don't back this patch out because of these regressions.

Comment 5

[Matt Brubeck (:mbrubeck)]

Sorry, I had to back this out (along with two no-bug followups by Waldo):
https://hg.mozilla.org/integration/mozilla-inbound/rev/906aa73122d9

because it apparently caused crashes during test_finalizer.js in Linux64 *PGO* builds only:
https://tbpl.mozilla.org/php/getParsedLog.php?id=10861493&tree=Mozilla-Inbound

Comment 6

[Marco Bonardo [:mak]]

(In reply to Brian Hackett (:bhackett) from comment #4)
> According to Try this dings dromaeo-v8 and dromaeo-basics by a couple %. 
> Other dromaeo tests, including DOM and CSS, are unaffected.

inbound shows also regressions on DOM, CSS, String/Array/Eval/Regexp, jslib.

Comment 7

[Matt Brubeck (:mbrubeck)]

(In reply to Matt Brubeck (:mbrubeck) from comment #5)
> Sorry, I had to back this out (along with two no-bug followups by Waldo)
> because it apparently caused crashes during test_finalizer.js in Linux64
> *PGO* builds only

The backout did not fix the crashes, so I will re-land the backed-out changesets.  Sorry again.

Comment 8

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4cfb132c7c16
https://hg.mozilla.org/integration/mozilla-inbound/rev/95fd0e025439
https://hg.mozilla.org/integration/mozilla-inbound/rev/94eb880e0cad

Comment 9

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/4cfb132c7c16
https://hg.mozilla.org/mozilla-central/rev/95fd0e025439
https://hg.mozilla.org/mozilla-central/rev/94eb880e0cad

If possible, please don't push any no-bug changesets that is not a whitespace or comment change.