jsgc cleanup for ActionMonkey stage 0. Should this remove JS_ClearNewbornRoots from the JS API? There's be some logic changes so that in the existing case of checking for weak roots, it'll do nothing instead.

Don't remove APIs, ever (exceptions would be APIs that were added recently, not shipped in a standalone SpiderMonkey release, and considered harmful; or anything so fatally flawed that we'd be better off removing it from jsapi.h). JS_ClearNewbornRoots should just become a stub. /be

There's some logic changes that basically do nothing after the patch if there was a weakRoot check. The comment for JS_ClearNewbornRoots in jsapi.h probably needs to change eventually.

Comment on attachment 272859 [details] [diff] [review] v1 It's too early for this change; it would break things if we do this before switching to MMgc.

Oops, I found a bug. New patch coming in a second.

Comment on attachment 281537 [details] [diff] [review] v2 >diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp > JS_PUBLIC_API(void) > JS_ClearNewbornRoots(JSContext *cx) > { >- JS_CLEAR_WEAK_ROOTS(&cx->weakRoots); > } Add obsolete comment here. >@@ -3805,17 +3803,16 @@ JS_NewPropertyIterator(JSContext *cx, JS > } > > /* iterobj can not escape to other threads here. */ > STOBJ_SET_SLOT(iterobj, JSSLOT_PRIVATE, PRIVATE_TO_JSVAL(pdata)); > STOBJ_SET_SLOT(iterobj, JSSLOT_ITER_INDEX, INT_TO_JSVAL(index)); > return iterobj; > > bad: >- cx->weakRoots.newborn[GCX_OBJECT] = NULL; > return NULL; > } Nit: replace "goto bad" by "return NULL". diff --git a/js/src/jsarray.cpp b/js/src/jsarray.cpp The patch removes the local roots usage but it has not updated array_methods to set the local root counters to 0. >diff --git a/js/src/jsexn.cpp b/js/src/jsexn.cpp >--- a/js/src/jsexn.cpp > static JSBool > exn_toSource(JSContext *cx, uintN argc, jsval *vp) Again, exception_methods is not updated to set the local root conter to 0. > /* Initialize the prototypes first. */ > for (i = 0; exceptions[i].name != 0; i++) { > JSAtom *atom; > JSFunction *fun; > JSObject *funobj; > JSString *nameString; > int protoIndex = exceptions[i].protoIndex; > >@@ -1069,17 +1062,16 @@ js_InitExceptionClasses(JSContext *cx, J > break; > } > > /* Finally, stash the constructor for later uses. */ > if (!js_SetClassObject(cx, obj, exceptions[i].key, funobj)) > break; > } > >- js_LeaveLocalRootScope(cx); > if (exceptions[i].name) > return NULL; Nit: Replace break in the above code by "return NULL" >diff --git a/js/src/jsiter.cpp b/js/src/jsiter.cpp >@@ -799,17 +799,16 @@ js_NewGenerator(JSContext *cx, JSStackFr > > if (!JS_SetPrivate(cx, obj, gen)) { > JS_free(cx, gen); > goto bad; > } > return obj; > > bad: >- cx->weakRoots.newborn[GCX_OBJECT] = NULL; > return NULL; > } Nit: replace "goto bad" by "return NULL"

Comment on attachment 281664 [details] [diff] [review] v3 to v4 incremental patch >@@ -1974,27 +1972,27 @@ static JSFunctionSpec array_methods[] = .... >- JS_FN("reverse", array_reverse, 0,0,JSFUN_GENERIC_NATIVE,2), >- JS_FN("sort", array_sort, 0,1,JSFUN_GENERIC_NATIVE,1), >+ JS_FN("reverse", array_reverse, 0,0,JSFUN_GENERIC_NATIVE,0), >+ JS_FN("sort", array_sort, 0,1,JSFUN_GENERIC_NATIVE,0), Are there any JS_FN left after the patch that needs local roots after the patch? >diff --git a/js/src/jsexn.cpp b/js/src/jsexn.cpp >@@ -1021,55 +1021,55 @@ js_InitExceptionClasses(JSContext *cx, J ... > if (exceptions[i].name) > return NULL; After the changes exceptions[i].name is always null here. r+ with the lines removed.

(In reply to comment #10) > Are there any JS_FN left after the patch that needs local roots after the > patch? No. I didn't remove the last argument from JS_FN(), though, because it's in jsapi.h.

v5 is just v4 plus the two-line change Igor suggested.

pushed to actionmonkey branch - changeset 3ae3132cd872.

JS_FN is new enough that it has not shipped in a standalone SpiderMonkey release or a XULRunner embedding release, so we could change it for Mozilla 2 and make the CVS version be a non-frozen prototype API. JS API compatibility will be a "mostly" thing, with some changes (some opt-in via #ifdefs, I think, but others that are "small enough" just incompatible changes). Comments? /be