Closed
Bug 388622
Opened 17 years ago
Closed 17 years ago
ActionMonkey: Remove JS{Weak,Newborn}Roots
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: Mardak, Assigned: jorendorff)
References
Details
Attachments
(1 file, 5 obsolete files)
72.63 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•17 years ago
|
||
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
Reporter | ||
Updated•17 years ago
|
Summary: Remove JS{Weak,Newborn}Roots → ActionMonkey: Remove JS{Weak,Newborn}Roots
Reporter | ||
Comment 2•17 years ago
|
||
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)
Assignee | ||
Comment 3•17 years ago
|
||
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)
Assignee | ||
Comment 4•17 years ago
|
||
Assignee: edilee → jorendorff
Attachment #272859 -
Attachment is obsolete: true
Attachment #281537 -
Flags: review?(igor)
Assignee | ||
Comment 5•17 years ago
|
||
Oops, I found a bug. New patch coming in a second.
Assignee | ||
Comment 6•17 years ago
|
||
Attachment #281537 -
Attachment is obsolete: true
Attachment #281540 -
Flags: review?(igor)
Attachment #281537 -
Flags: review?(igor)
Comment 7•17 years ago
|
||
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
Assignee | ||
Comment 8•17 years ago
|
||
Attachment #281537 -
Attachment is obsolete: true
Attachment #281540 -
Attachment is obsolete: true
Attachment #281540 -
Flags: review?(igor)
Assignee | ||
Comment 9•17 years ago
|
||
Attachment #281665 -
Flags: review?(igor)
Comment 10•17 years ago
|
||
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.
Assignee | ||
Comment 11•17 years ago
|
||
(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.
Assignee | ||
Comment 12•17 years ago
|
||
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)
Assignee | ||
Comment 13•17 years ago
|
||
pushed to actionmonkey branch - changeset 3ae3132cd872.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 14•17 years ago
|
||
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
Updated•17 years ago
|
Flags: in-testsuite-
You need to log in
before you can comment on or make changes to this bug.
Description
•