377090 - Introduce API to null out and restore cx->fp on a JSContext

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 371858.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed patch, v1

Brendan had proposed some names, but I've forgotten those. So here's one proposal.

Comment 2

[Johnny Stenback (:jst)]

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

[Brendan Eich [:brendan]]

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

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fixed patch

Yikes, I must have diff'd that patch before saving -- it was missing jsdbgapi.c altogether. I've actually looked at this diff!

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: With *everything* fixed

I missed the assert with that last patch.

Comment 6

[Brendan Eich [:brendan]]

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

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Addressing comments

Comment 8

[Brendan Eich [:brendan]]

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

Comment 9

[Daniel Veditz [:dveditz]]

Will approve a branch patch after trunk landing, but not blocking

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: What I'm checking in

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Isn't that patch assuming that cx->dormantFrameChain == fp at the entry to JS_RestoreFrameChain?  If so, why bother with the arg?

Comment 13

[Brendan Eich [:brendan]]

(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

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Followup

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

[Brendan Eich [:brendan]]

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

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

JS_SetAsideStack/JS_RestoreStack ?

Comment 19

[Brendan Eich [:brendan]]

I mentioned "set-aside" but didn't proffer an antonym.

JS_SetAsideStack/JS_PutBackStack?

/be

Comment 20

[Boris Zbarsky [:bzbarsky]]

I could live with that.

Comment 21

[Brendan Eich [:brendan]]

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

Comment 22

[Boris Zbarsky [:bzbarsky]]

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

[Brendan Eich [:brendan]]

mrbkap must be busy -- college boy!

Ok, revised patch next.

/be

Comment 24

[Brendan Eich [:brendan]]

Attachment: alternative API, but see comment below

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

Comment 25

[Brendan Eich [:brendan]]

Attachment: better followup fix

Just the missing assertion in JS_RestoreFrameChain.

I kept the save and restore trope, and the concrete "frame chain" idiom.

/be

Comment 26

[Boris Zbarsky [:bzbarsky]]

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....

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

The followup fix is now checked into trunk.

Comment 29

[Brendan Eich [:brendan]]

Comment on attachment 261626 [details] [diff] [review]
Followup

Wahhh, I liked my fp on the left of == assertion :-P.

/be

Comment 30

[Boris Zbarsky [:bzbarsky]]

So could we get this in on branch too?

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Rolled-up patch

Sadly, this only applies to the 1.8.1 branch.

Comment 32

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: 1.8.0 patch

Trivially hand-merged.

Comment 33

[Daniel Veditz [:dveditz]]

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

Comment 34

[Johnny Stenback (:jst)]

Fix landed on behalf of bz on both branches.