847934 - GC: Remove Unrooted

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jon Coppeard (:jonco)]

Attachment: 1 - Remove use of Unrooted

Removes use of DropUnrooted and renames UnrootedThings to RawThings

Comment 2

[Jon Coppeard (:jonco)]

Attachment: 2 - Remove definitions of Unrooted

Comment 3

[Terrence Cole [:terrence]]

Comment on attachment 721259 [details] [diff] [review]
1 - Remove use of Unrooted

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

It's mostly indentation changes, places were we want to return NULL instead of RawFoo(NULL), and unnecessary casts. You can probably fix most of this with a few find-and-replace on the patch file.

::: js/src/builtin/Eval.cpp
@@ +297,1 @@
>                                                            chars.get(), length, stableStr, staticLevel);

Indentation.

@@ +358,1 @@
>                                                            chars.get(), length, stableStr, staticLevel);

Indentation.

::: js/src/frontend/BytecodeCompiler.cpp
@@ +99,5 @@
>      JS_ASSERT_IF(evalCaller, options.compileAndGo);
>      JS_ASSERT_IF(staticLevel != 0, evalCaller);
>  
>      if (!CheckLength(cx, length))
> +        return RawScript(NULL);

These, and the rest in here should just be return NULL. Just grep for "return Raw".

::: js/src/jsarray.cpp
@@ +154,5 @@
>      JS_ASSERT(globalObj);
>  
>      JSObject *proto = globalObj->global().getOrCreateArrayPrototype(cx);
>      if (!proto)
> +        return RawShape(NULL);

Just NULL.

::: js/src/jsatom.cpp
@@ +272,3 @@
>      if (!flat) {
>          js_free((void*)tbchars);
> +        return RawAtom();

Return NULL.

@@ +278,4 @@
>  
>      if (!cx->runtime->atoms.relookupOrAdd(p, lookup, AtomStateEntry(atom, bool(ib)))) {
>          JS_ReportOutOfMemory(cx); /* SystemAllocPolicy does not report OOM. */
> +        return RawAtom();

ditto

@@ +318,4 @@
>  
>      if (!cx->runtime->atoms.relookupOrAdd(p, lookup, AtomStateEntry(atom, bool(ib)))) {
>          JS_ReportOutOfMemory(cx); /* SystemAllocPolicy does not report OOM. */
> +        return RawAtom();

ditto

@@ +393,5 @@
>      }
>  
>      jschar *tbcharsZ = InflateString(cx, bytes, &length);
>      if (!tbcharsZ)
> +        return RawAtom();

ditto

::: js/src/jsfriendapi.h
@@ +683,5 @@
>  
>  JS_FRIEND_API(void)
>  EnableRuntimeProfilingStack(JSRuntime *rt, bool enabled);
>  
> +// Use RawScript rather than RawScript because it may be called from a

Use RawScript rather than RawScript. I think we can remove this comment :-).

::: js/src/jsfun.h
@@ +187,5 @@
>          if (isInterpretedLazy()) {
>              js::RootedFunction self(cx, this);
>              js::MaybeCheckStackRoots(cx);
>              if (!self->initializeLazyScript(cx))
> +                return js::RawScript(NULL);

Return NULL.

@@ +211,5 @@
>          return JS::HandleScript::fromMarkedLocation(&u.i.script_);
>      }
>  
> +    js::RawScript maybeNonLazyScript() const {
> +        return isInterpreted() ? nonLazyScript() : js::RawScript(NULL);

return .... NULL;

::: js/src/jsinfer.cpp
@@ +856,4 @@
>  GetSingletonShape(JSContext *cx, RawObject obj, RawId idArg)
>  {
>      if (!obj->isNative())
> +        return RawShape(NULL);

return NULL;

@@ +862,3 @@
>      if (shape && shape->hasDefaultGetter() && shape->hasSlot())
>          return shape;
> +    return RawShape(NULL);

ditto

::: js/src/jsobj.cpp
@@ +3111,5 @@
>       * this optimistically (assuming no failure below) before locking obj, so
>       * we can lock the shadowed scope.
>       */
>      if (!js_PurgeScopeChain(cx, obj, id))
> +        return RawShape(NULL);

return NULL

::: js/src/jsobj.h
@@ +739,1 @@
>                                                     js::HandleShape parent, js::StackShape &child);

Indentation.

@@ +750,5 @@
>                                                   JS::HandleObject obj, JS::HandleId id,
>                                                   JSPropertyOp getter, JSStrictPropertyOp setter,
>                                                   uint32_t slot, unsigned attrs,
>                                                   unsigned flags, int shortid, js::Shape **spp,
>                                                   bool allowDictionary);

Indentation.

@@ +769,3 @@
>                                           JSPropertyOp getter, JSStrictPropertyOp setter,
>                                           uint32_t slot, unsigned attrs, unsigned flags,
>                                           int shortid, bool allowDictionary = true);

