446386 - SM: eliminating compiler pseudo-frames

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

Since fixing the bug 404935 the parser in SpiderMonkey uses compiler pseudo-frames just as a way to record information about scrip's parent scope and whether the compiled code is a special eval or debug script.

It would be nice to just use JSTreeContext for that avoiding the complexity of MaybeSetupFrame.

Comment 1

[Igor Bukanov]

Attachment: v1

The patch adds TCF_SPECIAL_SCRIPT flag to distinguish eval and debug scripts and adds the parent field to JSTreeContext. The field is shared with the fun field through a union, as the field is only relevant when compiling a script, not a function.

Comment 2

[Igor Bukanov]

Attachment: v2

The new version of the patch removes no longer necessary marking as eval frames intermediate function invocation frames when compiling the eval scripts.

Comment 3

[Igor Bukanov]

Attachment: v3

The new version of the patch syncs it with changes from the bug 445901.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 331083 [details] [diff] [review]
v3

>-            } else if (cg->treeContext.flags & TCF_COMPILE_N_GO) {
>-                obj = cx->fp->varobj;
>+            } else {
>+                JS_ASSERT(cg->treeContext.flags & TCF_COMPILE_N_GO);
>+                obj = cg->treeContext.u.parent;

Poor old "parent" is an overloaded term, yet not meaningful with respect to "script" -- better to keep using varobj, or else use scopeChain?

>-        if (fp->flags & JSFRAME_SCRIPT_OBJECT)

Can JSFRAME_SCRIPT_OBJECT be removed now too?

>+    union {
>+        JSFunction  *fun;           /* function to store argument and variable
>                                        names when flags & TCF_IN_FUNCTION */
>+        JSObject   *parent;         /* parent object for the script */

Indentation is off for *parent.

Great patch, want to get mrbkap to bless it too. I'll not r+ me with nits picked and pass review along to him.

/be

Comment 5

[Igor Bukanov]

Attachment: v4

The changes from the bug 448595 required to hand-merge the patch with the trunk in jsemit.cpp, so I ask for another review. The new version also addresses Brendan's nits.

Comment 6

[Igor Bukanov]

Review ping.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 332411 [details] [diff] [review]
v4

Sorry for the delay -- I was out of action at BlackHat and DEFCON for the past week.

Comment 8

[Igor Bukanov]

Attachment: v5

The landing of the up-globals support required to substantially alter the patch especially in BindNameToSlot from jsemit.cpp.

Note that the patch removes 

  STOBJ_SET_PARENT(FUN_OBJECT(cg->treeContext.fun), cx->fp->scopeChain);

from js_EmitFunctionScript as this is no longer necessary as the patch sets the proper scope for functions from compile-and-go when it creates the functions.

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 335717 [details] [diff] [review]
v5

Thanks so much for sweeping up the messes left over the years. This is a righteous patch.

I just noticed that JSOP_DEFFUN does not follow ES3 10.1.3! If it did, then I believe we could eliminate JSOP_CLOSURE. Can you confirm and if so, file a bug? Thanks again.

/be

Comment 10

[Igor Bukanov]

I will fix the bug 452742 with an updated patch.

Comment 11

[Igor Bukanov]

Attachment: v6 (-b version)

To properly implement display optimization for the eval scripts BindNameToSlot needs to access the callers frame as the scope chain alone does not provide the necessary information. 

So the new version of the patch passes both the scope chain and caller frame to js_CompileScript. The patch also uses the latter to distinguish eval and debug code from the rest of the scripts eliminating the need for TCF_SPECIAL_SCRIPT flag that the previous versions had.

The patch also fixes the bug 452742.

Comment 12

[Igor Bukanov]

Attachment: v6 (full patchc)

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 336163 [details] [diff] [review]
v6 (full patchc)

I wonder whether the goto arguments_check could be avoided by a tail helper function, or do-while(0) with break. Neither is clearly better than a downward goto or two, just wanted to get your thoughts.

>+     * local slot name. We also outside of any with blocks. Check if we can

s/We also/We are/ or "We are also"

>+    union {
>+        JSFunction  *fun;           /* function to store argument and variable
>                                        names when flags & TCF_IN_FUNCTION */
>+        struct {
>+            JSObject        *scopeChain;/* scope chain object for the script */
>+            JSStackFrame    *callerFrame;/* scripted caller frame for eval
>+                                            and debug scripts */
>+        } script;

Suggest calling this arm of the union "prog" to avoid JSScript *script canonical name confusion. Also, the member declarators should probably be indented less to avoid crowding the comments:

        struct {
            JSObject     *scopeChain;   /* scope chain object for the script */
            JSStackFrame *callerFrame;  /* scripted caller frame for eval and
                                           debug scripts */
        } prog;
12345678901234567890123456789012345678901234567890123456789012345678901234567890

Looks great otherwise.

Did the testsuite catch the with detection bug?

Any thoughts on unifying JSOP_CLOSURE and JSOP_DEFFUN?

/be

Comment 14

[Igor Bukanov]

(In reply to comment #13)
> Did the testsuite catch the with detection bug?

No.

> Any thoughts on unifying JSOP_CLOSURE and JSOP_DEFFUN?

JSOP_DEFFUN is purely an optimized version of JSOP_CLOSURE. It assumes the scope chain top is a variable object and that the frame is not an eval. To witness stripping comments:

BEGIN_CASE(JSOP_DEFFUN)
  LOAD_FUNCTION(0);
  JS_ASSERT(!fp->blockChain);
  JS_ASSERT((fp->flags & JSFRAME_EVAL) == 0);
  JS_ASSERT(fp->scopeChain == fp->varobj);
  obj2 = fp->scopeChain;
  attrs = JSPROP_ENUMERATE | JSPROP_PERMANENT;

do_deffun:
...

BEGIN_CASE(JSOP_CLOSURE)
  LOAD_FUNCTION(0);
  obj2 = js_GetScopeChain(cx, fp);
  if (!obj2)
      goto error;
  attrs = JSPROP_ENUMERATE;
  if (!(fp->flags & JSFRAME_EVAL))
      attrs |= JSPROP_PERMANENT;

  goto do_deffun;

Is the idea to remove this (probably worthless given the complexity of of the rest of code) optimization?

Comment 15

[Igor Bukanov]

(In reply to comment #13)
> >+        struct {
> >+            JSObject        *scopeChain;/* scope chain object for the script */
> >+            JSStackFrame    *callerFrame;/* scripted caller frame for eval
> >+                                            and debug scripts */
> >+        } script;
> 
> Also, the member declarators should probably be
> indented less to avoid crowding the comments

The word JSStackFrame finishes on the tab boundary (the length of "JSStackFrame" is 12). This requires to indent the members to the next tab stop to align them on the same column. This is what the patch does.

Comment 16

[Igor Bukanov]

Attachment: v7

In the new version I moved callerFrame to JSParseContext so JSTreccContext would not be bloated when compiling functions. That solved the problem with the crowded comments automatically :)

Comment 17

[Brendan Eich [:brendan]]

(In reply to comment #14)
> (In reply to comment #13)
> > Did the testsuite catch the with detection bug?
> 
> No.

D'oh!

> BEGIN_CASE(JSOP_DEFFUN)
...
>   attrs = JSPROP_ENUMERATE | JSPROP_PERMANENT;
> do_deffun:
> ...
> 
> BEGIN_CASE(JSOP_CLOSURE)
...
>   attrs = JSPROP_ENUMERATE;
>   if (!(fp->flags & JSFRAME_EVAL))
>       attrs |= JSPROP_PERMANENT;
>   goto do_deffun;

There is the bug I mentioned: ES1-3 10.1.3 all require eval-created function definitions (which are variable instantiations) to be deletable.

> Is the idea to remove this (probably worthless given the complexity of of the
> rest of code) optimization?

Yes.

/be

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 336166 [details] [diff] [review]
v7

>@@ -195,17 +195,17 @@ script_toString(JSContext *cx, uintN arg
> static JSBool
> script_compile_sub(JSContext *cx, JSObject *obj, uintN argc, jsval *argv,
>                    jsval *rval)
> {
...
>     script = js_CompileScript(cx, scopeobj, principals, tcflags,

No caller 3rd arg here.

Testing JS_HAS_SCRIPT_OBJECT seems worthwhile. Maybe we should get rid of Script objects finally. I haven't used them for testing in a long time (it's just not worth recompiling with the #ifs enabled).

r=me with that fix.

/be

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #18)
> Testing JS_HAS_SCRIPT_OBJECT seems worthwhile. Maybe we should get rid of
> Script objects finally.

Graydon found script objects useful for testing top-level scripts while he was working on updating the |const| implementation.

Comment 20

[Brian Crowder]

Didn't we resolve that use-case by adding disfile() to the shell?

Comment 21

[Igor Bukanov]

Attachment: v8

The new version makes jsscript.c to compile with JS_HAS_SCRIPT_OBJECT defined.

Comment 22

[Igor Bukanov]

Attachment: v9

My attempt to land the previous version of the patch triggered TUnit test failures. The reason for that is the check for cx->fp in js_ErrorToException that is called by js_ReportCompileErrorNumber. Since the patch removes the compiler psedo-frame, that triggered early reporting of syntax without creating an exception object. That in turn confused xpcshell.

In the new version I fixed that by moving cx->fp check to two other callers of js_ErrorToException.

Here is the regression test for js shell:

try {
    evalcx(".");
    throw "must throw";
} catch (e) {
    if (e.name != "SyntaxError")
        throw e;
}

print("Ok");

Comment 23

[Igor Bukanov]

Attachment: v8-v9 delta

Comment 24

[Brendan Eich [:brendan]]

Comment on attachment 336479 [details] [diff] [review]
v9

Sorry, missed this in my bugmail and should have checked my request queue.

/be

Comment 25

[Igor Bukanov]

landed - http://hg.mozilla.org/mozilla-central/rev/ef557ac7be73

Comment 26

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-446386.js,v  <--  regress-446386.js
initial revision: 1.1

http://hg.mozilla.org/mozilla-central/rev/f0e9fd501e63