391676 - ActionMonkey: Remove stack TempValueRooters

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Thanks to MMgc's conservative stack scanning, most TempValueRooters are now unnecessary.  A handful will require special attention--a separate task.

The ones that can be removed now are those that simply protect local (stack) variables (including struct members and array elements).

Since MMgc only scans the stack of the calling thread, not all threads, this sacrifices JSRuntime thread-safety.  This is acceptable collateral damage per Brendan.  (Brendan please note:  Up to now, I think we've retained thread-safety.  All calls into MMgc go through jsgc.cpp, and I have not removed any locking code from there.)

Comment 1

[arno renevier]

Attachment: removes many useless TempValueRooters

Here is a patch that deletes JSTempValueRooter protecting stack variables. Additionally, some cleanup was made possible by those removals (a few goto could be removed).

Maybe more TempValueRooters can be removed. When unsure, I prefered not to remove them.

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 276123 [details] [diff] [review]
removes many useless TempValueRooters

Arno is already working on a new patch to replace this, expected on Monday or so.

Comment 3

[arno renevier]

Attachment: removes more TempValueRooters

Thanks to Jason explanations on irc, I could remove more TempValueRooters.

There are still a few remaining:

one in jsarray.cpp:array_sort, and one in jsobj.cpp:js_NewObject : they protect variables allocated via new or JS_malloc, and therefore, MMGc does not hold a reference on them

one in jsinterp:js_GetScopeChain
it is pushed with JS_PUSH_TEMP_ROOT_OBJECT, and therefore, tvr.object becomes a pointer to a JSObject. But a few lines after, tvr.value is read. I did not understood that, so I did not touch that.

one in jsgc.cpp:js_TraceContext that one is also too difficult for me to understand.

Comment 4

[Ed Lee :Mardak]

(In reply to comment #3)
> one in jsgc.cpp:js_TraceContext that one is also too difficult for me to
> understand.
Are you referring to to the part that iterates through the stack of JSTempValueRooter? How many instances of TempValueRooters are going to remain? If none, we don't need to scan that stack and trace items to mark. (I.e., remove the code.)

Comment 5

[Ed Lee :Mardak]

Attachment: idea: remove JSTempValueRooter.u.array

Just an idea.. See patch for comments for removing JSTempValueRooter's array of jsvals.

Comment 6

[Ed Lee :Mardak]

Attachment: js_NewObject's tvr

(In reply to comment #3)
> jsobj.cpp:js_NewObject : they protect
> variables allocated via new or JS_malloc, and therefore, MMGc does not hold a
> reference on them
Removing tvr for JSObject is fine because JSObject inherits from JSFinalizedGCThing (overloaded new) which will be MMgc allocated.

(In reply to comment #5)
>-class JSAutoTempValueRooter
Mmm.. probably not. Seems like somethings are using it.. modules/plugin/base/src/nsJSNPRuntime.cpp modules/plugin/base/src/ns4xPlugin.cpp
So we'll definitely need to keep things around as those won't be MMgc allocated. This also means the logic in js_TraceContext needs to stay for various "count" types probably except weak roots.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #6)
> modules/plugin/base/src/nsJSNPRuntime.cpp
> modules/plugin/base/src/ns4xPlugin.cpp

If you really want to get rid of JSAutoTempValueRooter, couldn't you convert the code in question over to using straight jsvals on the C stack, and let MMgc's conservative collector do its thing?

Comment 8

[arno renevier]

Attachment: removes also js_NewObject's tvr

(In reply to comment #6)
> Created an attachment (id=276454) [details]
> js_NewObject's tvr
> 
> (In reply to comment #3)
> > jsobj.cpp:js_NewObject : they protect
> > variables allocated via new or JS_malloc, and therefore, MMGc does not hold a
> > reference on them
> Removing tvr for JSObject is fine because JSObject inherits from
> JSFinalizedGCThing (overloaded new) which will be MMgc allocated.
> 

Thanks, I had not noticed that.

About removing temporary rooting in modules/plugin/base/src, I am not self-confident enough: after removing tvr in js/src, I run the jsDriver.pl tests, and when something goes wrong, I just revert. I don't known how to run equivalent tests in modules/plugin directory.

I also don't known how to convert code to make vec use MMgc instead of JS_malloc (in jsarray.cpp:array_sort)

So, I currently cannot remove JSTempValueRooter.u.array

Comment 9

[Jason Orendorff [:jorendorff]]

(In reply to comment #7)
> (In reply to comment #6)
> > modules/plugin/base/src/nsJSNPRuntime.cpp
> > modules/plugin/base/src/ns4xPlugin.cpp
> 
> If you really want to get rid of JSAutoTempValueRooter, couldn't you convert
> the code in question over to using straight jsvals on the C stack, and let
> MMgc's conservative collector do its thing?

Yes!  Please, let's get rid of this nonsense:

--- modules/plugin/base/src/nsJSNPRuntime.cpp
- // FIXME(bug 332648): Give me a real API please!
- #include "jscntxt.h"

(for arno's benefit) Code that uses SpiderMonkey should only #include "jsapi.h", not SpiderMonkey's other headers, like "jscntxt.h", which are considered internal.

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 276461 [details] [diff] [review]
removes also  js_NewObject's tvr  

OK, let's try to take care of the easy cases first.  Ed, if you have time, could you review this patch carefully and commit it if appropriate?

We'll leave the bug open for the few remaining cases.

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: same as arno's patch, unbitrotted

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: v2

I discovered two things to watch out for.

First, if a function takes a jsval* parameter, it might point to a variable that is not on the stack or otherwise reachable by GC.  So it has to be treated as unreachable; the easiest way is to use a local variable and push the result out to the jsval* at the last minute before returning.

Second, scope properties aren't GC'd yet, so JS_PUSH_TEMP_ROOT_SPROP has to stay, for now.

I also found a few places where control flow could be simplified more.

Still haven't tried running the tests under JS_GC_ZEAL (yeah, I know, but if it catches GC hazards, this is a good time to run it).

Comment 13

[Jason Orendorff [:jorendorff]]

Attachment: v3 - for checkin

OK, here's the baseline.  Tests pass.  Igor, could you take a quick look?  I'd like to go ahead and check this part in.

In separate patches, I'll implement Mardak's suggestion for removing the array root in jsarray.cpp and delete obsolete parts of TempValueRooter.

Comment 14

[Igor Bukanov]

Comment on attachment 281323 [details] [diff] [review]
v3 - for checkin

>diff --git a/js/src/jsarray.cpp b/js/src/jsarray.cpp
>--- a/js/src/jsarray.cpp
>+++ b/js/src/jsarray.cpp
>@@ -159,37 +159,35 @@ ValueIsLength(JSContext *cx, jsval v, js
>         return JS_FALSE;
>     }
>     return JS_TRUE;
> }
> 
> JSBool
> js_GetLengthProperty(JSContext *cx, JSObject *obj, jsuint *lengthp)
> {
>-    JSTempValueRooter tvr;
>+    jsval value = JSVAL_NULL;

Nit: use "v", not a "value", for jsval locals. In general SM uses short names for locals and parameters while using the full word for field names.
 
Non-nit: no need to set the value to null. tvr required that since the GC can not deal with a junk in the fields, but MMgc should know how to handle that. 

>     jsid id;
>     JSBool ok;
>     jsint i;
> 
>-    JS_PUSH_SINGLE_TEMP_ROOT(cx, JSVAL_NULL, &tvr);
>     id = ATOM_TO_JSID(cx->runtime->atomState.lengthAtom);
>-    ok = OBJ_GET_PROPERTY(cx, obj, id, &tvr.u.value);
>+    ok = OBJ_GET_PROPERTY(cx, obj, id, &value);
>     if (ok) {
>         /*
>          * Short-circuit, because js_ValueToECMAUint32 fails when called
>          * during init time.
>          */
>-        if (JSVAL_IS_INT(tvr.u.value)) {
>-            i = JSVAL_TO_INT(tvr.u.value);
>+        if (JSVAL_IS_INT(value)) {
>+            i = JSVAL_TO_INT(value);
>             *lengthp = (jsuint)i;       /* jsuint cast does ToUint32 */
>         } else {
>-            ok = js_ValueToECMAUint32(cx, tvr.u.value, (uint32 *)lengthp);
>-        }
>-    }
>-    JS_POP_TEMP_ROOT(cx, &tvr);
>+            ok = js_ValueToECMAUint32(cx, value, (uint32 *)lengthp);
>+        }
>+    }
>     return ok;
> }

Nit: Here ok is no longer necessary and return-on-failure can be used instead.

>@@ -343,28 +341,26 @@ js_SetLengthProperty(JSContext *cx, JSOb
> JSBool
> js_HasLengthProperty(JSContext *cx, JSObject *obj, jsuint *lengthp)
> {
>     JSErrorReporter older;
>-    JSTempValueRooter tvr;
>+    jsval value = JSVAL_NULL;
>     jsid id;
>     JSBool ok;
> 
>     older = JS_SetErrorReporter(cx, NULL);
>-    JS_PUSH_SINGLE_TEMP_ROOT(cx, JSVAL_NULL, &tvr);
>     id = ATOM_TO_JSID(cx->runtime->atomState.lengthAtom);
>-    ok = OBJ_GET_PROPERTY(cx, obj, id, &tvr.u.value);
>+    ok = OBJ_GET_PROPERTY(cx, obj, id, &value);
>     JS_SetErrorReporter(cx, older);
>     if (ok)
>-        ok = ValueIsLength(cx, tvr.u.value, lengthp);
>-    JS_POP_TEMP_ROOT(cx, &tvr);
>+        ok = ValueIsLength(cx, value, lengthp);
>     return ok;
> }

Nit: The same as above: value->v without initialization and ok elimination.

>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
>@@ -605,84 +593,67 @@ js_ComputeThis(JSContext *cx, jsval *arg
> 
> static JSBool
> NoSuchMethod(JSContext *cx, JSStackFrame *fp, jsval *vp, uint32 flags,
>              uintN argc)
> {
>     JSObject *thisp, *argsobj;
>     JSAtom *atom;
>     jsval *sp, roots[3];
>-    JSTempValueRooter tvr;
>     jsid id;
>-    JSBool ok;
>     jsbytecode *pc;

Replace here roots[3] by argv[2] and fval and patch the code accordingly.

>diff --git a/js/src/jsregexp.cpp b/js/src/jsregexp.cpp
>--- a/js/src/jsregexp.cpp
>+++ b/js/src/jsregexp.cpp
>@@ -4252,33 +4252,30 @@ bad:
> 
> JSObject *
> js_NewRegExpObject(JSContext *cx, JSTokenStream *ts,
>                    jschar *chars, size_t length, uintN flags)
> {
...
>     if (!obj || !JS_SetPrivate(cx, obj, re)) {
>         js_DestroyRegExp(cx, re);
>         obj = NULL;
>     }
>     if (obj && !js_SetLastIndex(cx, obj, 0))
>         obj = NULL;
>-    JS_POP_TEMP_ROOT(cx, &tvr);
>     return obj;
> }

Nit: Replace "obj = NULL" by "return NULL" here.  

>diff --git a/js/src/jsstr.cpp b/js/src/jsstr.cpp
>--- a/js/src/jsstr.cpp
>+++ b/js/src/jsstr.cpp
>@@ -2761,43 +2761,42 @@ js_ValueToString(JSContext *cx, jsval v)
>         str = ATOM_TO_STRING(cx->runtime->atomState.typeAtoms[JSTYPE_VOID]);
>     }
>     return str;
> }
> 
> JS_FRIEND_API(JSString *)
> js_ValueToSource(JSContext *cx, jsval v)
> {
>-    JSTempValueRooter tvr;
>     JSString *str;
>+    jsval value;

Nit: s/value/v

...
>-    JS_PUSH_SINGLE_TEMP_ROOT(cx, JSVAL_NULL, &tvr);
>+    value = JSVAL_NULL;

Again, no need to clear jsval.

>     if (!js_TryMethod(cx, JSVAL_TO_OBJECT(v),
>                       cx->runtime->atomState.toSourceAtom,
>-                      0, NULL, &tvr.u.value)) {
>+                      0, NULL, &value)) {
>         str = NULL;

Nit: Use early return here.

>     } else {
>-        str = js_ValueToString(cx, tvr.u.value);
>-    }
>-    JS_POP_TEMP_ROOT(cx, &tvr);
>+        str = js_ValueToString(cx, value);
>+    }
>     return str;
> }
> 

>diff --git a/js/src/jsxml.cpp b/js/src/jsxml.cpp
>--- a/js/src/jsxml.cpp
>+++ b/js/src/jsxml.cpp
>@@ -3124,19 +3123,17 @@ ToAttributeName(JSContext *cx, jsval v)
>             uri = prefix = cx->runtime->emptyString;
>         }
>     }
> 
>     qn = js_NewXMLQName(cx, uri, prefix, name);
>     if (!qn)
>         return NULL;
> 
>-    JS_PUSH_TEMP_ROOT_GCTHING(cx, qn, &tvr);
>     obj = js_GetAttributeNameObject(cx, qn);
>-    JS_POP_TEMP_ROOT(cx, &tvr);
>     if (!obj)
>         return NULL;
>     return qn;
> }

Nit: use "return obj ? qn : NULL; 

> static JSXML *
> CopyOnWrite(JSContext *cx, JSXML *xml, JSObject *obj)
> {
>     JS_ASSERT(xml->object != obj);
> 
>@@ -4182,28 +4171,37 @@ static JSBool
> static JSBool
> ResolveValue(JSContext *cx, JSXML *list, JSXML **result);
> 
> /* ECMA-357 9.1.1.2 XML [[Put]] and 9.2.1.2 XMLList [[Put]]. */
> static JSBool
> PutProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
> {
>     JSBool ok, primitiveAssign;
>-    enum { OBJ_ROOT, ID_ROOT, VAL_ROOT };
>-    jsval roots[3];
>-    JSTempValueRooter tvr;
>     JSXML *xml, *vxml, *rxml, *kid, *attr, *parent, *copy, *kid2, *match;
>     JSObject *vobj, *nameobj, *attrobj, *parentobj, *kidobj, *copyobj;
>     JSXMLQName *targetprop, *nameqn, *attrqn;
>     uint32 index, i, j, k, n, q, matchIndex;
>     jsval attrval, nsval;
>     jsid funid;
>     JSString *left, *right, *space;
>     JSXMLNamespace *ns;
> 
>+    /*
>+     * We can't be sure vp points to memory that is reachable by the GC, so
>+     * here we root the contents of *vp in a stack variable, |valroot|.
>+     * |volatile| is to prevent the compiler from optimizing it away (we never
>+     * actually use the value).
>+     *
>+     * XXX Perhaps SpiderMonkey should be changed so that PropertyOps can
>+     * assume that vp always points to a variable on the stack.  That would
>+     * eliminate the need for this weirdness.
>+     */
>+    volatile jsval valroot;

This is not good. For now I suggest to keep tvr to use it to root vp avoiding on volatile assumptions. Plus tvr will allow to grep for problems with MMgc integration.

>@@ -4939,42 +4931,41 @@ HasSimpleContent(JSXML *xml);
> HasSimpleContent(JSXML *xml);
> 
> static JSBool
> HasFunctionProperty(JSContext *cx, JSObject *obj, jsid funid, JSBool *found)
> {
>     JSObject *pobj;
>     JSProperty *prop;
>     JSXML *xml;
>-    JSTempValueRooter tvr;
>+    JSObject *object;

Nit: s/object/obj

>     JSBool ok;
> 
>     JS_ASSERT(OBJ_GET_CLASS(cx, obj) == &js_XMLClass);
> 
>     if (!js_LookupProperty(cx, obj, funid, &pobj, &prop))
>         return JS_FALSE;
>     if (prop) {
>         OBJ_DROP_PROPERTY(cx, pobj, prop);
>     } else {
>         xml = (JSXML *) JS_GetPrivate(cx, obj);
>         if (HasSimpleContent(xml)) {
>             /*
>              * Search in String.prototype to set found whenever
>              * js_GetXMLFunction returns existing function.
>              */
>-            JS_PUSH_TEMP_ROOT_OBJECT(cx, NULL, &tvr);
>+            object = NULL;
>             ok = js_GetClassPrototype(cx, NULL, INT_TO_JSID(JSProto_String),
>-                                      &tvr.u.object);
>-            JS_ASSERT(tvr.u.object);
>+                                      &object);
>+            JS_ASSERT(object);

Nit: use if (!js_GetClassPrototype()) return NULL here.



> static JSObject *
> xml_getMethod(JSContext *cx, JSObject *obj, jsid id, jsval *vp)
> {
>-    JSTempValueRooter tvr;
>+    jsval value = JSVAL_NULL;

No need to set value to null.

> 
>     JS_ASSERT(JS_InstanceOf(cx, obj, &js_XMLClass, NULL));
> 
>-    /*
>-     * As our callers have a bad habit of passing a pointer to an unrooted
>-     * local value as vp, we use a proper root here.
>-     */
>-    JS_PUSH_SINGLE_TEMP_ROOT(cx, JSVAL_NULL, &tvr);
>-    if (!js_GetXMLFunction(cx, obj, id, &tvr.u.value))
>+    if (!js_GetXMLFunction(cx, obj, id, &value))
>         obj = NULL;

Nit: use early return.

>@@ -5721,27 +5706,24 @@ xml_attribute(JSContext *cx, uintN argc,
> static JSBool
> xml_attributes(JSContext *cx, uintN argc, jsval *vp)
> {
>     jsval name;
>     JSXMLQName *qn;
>-    JSTempValueRooter tvr;
>     JSBool ok;
> 
>     name = ATOM_KEY(cx->runtime->atomState.starAtom);
>     qn = ToAttributeName(cx, name);
>     if (!qn)
>         return JS_FALSE;
>     name = OBJECT_TO_JSVAL(qn->object);
>-    JS_PUSH_SINGLE_TEMP_ROOT(cx, name, &tvr);
>     ok = GetProperty(cx, JS_THIS_OBJECT(cx, vp), name, vp);
>-    JS_POP_TEMP_ROOT(cx, &tvr);
>     return ok;
> }

Nit: use return GetProperty(cx, JS_THIS_OBJECT(cx, vp), name, vp) eliminating ok.


> JSObject *
> js_NewXMLObject(JSContext *cx, JSXMLClass xml_class)
> {
>     JSXML *xml;
>     JSObject *obj;
>-    JSTempValueRooter tvr;
> 
>     xml = js_NewXML(cx, xml_class);
>     if (!xml)
>         return NULL;
>-    JS_PUSH_TEMP_ROOT_GCTHING(cx, xml, &tvr);
>     obj = js_GetXMLObject(cx, xml);
>-    JS_POP_TEMP_ROOT(cx, &tvr);
>     return obj;
> }

Nit: use return js_GetXMLObject(cx, xml)

> 
> static JSObject *
> NewXMLObject(JSContext *cx, JSXML *xml)
> {
>     JSObject *obj;
> 
>@@ -8107,56 +8086,47 @@ js_FindXMLProperty(JSContext *cx, jsval 
>     return JS_FALSE;
> }
> 
> JSBool
> js_GetXMLFunction(JSContext *cx, JSObject *obj, jsid id, jsval *vp)
> {
>     JSObject *target;
>     JSXML *xml;
>-    JSTempValueRooter tvr;
>-    JSBool ok;
>+    JSObject *object = NULL;
>+    JSBool ok = JS_TRUE;
> 

Nit: initialize object and ok before the loop.

Comment 15

[Ed Lee :Mardak]

Comment on attachment 281323 [details] [diff] [review]
v3 - for checkin

(In reply to comment #12)
> First, if a function takes a jsval* parameter, it might point to a variable
> that is not on the stack or otherwise reachable by GC.  So it has to be treated
> as unreachable; the easiest way is to use a local variable and push the result
> out to the jsval* at the last minute before returning.

Are you referring in particular to a case like this?

> JSObject *
> js_ConstructObject(JSContext *cx, JSClass *clasp, JSObject *proto,
>                    JSObject *parent, uintN argc, jsval *argv)
> {
>     jsid id;
>     jsval cval, rval;
>-    JSTempValueRooter argtvr, tvr;
>+    JSTempValueRooter argtvr;
>     JSObject *obj, *ctor;
> 
>     JS_PUSH_TEMP_ROOT(cx, argc, argv, &argtvr);

Will that be a similar issue with array_sort? The jsval * that it's pushing is a plain JS_malloc'd array that isn't managed by the GC.

Comment 16

[Jason Orendorff [:jorendorff]]

Attachment: v4 - addressing igor's comments

Comment 17

[Jason Orendorff [:jorendorff]]

Attachment: v3 to v4

This patch is described by the equation

  v3 + this = v4          (for suitable definitions of + and =)

I know the usual thing is to diff the new patch against the previous one, but I think this might be more readable, given the nature of the changes.

Igor, in a few places s/value/v/ or s/object/obj/ wouldn't work because there is already some other v or obj in the function; so in those cases I tried to choose more descriptive names.

Comment 18

[Ed Lee :Mardak]

(In reply to comment #17)
>   v3 + this = v4          (for suitable definitions of + and =)
You can do something like "hg export rm-tvr-v3.patch:rm-tvr-v3-to-v4.diff > v4.patch" which will just concatenate the 2 patches together. (Or even fancier.. hg export qbase:qtip, etc.) Useful for keeping logically changes separate but in a single file patch. (This isn't quite the same as qfold which merges two patches.)

One downside is that bugzilla's interdiff totally fails because the "one" patch patches the same file multiple times. But I suppose if all incremental/review changes are always in a qnew'd patch, the reviewer can just look at the bottom-most patch.

Comment 19

[Igor Bukanov]

Comment on attachment 281340 [details] [diff] [review]
v3 to v4

Thanks for the explicit interdiff, it is so much easy to do comments on comments!

>diff --git a/js/src/jsarray.cpp b/js/src/jsarray.cpp
>--- a/js/src/jsarray.cpp
>+++ b/js/src/jsarray.cpp
>@@ -164,26 +164,25 @@ JSBool
> JSBool
> js_GetLengthProperty(JSContext *cx, JSObject *obj, jsuint *lengthp)
> {
...
>+    /*
>+     * Short-circuit, because js_ValueToECMAUint32 fails when called
>+     * during init time.
>+     */
>+    if (JSVAL_IS_INT(v)) {
>+        i = JSVAL_TO_INT(v);
>+        *lengthp = (jsuint)i;       /* jsuint cast does ToUint32 */
>+        return JS_TRUE;
>+    } else {
>+        return js_ValueToECMAUint32(cx, v, (uint32 *)lengthp);
>+    }
> }

Nit: avoid " return ; else " pattern, that is, drop "else" here. 

> 
> static JSBool
> JSBool
>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
>--- a/js/src/jsinterp.cpp
>+++ b/js/src/jsinterp.cpp
>@@ -597,7 +597,7 @@ NoSuchMethod(JSContext *cx, JSStackFrame
> {
>     JSObject *thisp, *argsobj;
>     JSAtom *atom;
>-    jsval *sp, roots[3];
>+    jsval *sp, fun, argv[2];
>     jsid id;
>     jsbytecode *pc;

Nit: s/fun/fval/. "fun" is reserved in SM for JSFunction* locals. 

>diff --git a/js/src/jsstr.cpp b/js/src/jsstr.cpp
>--- a/js/src/jsstr.cpp
>+++ b/js/src/jsstr.cpp
>@@ -2767,7 +2767,7 @@ js_ValueToSource(JSContext *cx, jsval v)
> js_ValueToSource(JSContext *cx, jsval v)
> {
>     JSString *str;
>-    jsval value;
>+    jsval src;

I think srcval would be better here.

> 
>     if (JSVAL_IS_VOID(v))
>         return ATOM_TO_STRING(cx->runtime->atomState.void0Atom);
>@@ -2784,15 +2784,11 @@ js_ValueToSource(JSContext *cx, jsval v)
>         return js_ValueToString(cx, v);
>     }
> 
>-    value = JSVAL_NULL;
>     if (!js_TryMethod(cx, JSVAL_TO_OBJECT(v),
>                       cx->runtime->atomState.toSourceAtom,
>-                      0, NULL, &value)) {
>-        str = NULL;
>-    } else {
>-        str = js_ValueToString(cx, value);
>-    }
>-    return str;
>+                      0, NULL, &src))
>+        return NULL;
>+    return js_ValueToString(cx, src);
> }

Minor nit: chain the methods on return like in: 

return  js_TryMethod(...
                     ...) &&
        js_ValueToString(cx, srcval);


>@@ -5338,13 +5333,13 @@ static JSObject *
> static JSObject *
> xml_getMethod(JSContext *cx, JSObject *obj, jsid id, jsval *vp)
> {
>-    jsval value = JSVAL_NULL;
>+    jsval fun;

Nit: s/fun/fval/

Comment 20

[Igor Bukanov]

(In reply to comment #19)
> >-    value = JSVAL_NULL;
> >     if (!js_TryMethod(cx, JSVAL_TO_OBJECT(v),
> >                       cx->runtime->atomState.toSourceAtom,
> >-                      0, NULL, &value)) {
> >-        str = NULL;
> >-    } else {
> >-        str = js_ValueToString(cx, value);
> >-    }
> >-    return str;
> >+                      0, NULL, &src))
> >+        return NULL;
> >+    return js_ValueToString(cx, src);
> > }
> 
> Minor nit: chain the methods on return like in: 

Ignore this part, I am wrong here as chaining is not possible.

Comment 21

[Jason Orendorff [:jorendorff]]

Attachment: v5

Comment 22

[Jason Orendorff [:jorendorff]]

Attachment: v4 to v5 incremental patch

v5 addresses Igor's comments on v4.

It also addresses the volatile local "valroot" in jsxml:PutProperty.  I should have addressed this in v4, but I forgot.

My new approach is to copy *pv into a local (named |v|) and manipulate that instead of manipulating *pv in place.  Then store the result back into *pv only on success.  This way, there's no volatile local, and no TempValueRooter either.

(This is a change in behavior only if the function fails, in which case the caller is supposed to treat *pv as undefined anyway.)

Comment 23

[Igor Bukanov]

Comment on attachment 281346 [details] [diff] [review]
v5

Right, no need for tvr in PutProperty from jsxml.c, copy to rooted local is the ticket.

Comment 24

[Jason Orendorff [:jorendorff]]

Attachment: arraysort v1 (remove another TempValueRooter)

Implements Mardak's idea.

(JSTempValueRooter.u.array can't be removed yet; it's still being used in js_ConstructObject, to root things that might not otherwise be reachable.)

Comment 25

[Jason Orendorff [:jorendorff]]

Attachment: tvr-support v1 (remove support for some TempValueRooter flavors)

This patch removes support for the TempValueRooter flavors that we've eradicated.  It removes a few remaining stack TempValueRooters from source files in  modules/plugin/base/src.

(This patch can be applied with or without rm-tvr-arraysort-v1.patch.)

Comment 26

[Igor Bukanov]

Comment on attachment 281477 [details] [diff] [review]
arraysort v1 (remove another TempValueRooter)

>diff --git a/js/src/jsarray.cpp b/js/src/jsarray.cpp
>--- a/js/src/jsarray.cpp
>+++ b/js/src/jsarray.cpp
>@@ -1081,17 +1081,16 @@ JS_STATIC_ASSERT(JSVAL_NULL == 0);
>     /*
>      * Allocate 2 * len instead of len, to reserve space for the mergesort
>      * algorithm.
>      */
>-    vec = (jsval *) JS_malloc(cx, 2 * (size_t)len * sizeof(jsval));
>+    vec = (jsval *) cx->runtime->gc->Alloc(2 * (size_t)len * sizeof(jsval),
>+                                           MMgc::GC::kZero
>+                                           | MMgc::GC::kContainsPointers);

I am not sure that it is a good idea to use cx->runtime->gc->Alloc here. What happens with the patch when you try to run in a browser code like:

Array(1 << 26).sort() ?

Or like

var a = Array(1 << 26); a.__defineGetter__(0, gc()); a.sort()?

I think if MMgc does not provide a way to root temporary huge arrays, then it should be extended with that. Until it is done, lets keep tvr here.

Comment 27

[Igor Bukanov]

(In reply to comment #26)
> 
> Array(1 << 26).sort() ?
> 
> Or like
> 
> var a = Array(1 << 26); a.__defineGetter__(0, gc()); a.sort()?

The point here is that Array(1 << 26) produces an array with length equals to 1 << 26 and no elements. Now array.sort implementation does not implement a special code for spare arrays and always mallocs the buffer to hold length elements. But there is no denial-of-service in the code since the allocated buffer is only populated with elements that exists. Thus with a reasonable virtual memory implementation the allocated memory will never be committed and Array(1 << 26) will be stopped by the branch callback.

This would not be the case with MMgc as it will try to zero all the memory and scan it for garbage with the second example. To fix this one has to either implement an efficient soring of spare arrays or extend MMgc with special allocated but not populated arrays. Until such fix is written I would suggest to keep tvr here as a teaser.

Comment 28

[Jason Orendorff [:jorendorff]]

Comment on attachment 281477 [details] [diff] [review]
arraysort v1 (remove another TempValueRooter)

Yep, good catch.  I don't see a better way to do this.

(In response to your comment about MMgc and temporary roots:  I have hacked a custom marking mechanism into MMgc that could be used here, but that's probably worse than a TempValueRooter.)

Comment 29

[Jason Orendorff [:jorendorff]]

Pushed changesets df49d75fad01 (attachment 281346 [details] [diff] [review]) and 51968967fc2a (attachment 281479 [details] [diff] [review]) to actionmonkey.