Indentation.

@@ +790,3 @@
>                                           JSPropertyOp getter, JSStrictPropertyOp setter,
>                                           uint32_t slot, unsigned attrs,
>                                           unsigned flags, int shortid);

ditto

@@ +794,4 @@
>                                           js::PropertyName *name,
>                                           JSPropertyOp getter, JSStrictPropertyOp setter,
>                                           uint32_t slot, unsigned attrs,
>                                           unsigned flags, int shortid)

ditto

@@ +805,2 @@
>                                              js::HandleShape shape, unsigned attrs, unsigned mask,
>                                              JSPropertyOp getter, JSStrictPropertyOp setter);

ditto

::: js/src/jsonparser.cpp
@@ +93,1 @@
>                                        : buffer.finishString();

1) Indentation.
2) You don't need the cast anymore.

::: js/src/jspropertytree.cpp
@@ +193,2 @@
>      if (!shape)
> +        return RawShape(NULL);

Return NULL;

@@ +195,5 @@
>  
>      new (shape) Shape(child, nfixed);
>  
>      if (!insertChild(cx, parent, shape))
> +        return RawShape(NULL);

ditto

::: js/src/jsscript.cpp
@@ +1698,5 @@
>                   ScriptSource *ss, uint32_t bufStart, uint32_t bufEnd)
>  {
>      RootedScript script(cx, js_NewGCScript(cx));
>      if (!script)
> +        return RawScript(NULL);

Return NULL; here and the other places in this file.

::: js/src/jsscript.h
@@ +190,5 @@
>      unsigned numVars() const { return numVars_; }
>      unsigned count() const { return numArgs() + numVars(); }
>  
>      /* Return the initial shape of call objects created for this scope. */
> +    RawShape callObjShape() const { return callObjShape_.get(); }

.get() should no longer be necessary.

@@ +530,2 @@
>                                      const JS::CompileOptions &options, unsigned staticLevel,
>                                      js::ScriptSource *ss, uint32_t sourceStart, uint32_t sourceEnd);

Indentation.

::: js/src/methodjit/MethodJIT.cpp
@@ +1407,5 @@
>  mjit::GetPICSingleShape(JSContext *cx, JSScript *script, jsbytecode *pc, bool constructing)
>  {
>      ic::PICInfo *pic = GetPIC(cx, script, pc, constructing);
>      if (!pic)
> +        return RawShape(NULL);

return NULL;

::: js/src/methodjit/PolyIC.h
@@ +497,2 @@
>          if (disabled || hadUncacheable || stubsGenerated > 0)
> +            return RawShape(NULL);

Return NULL

::: js/src/shell/js.cpp
@@ +1319,5 @@
>  ValueToScript(JSContext *cx, jsval v, JSFunction **funp = NULL)
>  {
>      RootedFunction fun(cx, JS_ValueToFunction(cx, v));
>      if (!fun)
> +        return RawScript(NULL);

Return NULL.

::: js/src/vm/RegExpObject.cpp
@@ +282,5 @@
>      RootedObject self(cx, this);
>  
>      /* The lastIndex property alone is writable but non-configurable. */
>      if (!addDataProperty(cx, cx->names().lastIndex, LAST_INDEX_SLOT, JSPROP_PERMANENT))
> +        return RawShape(NULL);

return NULL, and the rest below.

::: js/src/vm/SPSProfiler.cpp
@@ +235,5 @@
>      pcLengths(pcLengths),
>      chunk(chunk)
>  {}
>  
> +// Use RawScript instead of RawScript because this may be called from a

"Use RawScript instead of RawScript"

@@ +265,5 @@
>  
>      return NULL;
>  }
>  
> +// Use RawScript instead of RawScript because this may be called from a

ditto

::: js/src/vm/ScopeObject.cpp
@@ +63,5 @@
>  {
>      JS_ASSERT(hasDynamicScopeObject());
>      JS_ASSERT(type() != NAMED_LAMBDA);
>      return type() == BLOCK
> +           ? RawShape(block().lastProperty())

Manual casting shouldn't be needed anymore.

@@ +698,5 @@
>      /* Inline JSObject::addProperty in order to trap the redefinition case. */
>      Shape **spp;
>      if (Shape::search(cx, block->lastProperty(), id, &spp, true)) {
>          *redeclared = true;
> +        return RawShape(NULL);

return NULL

::: js/src/vm/ScopeObject.h
@@ +352,1 @@
>                                  int index, bool *redeclared);

Indentation.

::: js/src/vm/Shape.cpp
@@ +311,2 @@
>          if (!nbase)
> +            return RawShape(NULL);

return NULL; here and several other places in this file.

@@ +772,1 @@
>                                           attrs, shape->flags, shape->maybeShortid());

Indentation.

::: js/src/vm/Shape.h
@@ +500,1 @@
>                                         Shape ***pspp, bool adding = false);

