Last Comment Bug 714647 - Add rooters to get rooting analysis to pass sunspider/v8bench
: Add rooters to get rooting analysis to pass sunspider/v8bench
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla14
Assigned To: general
:
Mentors:
Depends on:
Blocks: GenerationalGC 707049
  Show dependency treegraph
 
Reported: 2012-01-02 09:27 PST by Brian Hackett (:bhackett)
Modified: 2013-01-16 04:51 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (10f831bfaf08) (381.65 KB, patch)
2012-01-02 09:27 PST, Brian Hackett (:bhackett)
wmccloskey: review+
Details | Diff | Splinter Review

Description Brian Hackett (:bhackett) 2012-01-02 09:27:31 PST
Created attachment 585295 [details] [diff] [review]
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 2012-01-22 23:29:58 PST
Is this bug still needed?
Comment 2 Brian Hackett (:bhackett) 2012-01-23 06:04:17 PST
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 (:billm) 2012-02-01 16:30:59 PST
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 (:bhackett) 2012-04-12 09:27:15 PDT
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) 2012-04-12 22:10:43 PDT
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] 2012-04-13 02:04:41 PDT
(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) 2012-04-13 07:42:09 PDT
(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 9 Marco Bonardo [::mak] 2012-04-14 06:33:46 PDT
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.

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