Closed
Bug 377090
Opened 18 years ago
Closed 18 years ago
Introduce API to null out and restore cx->fp on a JSContext
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bzbarsky, Assigned: mrbkap)
References
Details
(Keywords: fixed1.8.0.13, fixed1.8.1.5)
Attachments
(5 files, 5 obsolete files)
5.09 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
5.21 KB,
patch
|
Details | Diff | Splinter Review | |
709 bytes,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
5.22 KB,
patch
|
dveditz
:
approval1.8.1.5+
|
Details | Diff | Splinter Review |
5.23 KB,
patch
|
dveditz
:
approval1.8.0.13+
|
Details | Diff | Splinter Review |
See bug 371858.
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
Updated•18 years ago
|
Assignee: general → mrbkap
Assignee | ||
Comment 1•18 years ago
|
||
Brendan had proposed some names, but I've forgotten those. So here's one proposal.
Attachment #261209 -
Flags: review?(brendan)
Comment 2•18 years ago
|
||
Doesn't the GC need to be able to reach all the suspended cg->fp's in the case where GC runs while some frames are suspended? IOW, shouldn't the JSContext keep a reference to the suspened fp and mark that when GC'ing?
Comment 3•18 years ago
|
||
Comment on attachment 261209 [details] [diff] [review]
Proposed patch, v1
> JS_PUBLIC_API(JSStackFrame *)
>+JS_SuspendFrameExecution(JSContext *cx)
JS_SaveFrameChain and JS_RestoreFrameChain are better names I think.
Documentation should note how they must be used LIFO with the cx's stack balanced in between.
>+{
>+ JSStackFrame *fp;
>+
>+ fp = cx->fp;
>+ fp->dormantNext = cx->dormantFrameChain;
As jst notes, you must then set cx->dormantFrameChain = fp.
>+ cx->fp = NULL;
>+ return fp;
>+}
>+
>+JS_PUBLIC_API(void)
>+JS_ResumeFrameExecution(JSContext *cx, JSStackFrame *fp)
>+{
>+ JS_ASSERT(cx->fp == NULL);
!cx->fp is the local C idiom.
>+
>+ cx->fp = fp;
>+ cx->dormantFrameChain = fp->dormantNext;
>+ fp->dormantNext = NULL;
>+}
The rest looks right.
/be
Attachment #261209 -
Flags: review?(brendan) → review-
Assignee | ||
Comment 4•18 years ago
|
||
Yikes, I must have diff'd that patch before saving -- it was missing jsdbgapi.c altogether. I've actually looked at this diff!
Attachment #261209 -
Attachment is obsolete: true
Attachment #261353 -
Flags: review?(brendan)
Assignee | ||
Comment 5•18 years ago
|
||
I missed the assert with that last patch.
Attachment #261353 -
Attachment is obsolete: true
Attachment #261354 -
Flags: review?(brendan)
Attachment #261353 -
Flags: review?(brendan)
Comment 6•18 years ago
|
||
Comment on attachment 261354 [details] [diff] [review]
With *everything* fixed
>Index: jsdbgapi.c
I wonder whether we shouldn't add these to jsapi.h and move JSStackFrame's typedef from jsprvtd.h to jspubtd.h.
> JS_PUBLIC_API(JSStackFrame *)
>+JS_SaveFrameChain(JSContext *cx)
>+{
>+ JSStackFrame *fp;
>+
>+ fp = cx->fp;
Null-check fp and return it early to relieve callers from having to worry about whether cx->fp is null.
JS_ASSERT(!fp->dormantNext) here.
>+ fp->dormantNext = cx->dormantFrameChain;
>+ cx->dormantFrameChain = fp;
>+ cx->fp = NULL;
>+ return fp;
>+}
>+
>+JS_PUBLIC_API(void)
>+JS_RestoreFrameChain(JSContext *cx, JSStackFrame *fp)
>+{
>+ JS_ASSERT(!cx->fp);
if (!fp) return.
>+
>+ cx->fp = fp;
>+ cx->dormantFrameChain = fp->dormantNext;
>+ fp->dormantNext = NULL;
[...]
> /*
>+ * Saving and restoring frame chains.
>+ *
>+ * These two functions are used to set aside cx->fp while that frame is
>+ * inactive. After a call to JS_SaveFrameChain, it looks as if there is no
>+ * code running on cx. Before calling JS_RestoreFrameChain, cx's call stack
>+ * must be balanced and all nested calls to JS_SaveFrameChain must have had
>+ * matching JS_RestoreFrameChain calls.
Note !cx->fp tolerance in separate paragraph.
/be
Assignee | ||
Comment 7•18 years ago
|
||
Attachment #261354 -
Attachment is obsolete: true
Attachment #261384 -
Flags: review?(brendan)
Attachment #261354 -
Flags: review?(brendan)
Comment 8•18 years ago
|
||
Comment on attachment 261384 [details] [diff] [review]
Addressing comments
>Index: jsapi.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsapi.c,v
>retrieving revision 3.311
>diff -p -U8 -r3.311 jsapi.c
>--- jsapi.c 10 Apr 2007 13:29:10 -0000 3.311
>+++ jsapi.c 12 Apr 2007 15:36:03 -0000
>@@ -4416,16 +4416,43 @@ JS_PUBLIC_API(void)
> JS_SetCallReturnValue2(JSContext *cx, jsval v)
> {
> #if JS_HAS_LVALUE_RETURN
> cx->rval2 = v;
> cx->rval2set = JS_TRUE;
> #endif
> }
>
>+JS_PUBLIC_API(JSStackFrame *)
>+JS_SaveFrameChain(JSContext *cx)
>+{
>+ JSStackFrame *fp;
>+
>+ fp = cx->fp;
>+ if (!fp)
>+ return fp;
Nit: extra blank line here.
>+ JS_ASSERT(!fp->dormantNext);
>+ fp->dormantNext = cx->dormantFrameChain;
>+ cx->dormantFrameChain = fp;
>+ cx->fp = NULL;
>+ return fp;
>+}
>+
>+JS_PUBLIC_API(void)
>+JS_RestoreFrameChain(JSContext *cx, JSStackFrame *fp)
>+{
>+ JS_ASSERT(!cx->fp);
>+
>+ if (!fp)
>+ return;
Personally I would cuddle the assertion and the if-return and put the blank line after the return.
>+ cx->fp = fp;
>+ cx->dormantFrameChain = fp->dormantNext;
>+ fp->dormantNext = NULL;
>+}
>+
> /************************************************************************/
>
> JS_PUBLIC_API(JSString *)
> JS_NewString(JSContext *cx, char *bytes, size_t nbytes)
> {
> size_t length = nbytes;
> jschar *chars;
> JSString *str;
>Index: jsapi.h
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsapi.h,v
>retrieving revision 3.141
>diff -p -U8 -r3.141 jsapi.h
>--- jsapi.h 7 Apr 2007 01:20:28 -0000 3.141
>+++ jsapi.h 12 Apr 2007 15:36:03 -0000
>@@ -1843,16 +1843,35 @@ JS_IsAssigning(JSContext *cx);
> * type value (obj, id). Only native methods can return reference types,
> * and if the returned value is used on the left-hand side of an assignment
> * op, the identified property will be set. If the return value is in an
> * r-value, the interpreter just gets obj[id]'s value.
> */
> extern JS_PUBLIC_API(void)
> JS_SetCallReturnValue2(JSContext *cx, jsval v);
>
>+/*
>+ * Saving and restoring frame chains.
>+ *
>+ * These two functions are used to set aside cx->fp while that frame is
>+ * inactive. After a call to JS_SaveFrameChain, it looks as if there is no
>+ * code running on cx. Before calling JS_RestoreFrameChain, cx's call stack
>+ * must be balanced and all nested calls to JS_SaveFrameChain must have had
>+ * matching JS_RestoreFrameChain calls.
>+ *
>+ * JS_SaveFrameChain deals with cx not having any code running on it. A NULL
Comments use null, not NULL, as the spelling.
r=me with these nits picked.
/be
Attachment #261384 -
Flags: review?(brendan) → review+
Comment 9•18 years ago
|
||
Will approve a branch patch after trunk landing, but not blocking
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
Assignee | ||
Comment 10•18 years ago
|
||
Assignee | ||
Comment 11•18 years ago
|
||
Fix checked into trunk.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 12•18 years ago
|
||
Isn't that patch assuming that cx->dormantFrameChain == fp at the entry to JS_RestoreFrameChain? If so, why bother with the arg?
Comment 13•18 years ago
|
||
(In reply to comment #12)
> Isn't that patch assuming that cx->dormantFrameChain == fp at the entry to
> JS_RestoreFrameChain? If so, why bother with the arg?
I dreamed there was an assertion to that effect. The arg is a good idea so we can assert that condition, but otherwise unnecessary. mrbkap, can you do a followup patch quickly? Thanks, bz.
/be
Assignee | ||
Comment 14•18 years ago
|
||
Attachment #261626 -
Flags: review?(brendan)
Reporter | ||
Comment 15•18 years ago
|
||
I guess it's just that the arg makes my life harder in bug 371858 -- I have to change data structures to store that arg... if it were not there, I would only need the cx, which I have.
Comment 16•18 years ago
|
||
Comment on attachment 261626 [details] [diff] [review]
Followup
Thanks. In light of bz's observation, perhaps we should have kept JSStackFrame private, as the new API begs the question: how does the GC know to scan the frame chain being set aside?
/be
Attachment #261626 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 17•18 years ago
|
||
It's not too late to undo that stuff. I think that the API that bz wants is something like:
JS_PUBLIC_API(void)
JS_SaveStackFrame(JSContext *cx);
JS_PUBLIC_API(void)
JS_RestoreStackFrame(JSContext *cx);
Which seems very magical to me (what's being saved, what's being restored?), but it isn't like the implementation of JS_RestoreStackFrame is going to change to actually need fp, ever.
Reporter | ||
Comment 18•18 years ago
|
||
JS_SetAsideStack/JS_RestoreStack ?
Comment 19•18 years ago
|
||
I mentioned "set-aside" but didn't proffer an antonym.
JS_SetAsideStack/JS_PutBackStack?
/be
Reporter | ||
Comment 20•18 years ago
|
||
I could live with that.
Comment 21•18 years ago
|
||
Hard to riff on the "dormant" metaphor tonight for some reason -- JS_Sleep and JS_Wakeup are wrong for so many reasons (they suggest thread or process scheduling from Unix). So I say it's comment 19 or bust -- mrbkap, want to reopen and patch one more time?
/be
Reporter | ||
Comment 22•18 years ago
|
||
So given that branch freeze is Friday, what should I do with bug 371858? Use this API? Or wait for the other one?
Comment 23•18 years ago
|
||
mrbkap must be busy -- college boy!
Ok, revised patch next.
/be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 24•18 years ago
|
||
I am now thinking the API checked in was the right one (sorry, bz). There's no safe way to handle the !cx->fp case without at least one bit of return value from JS_SetAsideStack, telling whether or not to call JS_PutBackStack. Otherwise one could have !cx->fp && cx->dormantFrameChain upon calling JS_SetAsideStack, which would return (void, in the proposal) right away. But JS_PutBackStack would then wrongly put back cx->dormantFrameChain as cx->fp.
With this patch, bz, you'd need to store a boolean or otherwise remember not to put back after a false-returning set aside call. That's hardly better than having to save fp, and saving fp allows us to sanity check cx->dormantFrameChain in put-back. So I'll revert these changes and propose another patch. mrbkap, feel free to minus this one.
/be
Attachment #261829 -
Flags: review?(mrbkap)
Comment 25•18 years ago
|
||
Just the missing assertion in JS_RestoreFrameChain.
I kept the save and restore trope, and the concrete "frame chain" idiom.
/be
Updated•18 years ago
|
Attachment #261832 -
Flags: review?(mrbkap)
Reporter | ||
Comment 26•18 years ago
|
||
Yeah, storing a boolean is just as much work as storing a JSStackFrame.
So I'll run with this API. I hadn't thought about the problem of no fp but having a dormantFrameChain....
Assignee | ||
Comment 27•18 years ago
|
||
I'm just going to check in the followup fix that you already stamped. Sorry for the delay. It's the second to last week of classes and all of my projects and papers are coming up.
Assignee | ||
Comment 28•18 years ago
|
||
The followup fix is now checked into trunk.
Status: REOPENED → RESOLVED
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•18 years ago
|
Attachment #261829 -
Attachment is obsolete: true
Attachment #261829 -
Flags: review?(mrbkap)
Assignee | ||
Updated•18 years ago
|
Attachment #261832 -
Attachment is obsolete: true
Attachment #261832 -
Flags: review?(mrbkap)
Comment 29•18 years ago
|
||
Comment on attachment 261626 [details] [diff] [review]
Followup
Wahhh, I liked my fp on the left of == assertion :-P.
/be
Updated•18 years ago
|
Flags: in-testsuite-
Reporter | ||
Comment 30•17 years ago
|
||
So could we get this in on branch too?
Assignee | ||
Comment 31•17 years ago
|
||
Sadly, this only applies to the 1.8.1 branch.
Attachment #264128 -
Flags: approval1.8.1.5?
Assignee | ||
Comment 32•17 years ago
|
||
Trivially hand-merged.
Attachment #264129 -
Flags: approval1.8.0.13?
Comment 33•17 years ago
|
||
Comment on attachment 264128 [details] [diff] [review]
Rolled-up patch
approved for 1.8.1.5 and 1.8.0.13, a=dveditz for release-drivers
Attachment #264128 -
Flags: approval1.8.1.5? → approval1.8.1.5+
Updated•17 years ago
|
Attachment #264129 -
Flags: approval1.8.0.13? → approval1.8.0.13+
Comment 34•17 years ago
|
||
Fix landed on behalf of bz on both branches.
Keywords: fixed1.8.0.13,
fixed1.8.1.5
You need to log in
before you can comment on or make changes to this bug.
Description
•