397041 - ActionMonkey: Remove unnecessary local root scopes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

With MMgc, I think most places where we use local scope roots (internally, in js/src) can be simplified.

Removing local scope roots is trickier than removing TempValueRooters or extra locals, because it's not clear exactly which objects are protected, and the scope of a local scope root is dynamic, not lexical.

Patch coming in a little while.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

The only place we used local root scopes ourselves was in jsxml.c.

It took a long time to convince myself this doesn't introduce any GC hazards.  It is still very hard to be more than about 99% sure, but I pored over all this code for a long time and I believe it's safe.

In addition to removing the calls to js_EnterLocalRootScope and js_LeaveRootScope{,WithResult}, I deleted a few places where a value was rooted by assigning it to vp[2] or some such.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 281877 [details] [diff] [review]
v1

>@@ -4194,37 +4132,35 @@ PutProperty(JSContext *cx, JSObject *obj

>         /* 2(a-b). */
>         if (xml->xml_target) {
>             ok = ResolveValue(cx, xml->xml_target, &rxml);
>             if (!ok)
>-                goto out;
>+                return JS_FALSE;

No ok needed here (and based on non-AM jsxml.c, ok doesn't seem to be necessary in PutProperty except in one case where an XMLArrayCursor is being used).


>@@ -4234,17 +4170,17 @@ PutProperty(JSContext *cx, JSObject *obj

>                     ok = js_GetXMLObject(cx, rxml) != NULL;
>                     if (!ok)
>-                        goto out;
>+                        return JS_FALSE;

Or here.


>@@ -4262,44 +4198,44 @@ PutProperty(JSContext *cx, JSObject *obj

>                 if (OBJ_GET_CLASS(cx, nameobj) == &js_AttributeNameClass) {
>                     /*
>                      * 2(c)(iii)(1-3).
>                      * Note that rxml can't be null here, because target
>                      * and targetprop are non-null.
>                      */
>                     ok = GetProperty(cx, rxml->object, id, &attrval);
>                     if (!ok)
>-                        goto out;
>+                        return JS_FALSE;

Again.


>@@ -4322,20 +4258,20 @@ PutProperty(JSContext *cx, JSObject *obj

>                     ok = Insert(cx, rxml, j + 1, OBJECT_TO_JSVAL(kidobj));
>                     if (!ok)
>-                        goto out;
>+                        return JS_FALSE;

Agasin.


>@@ -4343,52 +4279,52 @@ PutProperty(JSContext *cx, JSObject *obj

>             /* 2(c)(viii). */
>             ok = Append(cx, xml, kid);
>             if (!ok)
>-                goto out;
>+                return JS_FALSE;

Again.


>             ok = JS_ConvertValue(cx, v, JSTYPE_STRING, &v);
>             if (!ok)
>-                goto out;
>+                return JS_FALSE;

Again.


>                 /* 2(e)(i). */
>                 parentobj = js_GetXMLObject(cx, parent);
>                 if (!parentobj)
>-                    goto bad;
>+                    return JS_FALSE;
>                 ok = PutProperty(cx, parentobj, id, &v);
>                 if (!ok)
>-                    goto out;
>+                    return JS_FALSE;

Again.


>@@ -4397,30 +4333,30 @@ PutProperty(JSContext *cx, JSObject *obj

>             JS_ASSERT(parent != xml);
>             if (parent) {
>                 q = XMLARRAY_FIND_MEMBER(&parent->xml_kids, kid, NULL);
>                 JS_ASSERT(q != XML_NOT_FOUND);
>                 ok = Replace(cx, parent, q, OBJECT_TO_JSVAL(copyobj));
>                 if (!ok)
>-                    goto out;
>+                    return JS_FALSE;

Again.


>@@ -4433,31 +4369,31 @@ PutProperty(JSContext *cx, JSObject *obj

>             } else {
>                 ok = XMLArrayInsert(cx, &xml->xml_kids, i + 1, n - 1);
>                 if (!ok)
>-                    goto out;
>+                    return JS_FALSE;

Again.


>         /* 2(g). */
>         else if (vxml || JSXML_HAS_VALUE(kid)) {
>             if (parent) {
>                 q = XMLARRAY_FIND_MEMBER(&parent->xml_kids, kid, NULL);
>                 JS_ASSERT(q != XML_NOT_FOUND);
>                 ok = Replace(cx, parent, q, v);
>                 if (!ok)
>-                    goto out;
>+                    return JS_FALSE;

Again.


>@@ -4469,40 +4405,40 @@ PutProperty(JSContext *cx, JSObject *obj

>         /* 2(h). */
>         else {
>             kidobj = js_GetXMLObject(cx, kid);
>             if (!kidobj)
>-                goto bad;
>+                return JS_FALSE;
>             id = ATOM_KEY(cx->runtime->atomState.starAtom);
>             ok = PutProperty(cx, kidobj, id, &v);
>             if (!ok)
>-                goto out;
>+                return JS_FALSE;

Again.


>@@ -4511,31 +4447,31 @@ PutProperty(JSContext *cx, JSObject *obj

>             if (n == 0) {
>                 ok = ResolveValue(cx, xml, &rxml);
>                 if (!ok)
>-                    goto out;
>+                    return JS_FALSE;

Again.


>                 if (!rxml || JSXML_LENGTH(rxml) != 1)
>                     goto type_error;
>                 ok = Append(cx, xml, rxml);
>                 if (!ok)
>-                    goto out;
>+                    return JS_FALSE;

Again.


>@@ -4544,67 +4480,67 @@ PutProperty(JSContext *cx, JSObject *obj

>         if (!vxml ||
>             vxml->xml_class == JSXML_CLASS_TEXT ||
>             vxml->xml_class == JSXML_CLASS_ATTRIBUTE) {
>             ok = JS_ConvertValue(cx, v, JSTYPE_STRING, &v);
>             if (!ok)
>-                goto out;
>+                return JS_FALSE;

Again.


>         /*
>          * 6.
>          * Erratum: why is this done here, so early? use is way later....
>          */
>         ok = js_GetDefaultXMLNamespace(cx, &nsval);
>         if (!ok)
>-            goto out;
>+            return JS_FALSE;

Again.


>                 ok = JS_ConvertValue(cx, v, JSTYPE_STRING, &v);
>                 if (!ok)
>-                    goto out;
>+                    return JS_FALSE;

Again.


>@@ -4628,37 +4564,37 @@ PutProperty(JSContext *cx, JSObject *obj

>                 /* 7(f)(iv). */
>                 ok = XMLARRAY_ADD_MEMBER(cx, &xml->xml_attrs, n, attr);
>                 if (!ok)
>-                    goto out;
>+                    return JS_FALSE;

Again.


>                 /* 7(f)(v-vi). */
>                 ns = GetNamespace(cx, nameqn, NULL);
>                 if (!ns)
>-                    goto bad;
>+                    return JS_FALSE;
>                 ok = AddInScopeNamespace(cx, xml, ns);
>                 if (!ok)
>-                    goto out;
>+                    return JS_FALSE;

Again.


>@@ -4721,36 +4657,36 @@ PutProperty(JSContext *cx, JSObject *obj

>                 /* 13(b)(iv-vi). */
>                 ns = GetNamespace(cx, nameqn, NULL);
>                 if (!ns)
>-                    goto bad;
>+                    return JS_FALSE;
>                 ok = Replace(cx, xml, matchIndex, OBJECT_TO_JSVAL(vobj));
>                 if (!ok)
>-                    goto out;
>+                    return JS_FALSE;

Again.


>                 ok = AddInScopeNamespace(cx, vxml, ns);
>                 if (!ok)
>-                    goto out;
>+                    return JS_FALSE;

Again.


>         /* 14. */
>         if (primitiveAssign) {
>             JSXMLArrayCursor cursor;
> 
>             XMLArrayCursorInit(&cursor, &xml->xml_kids);

This is the only place where ok is necessary, and I think (?) you could easily get rid of that if you converted XMLArrayCursor to a scoped constructor-destructor object (I don't believe it's used any other way -- indeed I hope not, for sanity -- but I could be wrong).  Perhaps a followup...


>@@ -4775,39 +4711,33 @@ PutProperty(JSContext *cx, JSObject *obj
>             /* 15(a). */
>             ok = Replace(cx, xml, matchIndex, v);
>         }
>     }
> 
> out:
>     if (ok)
>         *vp = v;
>-    js_LeaveLocalRootScope(cx);
>     return ok;

With that, I think ok can be limited just to 14, so this is just an unconditional assignment.


>@@ -5418,102 +5348,83 @@ xml_equality(JSContext *cx, JSObject *ob

I'm not convinced ok is needed here, but the way this got diffed makes my head hurt, so perhaps best to follow up after this, when there's less noise here.  A -w diff might have been useful here...


>@@ -5970,46 +5866,41 @@ xml_elements_helper(JSContext *cx, JSObj
>     list->xml_targetprop = nameqn;
>     ok = JS_TRUE;
> 
>     if (xml->xml_class == JSXML_CLASS_LIST) {
>         /* 13.5.4.6 */
>         XMLArrayCursorInit(&cursor, &xml->xml_kids);
>         while ((kid = (JSXML *) XMLArrayCursorNext(&cursor)) != NULL) {
>             if (kid->xml_class == JSXML_CLASS_ELEMENT) {
>-                ok = js_EnterLocalRootScope(cx);
>-                if (!ok)
>-                    break;
>                 kidobj = js_GetXMLObject(cx, kid);
>                 if (kidobj) {
>                     ok = xml_elements_helper(cx, kidobj, kid, nameqn, &v);
>                 } else {
>                     ok = JS_FALSE;
>                     v = JSVAL_NULL;
>                 }
>-                js_LeaveLocalRootScopeWithResult(cx, v);
>                 if (!ok)
>                     break;
>                 vxml = (JSXML *) JS_GetPrivate(cx, JSVAL_TO_OBJECT(v));
>                 if (JSXML_LENGTH(vxml) != 0) {
>                     ok = Append(cx, list, vxml);
>                     if (!ok)
>                         break;
>                 }
>             }
>         }
>         XMLArrayCursorFinish(&cursor);

Another place where an XMLArray-with-dtor would be useful...``


>-                ok = Append(cx, list, kid);
>-                if (!ok)
>-                    break;
>+                if (!Append(cx, list, kid))
>+                    return JS_FALSE;
>             }
>         }
>     }
> 
>     return ok;

Again not convinced ok is needed except with the XMLArrayCursor, but probably better as followup...


>@@ -6626,19 +6511,18 @@ xml_processingInstructions_helper(JSCont
>         XMLArrayCursorFinish(&cursor);
>     } else {
>         /* 13.4.4.28 Step 4. */
>         for (i = 0, n = JSXML_LENGTH(xml); i < n; i++) {
>             kid = XMLARRAY_MEMBER(&xml->xml_kids, i, JSXML);
>             if (kid && kid->xml_class == JSXML_CLASS_PROCESSING_INSTRUCTION &&
>                 (IS_STAR(nameqn->localName) ||
>                  js_EqualStrings(nameqn->localName, kid->name->localName))) {
>-                ok = Append(cx, list, kid);
>-                if (!ok)
>-                    break;
>+                if (!Append(cx, list, kid))
>+                    return JS_FALSE;
>             }
>         }

ok isn't needed here because you don't have a live XMLArrayCursor.


>@@ -7091,34 +6964,31 @@ xml_toString_helper(JSContext *cx, JSXML

>     XMLArrayCursorInit(&cursor, &xml->xml_kids);
>     while ((kid = (JSXML *) XMLArrayCursorNext(&cursor)) != NULL) {
>         if (kid->xml_class != JSXML_CLASS_COMMENT &&
>             kid->xml_class != JSXML_CLASS_PROCESSING_INSTRUCTION) {
>             kidstr = xml_toString_helper(cx, kid);
>             if (!kidstr) {
>                 str = NULL;
>                 break;
>             }
>             str = js_ConcatStrings(cx, str, kidstr);
>             if (!str)
>                 break;
>         }
>     }
>     XMLArrayCursorFinish(&cursor);
>-    js_LeaveLocalRootScopeWithResult(cx, STRING_TO_JSVAL(str));
>     return str;

More XMLArrayCursor fodder...


> JSBool
> js_GetAnyName(JSContext *cx, jsval *vp)

-w diff here would have been good; I found it impossible to read except in Bugzilla's pretty-printed version.  I have to wonder, tho, why there was a do-while(0) loop there without any new stack variables, but I'm too lazy at the moment to consult blame to figure out who to ask. 


>@@ -8184,16 +8030,17 @@ js_FilterXMLList(JSContext *cx, JSObject
>     result = (JSXML *) JS_GetPrivate(cx, resobj);
> 
>     /* Hoist the scope chain update out of the loop over kids. */
>     withobj = js_NewWithObject(cx, NULL, scobj, -1);
>     if (!withobj)
>         goto bad;
>     fp->scopeChain = withobj;
> 
>+    ok = JS_TRUE;
>     XMLArrayCursorInit(&cursor, &list->xml_kids);
>     while ((kid = (JSXML *) XMLArrayCursorNext(&cursor)) != NULL) {
>         kidobj = js_GetXMLObject(cx, kid);
>         if (!kidobj)
>             break;
>         OBJ_SET_PROTO(cx, withobj, kidobj);
>         ok = js_Interpret(cx, pc, vp) && js_ValueToBoolean(cx, *vp, &match);
>         if (ok && match)

More XMLArrayCursor fodder...


I hadn't realized just how much conservative GC by way of stack scanning simplifies code.  Frankly, it astounds me that anyone writing a garbage collector would choose any other way given just how impressive a simplification of the code is possible.

Comment 3

[Brendan Eich [:brendan]]

> I hadn't realized just how much conservative GC by way of stack scanning
> simplifies code.  Frankly, it astounds me that anyone writing a garbage
> collector would choose any other way given just how impressive a simplification
> of the code is possible.

The counter-argument is that false positives break all promises to collect garbabe and finalize objects. Bill Joy in 1995 expressed this to me as "no dial tone" being the worst case due to false positives in GC underlying a hypothetical Java-based phone system. David Ungar said to me more recently that conservative GC is not even "real GC".

The truth is not clean and simple. There are trade-offs. What you cite in favor of conservative GC (smaller and simpler code) is one benefit. Another is worry-free human factors: explicitly rooting in a native-heavy C world is just too hard to get right, even with better APIs.

On the other hand, if native code is rare and your optimizing compiling runtime takes care of managing the root set, exact GC could look better because the human is out of the loop, the code is all JITted to perfection, and the false positives are banished.

Based on the above, and our long experience with SpiderMonkey's exact GC, I agree that the conservative stack-scanning grass really is greener.

/be

Comment 4

[Igor Bukanov]

(In reply to comment #3)
> > I hadn't realized just how much conservative GC by way of stack scanning
> > simplifies code.  Frankly, it astounds me that anyone writing a garbage
> > collector would choose any other way given just how impressive a simplification
> > of the code is possible.
> 
> The counter-argument is that false positives break all promises to collect
> garbabe and finalize objects.

Another counter-argument against conservative GC is that is does not allow to use a copy GC for the native stack roots. That restricts the number of possible GC optimizations.
...
> Based on the above, and our long experience with SpiderMonkey's exact GC, I
> agree that the conservative stack-scanning grass really is greener.

For me leaks caused by a conservative GC are an insignificant evil compared with GC hazards. So unless a reliable tool will be written that statically ensures hazard-free code (see bug 353231), conservative GC wins hands down. 

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: v1 to v2 incremental patch

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: v2 - add Waldo's suggestions

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: v2, ignoring whitespace (diff -b)

Comment 8

[Jason Orendorff [:jorendorff]]

(In reply to comment #2)
> I hadn't realized just how much conservative GC by way of stack scanning
> simplifies code.  Frankly, it astounds me that anyone writing a garbage
> collector would choose any other way given just how impressive a simplification
> of the code is possible.

Yep.  Just wait-- C++ exceptions are an equally big win, only across the entire codebase.

The danger is that C++ exceptions might end up hurting performance in practice. In theory they shouldn't.  By contrast, conservative stack scanning could hurt performance in theory but probably won't in practice. :)

Comment 9

[Igor Bukanov]

(In reply to comment #8)
> By contrast, conservative stack scanning could hurt
> performance in theory but probably won't in practice. :)

As I wrote, a conservative GC prevents certain optimizations that a precise GC can make like compacting heap, generational copy collections etc. But you right that in (Spider|Action)Monkey reality it won't hurt.  

I will review the patch later today.

Comment 10

[Igor Bukanov]

Comment on attachment 282106 [details] [diff] [review]
v2 - add Waldo's suggestions

Waldo's suggestions cover everything :)

Comment 11

[Jason Orendorff [:jorendorff]]

Pushed changeset 754498fcb5e4 to actionmonkey.