Indentation.

@@ +512,1 @@
>                                               TaggedProto proto, HandleShape shape);

ditto

@@ +1088,5 @@
>          if (shape->propidRef() == id)
>              return shape;
>      }
>  
> +    return RawShape(NULL);

return NULL

::: js/src/vm/Stack-inl.h
@@ +562,5 @@
>      if (fp->beginsIonActivation()) {
>          JSScript *script = NULL;
>          ion::GetPcScript(cx_, &script, ppc);
>          if (!allowCrossCompartment && script->compartment() != cx_->compartment)
> +            return RawScript(NULL);

return NULL; and elsewhere in this file.

::: js/src/vm/Stack.h
@@ +731,4 @@
>          return isFunctionFrame()
>                 ? isEvalFrame()
>                   ? u.evalScript
>                   : (RawScript)fun()->nonLazyScript()

No more cast needed here.

@@ +1737,1 @@
>                                          MaybeAllowCrossCompartment = DONT_ALLOW_CROSS_COMPARTMENT) const;

Indentation.

::: js/src/vm/String-inl.h
@@ +31,2 @@
>                                 ? JSInlineString::new_<allowGC>(cx)
>                                 : JSShortString::new_<allowGC>(cx);

Indentation.

::: js/src/vm/StringBuffer.cpp
@@ +42,5 @@
>  StringBuffer::finishString()
>  {
>      JSContext *cx = context();
>      if (cb.empty())
> +        return RawFlatString(cx->names().empty);

No cast needed.

@@ +47,4 @@
>  
>      size_t length = cb.length();
>      if (!JSString::validateLength(cx, length))
> +        return RawFlatString();

return NULL; and elsewhere in this file.

@@ +73,5 @@
>      JSContext *cx = context();
>  
>      size_t length = cb.length();
>      if (length == 0)
> +        return RawAtom(cx->names().empty);

No cast needed.

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 721260 [details] [diff] [review]
2 - Remove definitions of Unrooted

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

Yay!

::: js/src/gc/Root.h
@@ -105,5 @@
> - * - DropUnrooted(UnrootedT &v) will poison |v| and end its AutoAssertNoGC
> - *   scope. This can be used to force |v| out of scope before its C++ scope
> - *   would end naturally. The usage of braces C++ syntactical scopes |{...}|
> - *   is strongly perferred to this, but sometimes will not work because of
> - *   awkwardly overlapping lifetimes.

Thanks for remembering to remove the comment -- I almost always forget to.

@@ +100,5 @@
>   *
>   * The following diagram explains the list of supported, implicit type
>   * conversions between classes of this family:
>   *
> + *  RawT -----> Rooted<T> <---> Handle<T>

It looks like there is a pre-existing mistake here:
RawT ----> Rooted<T> ----> Handle<T>

@@ +105,1 @@
>   *                 ^               ^

Likewise, MutableHandle doesn't implicitly convert to a Rooted.

Comment 5

[Jon Coppeard (:jonco)]

(In reply to Terrence Cole [:terrence] from comment #3)

Wow, thanks for reviewing, and sorry there were so many comments!

Comment 6

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fcb84b09972f
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f3445fecec1

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fcb84b09972f
https://hg.mozilla.org/mozilla-central/rev/5f3445fecec1