Closed Bug 388622 Opened 17 years ago Closed 17 years ago

ActionMonkey: Remove JS{Weak,Newborn}Roots

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: Mardak, Assigned: jorendorff)

References

Details

Attachments

(1 file, 5 obsolete files)

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
Summary: Remove JS{Weak,Newborn}Roots → ActionMonkey: Remove JS{Weak,Newborn}Roots
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #272859 - Flags: review?(jorendorff)
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.
Attachment #272859 - Flags: review?(jorendorff)
Depends on: 391211
Attached patch v2 (obsolete) — Splinter Review
Assignee: edilee → jorendorff
Attachment #272859 - Attachment is obsolete: true
Attachment #281537 - Flags: review?(igor)
Oops, I found a bug.  New patch coming in a second.
Attached patch v3 (obsolete) — Splinter Review
Attachment #281537 - Attachment is obsolete: true
Attachment #281540 - Flags: review?(igor)
Attachment #281537 - Flags: review?(igor)
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"
Attachment #281537 - Attachment is obsolete: false
Attached patch v3 to v4 incremental patch (obsolete) — Splinter Review
Attachment #281537 - Attachment is obsolete: true
Attachment #281540 - Attachment is obsolete: true
Attachment #281540 - Flags: review?(igor)
Attached patch v4 (obsolete) — Splinter Review
Attachment #281665 - Flags: review?(igor)
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.
Attached patch v5Splinter Review
v5 is just v4 plus the two-line change Igor suggested.
Attachment #281664 - Attachment is obsolete: true
Attachment #281665 - Attachment is obsolete: true
Attachment #281813 - Flags: review+
Attachment #281665 - Flags: review?(igor)
pushed to actionmonkey branch - changeset 3ae3132cd872.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
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
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.