334261 - Avoid creating unnecessary objects for primitives

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Andreas]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.3) Gecko/20040914 (not Googlebot)
Build Identifier: CVS js

Every time a property or method is accessed for a primitive value, it is
first wrapped into an object. This is not needed if the method is native.
The object will be discarded without ever being used.


Reproducible: Always

Steps to Reproduce:
1. Apply patch
2. Run testcase

Actual Results:  
Huge performance win with patch applied.

Comment 1

[Andreas]

Attachment: patch

The main part of the patch is in jsinterp.c:

For JSOP_GETMETHOD, look up the method in the class prototype (note
bug 304376) and defer conversion to object until needed. It may be needed
if a method has been added to the prototype or if a __noSuchMethod__ handler
will be called.

For JSOP_GETPROP, if the property is string length of a native string,
return it directly.

If JS_VERSION < 160 then JSOP_GETPROP is also used for methods and the patch
will have no effect on them (JS_VERSION < 160 is currently broken, but the
patch includes a workaround to be able to test (undefs JS_THREADED_INTERP)).

Most predefined methods of the String, Number and Boolean prototypes are
already designed to take primitives as |this| value. Biggest problem is,
that there is no room for additional bits in the function flags. We need
bits to indicate if a function can operate on primitive values. I took
7 bits from the boolean "interpreted" byte. Another possibility would be to
use the "spare" word of the native part of the union, but maybe some day
there will be a way for interpreted functions to work on primitives.

Passed the testsuite without regressions, but I'm not very familiar with the
code, so there might be some bugs or better solutions.

Comment 2

[Andreas]

Attachment: testcase (Perl script)

Takes 2 arguments: paths to 2 js executables to compare.
Executes some loops in the form of

  do value.method(); while ( ++i < n );

and measures time (real time, not CPU time).
my_nop() is defined as String.prototype.my_nop= function () {};

Options:
  -i: Use immediate operand inside the loop instead of a variable
      (e.g. "abc".method() instead of v.method() where var v= "abc")
  -c <count>: Loop count (default 1e6)
  -s <string>: use <string> as value (default "abc")
  -n <number>: use <number> as value (default 0)
  -b <boolean>: use <boolean> as value (default true)

Output:
  1st number is the binary logarithm of the quotient of the two times with
  the loop overhead as measured by the first test subtracted. I.e., +1 means
  that the second executable needed 2^1 = 2 times as long as the first. If
  the arguments are swapped, only the sign should change. 2nd and 3rd number
  are absolute times including loop overhead.

I got these results (fresh CVS build, without and with patch):

                           "abc": -0.155 (295 -> 265)
                    "abc".length: -4.205 (3818 -> 456)
                "abc".toString(): -1.611 (4546 -> 1657)
                 "abc".valueOf(): -1.612 (4528 -> 1650)
                  "abc".charAt(): -1.786 (6571 -> 2085)
              "abc".charCodeAt(): -1.964 (6305 -> 1805)
               "abc".indexOf(""): -1.931 (6242 -> 1825)
                   "abc".slice(): -2.041 (6140 -> 1685)
               "abc".match(/.+/): -1.250 (9675 -> 4209)
          "abc".replace(/.+/,""): -1.100 (12098 -> 5771)
                 "abc".split(""): -0.361 (22898 -> 17862)
             "abc".toUpperCase(): -1.500 (7204 -> 2708)
                    "abc".bold(): -1.521 (7334 -> 2717)
                   "abc".quote(): -1.142 (10608 -> 4939)
                "abc".toSource(): -0.645 (11414 -> 7376)
                  "abc".my_nop(): -0.389 (4733 -> 3653)
                  (0).toString(): -0.869 (4645 -> 2646)
                   (0).valueOf(): -1.240 (3518 -> 1630)
                   (0).toFixed(): -0.931 (5420 -> 2953)
             (0).toExponential(): -1.017 (6792 -> 3475)
              (0).toPrecision(3): -0.857 (5773 -> 3289)
                  (0).toSource(): -0.672 (7079 -> 4522)
                 true.toString(): -1.208 (3458 -> 1634)
                  true.valueOf(): -1.197 (3387 -> 1614)
                 true.toSource(): -0.706 (6499 -> 4067)

Comment 3

[Brendan Eich [:brendan]]

Cool -- this idea has been a long time coming.  In 1995 I made a note, after reluctantly deciding to have primitive types at all in the language (to match Java, per Netscape management instructions -- what a mistake) to optimize away temporary object wrappers for primitive values operated on by methods.  That was in the original Mocha runtime, never open-sourced.

Detailed comments tomorrow.

/be

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> In 1995 I made a note, after
> reluctantly deciding to have primitive types at all in the language (to match
> Java, per Netscape management instructions -- what a mistake)

This is misleading, I didn't intend it to be.  Netscape management imperative was that JS have Java-like (C-like, really) syntax.  It was a high-level instruction. I have to admit since I was under great time pressure, I also found it easier in the short run to hack primitives as separate types from object.  So, I did not mean to "blame management".  It's all water way under the bridge now, anyway.

The good news is that in ECMA-262 Edition 4 (JS2, ES4), the primitive types go away.  Waldemar's drafts did away with them with my agreement many years ago.  No one depends on there being a difference, e.g. by capturing a ref to a wrapper from a method added to String.prototype.  I'm not even sure IE allows this capture, or does anything like what we do.

/be

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 218608 [details] [diff] [review]
patch

> struct JSFunctionSpec {
>     const char      *name;
>     JSNative        call;
>-    uint8           nargs;
>-    uint8           flags;
>+    uint16          nargs;
>+    uint16          flags;
>     uint16          extra;      /* number of arg slots for local GC roots */
> };

We try to keep binary as well as source API compatibility.  But we're out of bits, so may as well enlarge to 16 bits.

>+    if (JSVAL_IS_BOOLEAN(OBJECT_TO_JSVAL(obj)))
>+        v = OBJECT_TO_JSVAL(obj);
>+    else {

Nit: prevailing style braces both then and else parts if either is braced.

More substantively, I think (jsval)obj for the argument to JSVAL_IS_BOOLEAN is better.  It shows that there is something (a) not paradoxical, and (b) unusual due to special flags, going on here.

Same comment applies in other places.

>+++ mozilla/js/src/jsfun.h	16 Apr 2006 17:30:37 -0000
>@@ -40,43 +40,47 @@
> #ifndef jsfun_h___
> #define jsfun_h___
> /*
>  * JS function definitions.
>  */
> #include "jsprvtd.h"
> #include "jspubtd.h"
>
> JS_BEGIN_EXTERN_C
>
>+#define JSFUN_INTERPRETED 0x8000 /* special flag:
>+                                    use u.i if set, u.n if unset */

Suggest commenting on disjointness requirement w.r.t. JSFUN_* flags from jsapi.h.  Also, move down below struct to precede FUN_NATIVE/FUN_SCRIPT immediately.

>-#define FUN_NATIVE(fun)         ((fun)->interpreted ? NULL : (fun)->u.n.native)
>-#define FUN_SCRIPT(fun)         ((fun)->interpreted ? (fun)->u.i.script : NULL)
>+#define FUN_NATIVE(fun)         ((fun)->flags & JSFUN_INTERPRETED ?   \
>+                                 NULL : (fun)->u.n.native)
>+#define FUN_SCRIPT(fun)         ((fun)->flags & JSFUN_INTERPRETED ?   \
>+                                 (fun)->u.i.script : NULL)

Define a helper macro:

#define FUN_INTERPRETED(fun)    ((fun)->flags & JSFUN_INTERPRETED)

and use it to fit FUN_NATIVE/FUN_SCRIPT on one line each.  For future reference, prevailing style puts backslashes in column 79.

>+++ mozilla/js/src/jsinterp.c	16 Apr 2006 17:30:55 -0000
>@@ -294,20 +294,35 @@ static JSClass prop_iterator_class = {
>             b = JSVAL_TO_BOOLEAN(v);                                          \
>         } else {                                                              \
>             SAVE_SP_AND_PC(fp);                                               \
>             ok = js_ValueToBoolean(cx, v, &b);                                \
>             if (!ok)                                                          \
>                 goto out;                                                     \
>         }                                                                     \
>         sp--;                                                                 \
>     JS_END_MACRO
>
>+#define PRIMITIVE_TO_OBJECT(cx, v, obj)                                       \
>+    JS_BEGIN_MACRO                                                            \
>+        SAVE_SP(fp);                                                          \
>+        if (JSVAL_IS_STRING(v)) {                                             \
>+            obj = js_StringToObject(cx, JSVAL_TO_STRING(v));                  \
>+        } else if (JSVAL_IS_INT(v)) {                                         \
>+            obj = js_NumberToObject(cx, (jsdouble)JSVAL_TO_INT(v));           \
>+        } else if (JSVAL_IS_DOUBLE(v)) {                                      \
>+            obj = js_NumberToObject(cx, *JSVAL_TO_DOUBLE(v));                 \
>+        } else {                                                              \
>+            JS_ASSERT(JSVAL_IS_BOOLEAN(v));                                   \
>+            obj = js_BooleanToObject(cx, JSVAL_TO_BOOLEAN(v));                \
>+        }                                                                     \
>+    JS_END_MACRO

Here and elsewhere, null and undefined are not handled by new code that does handle other primitive types.  Of course undefined can't be the value of any |this| parameter, and null should not end up in |this|, but JSVAL_IS_OBJECT(JSVAL_NULL), so there's a hazard.  The JS_ASSERT(JSVAL_IS_BOOLEAN(v)) helps keep things sane, but a comment before the macro would be nicer.

>     /* Load thisp after potentially calling NoSuchMethod, which may set it. */
>-    thisp = JSVAL_TO_OBJECT(vp[1]);

Removing the line means changing or removing the comment, somehow.

>@@ -1086,68 +1106,102 @@ js_Invoke(JSContext *cx, uintN argc, uin
>             }
>         }
>         fun = NULL;
>         script = NULL;
>         nslots = nvars = 0;
>
>         /* Try a call or construct native object op. */
>         native = (flags & JSINVOKE_CONSTRUCT) ? ops->construct : ops->call;
>         if (!native)
>             goto bad;
>+
>+        if (!JSVAL_IS_OBJECT(vp[1])) {
>+            PRIMITIVE_TO_OBJECT(cx, vp[1], thisp);
>+            if (!thisp)
>+                goto out2;
>+            vp[1]= OBJECT_TO_JSVAL(thisp);
>+        }
>+
>+        /* Compute the 'this' parameter and store it in frame as frame.thisp. */
>+        thisp = JSVAL_TO_OBJECT(vp[1]);
>+        frame.thisp = js_ComputeThis(cx, thisp, sp - argc);

Could set thisp, not frame.thisp, here and in matching else or disjoint cases below, so that the frame.thisp addressing and store were commoned after the enclosing if/else.

>-        /* Handle bound method special case. */
>-        if (fun->flags & JSFUN_BOUND_METHOD)
>-            thisp = parent;
>+        if (fun->flags & JSFUN_BOUND_METHOD) {
>+            /* Handle bound method special case. */
>+            thisp= parent;

Nit about space before =, no doubt a typo.

>+            frame.thisp = js_ComputeThis(cx, thisp, sp - argc);

Defer frame.thisp address computation and store per above.  And so on below.

>+        } else if (JSVAL_IS_OBJECT(vp[1])) {
>+            thisp= (JSObject *) vp[1];

JSVAL_TO_OBJECT(vp[1])

>+            frame.thisp = js_ComputeThis(cx, thisp, sp - argc);

Ibid.

>+        } else {
>+            if (JSVAL_IS_STRING(vp[1])) {
>+                if (fun->flags & JSFUN_THISP_STRING) {
>+                    frame.thisp = thisp = (JSObject *) vp[1];
>+                    goto init_frame;

So the frame.thisp lvalue addressing and store (I mean, 'frame.thisp = thisp;) should be at init_frame:.

Good use of naked cast (JSObject *) vp[1], in contrast to JSVAL_TO_OBJECT.

>+                }
>+            } else if (JSVAL_IS_NUMBER(vp[1])) {
>+                if (fun->flags & JSFUN_THISP_NUMBER) {
>+                    frame.thisp = thisp = (JSObject *) vp[1];

Ibid.

>+                    goto init_frame;
>+                }
>+            } else {
>+                JS_ASSERT(JSVAL_IS_BOOLEAN(v));                                   \

Stray backslash after spaces from copy/paste.

>+                if (fun->flags & JSFUN_THISP_BOOLEAN) {
>+                    frame.thisp = thisp = (JSObject *) vp[1];
>+                    goto init_frame;
>+                }
>+            }
>+            PRIMITIVE_TO_OBJECT(cx, vp[1], thisp);

Does it help GCC or other compilers to put vp[1] into a local variable?

>@@ -1390,21 +1444,22 @@ js_InternalGetOrSet(JSContext *cx, JSObj
>      * policies requires it.  We make a checkAccess or checkObjectAccess call
>      * back to the embedding program only in those cases where we're not going
>      * to call an embedding-defined native function, getter, setter, or class
>      * hook anyway.  Where we do call such a native, there's no need for the
>      * engine to impose a separate access check callback on all embeddings --
>      * many embeddings have no security policy at all.
>      */
>     JS_ASSERT(mode == JSACC_READ || mode == JSACC_WRITE);
>     if (cx->runtime->checkObjectAccess &&
>         JSVAL_IS_FUNCTION(cx, fval) &&
>-        ((JSFunction *)JS_GetPrivate(cx, JSVAL_TO_OBJECT(fval)))->interpreted &&
>+        (((JSFunction *)JS_GetPrivate(cx, JSVAL_TO_OBJECT(fval)))->flags &
>+         JSFUN_INTERPRETED) &&

FUN_INTERPRETED(...) to the rescue, stylistically speaking! ;-)

A temporary JSFunction *fun variable wouldn't hurt, but I avoided it because single-use, and because the cast expression fit on one line.

> /*
>  * Threaded interpretation via computed goto appears to be well-supported by
>  * GCC 3 and higher.  IBM's C compiler when run with the right options (e.g.,
>  * -qlanglvl=extended) also supports threading.  Ditto the SunPro C compiler.
>  * Add your compiler support macros here.
>  */
>-#if __GNUC__ >= 3 ||                                                          \
>+#if JS_VERSION >= 160 && (                                                    \

Comment this change?

>+                SAVE_SP_AND_PC(fp); /* XXX do we need this here? */

Yes -- either before js_DecompileValueGenerator or js_GetClassPrototype call-out, whichever comes first.

>+                if (JSVAL_IS_STRING(lval)) {
>+                    atom = cx->runtime->atomState.StringAtom;
>+                } else if (JSVAL_IS_NUMBER(lval)) {
>+                    atom = cx->runtime->atomState.NumberAtom;
>+                } else if (JSVAL_IS_BOOLEAN(lval)) {
>+                    atom = cx->runtime->atomState.BooleanAtom;
>+                } else {
>+                    JS_ASSERT(JSVAL_IS_NULL(lval) || JSVAL_IS_VOID(lval));
>+                    str = js_DecompileValueGenerator(cx, JSDVG_SEARCH_STACK,
>+                                                     lval, NULL);
>+                    if (str) {
>+                        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
>+                                             JSMSG_NO_PROPERTIES,
>+                                             JS_GetStringBytes(str));
>+                    }
>+                    ok = JS_FALSE;
>+                    goto out;
>+                }
>+                ok = js_GetClassPrototype(cx, NULL, atom, &obj);
>+                if (!ok)
>+                    goto out;
>+                JS_ASSERT(obj);
>+                STORE_OPND(-1, OBJECT_TO_JSVAL(obj));
>+                SAVE_SP_AND_PC(fp);

But then you don't need it again here, since neither sp nor pc has changed.

Thanks for writing this patch!

/be

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #5)
> (From update of attachment 218608 [details] [diff] [review] [edit])
> > struct JSFunctionSpec {
> >     const char      *name;
> >     JSNative        call;
> >-    uint8           nargs;
> >-    uint8           flags;
> >+    uint16          nargs;
> >+    uint16          flags;
> >     uint16          extra;      /* number of arg slots for local GC roots */
> > };
> 
> We try to keep binary as well as source API compatibility.  But we're out of
> bits, so may as well enlarge to 16 bits.

Forgot to add that for alignment and future proofing, we should add a uint16 spare; at the end.

Andreas, do you want to take assignment of this bug?  I'll help review and get the patch checked in.

/be

Comment 7

[Andreas]

(In reply to comment #5)

> >+    if (JSVAL_IS_BOOLEAN(OBJECT_TO_JSVAL(obj)))
> >+        v = OBJECT_TO_JSVAL(obj);
> >+    else {
>
> Nit: prevailing style braces both then and else parts if either is braced.

I saw the braceless style in jsnum.c and did the same.
Fixed the 2 old occurrences I found too.

> >+#define JSFUN_INTERPRETED 0x8000 /* special flag:
> >+                                    use u.i if set, u.n if unset */
> 
> Suggest commenting on disjointness requirement w.r.t. JSFUN_* flags from
> jsapi.h.

Obvious in jsfun.h, but I added a comment in jsapi.h.
  
> >+#define PRIMITIVE_TO_OBJECT(cx, v, obj)
[...]
> Here and elsewhere, null and undefined are not handled by new code that does
> handle other primitive types.  Of course undefined can't be the value of any
> |this| parameter, and null should not end up in |this|, but
> JSVAL_IS_OBJECT(JSVAL_NULL), so there's a hazard.  The
> JS_ASSERT(JSVAL_IS_BOOLEAN(v)) helps keep things sane, but a comment before the
> macro would be nicer.

Every use of this macro is inside |if (!JSVAL_IS_OBJECT(v))|, thus v cannot
be null. The void case should be sufficiently covered by the assert.
Added a comment.

> >+        /* Compute the 'this' parameter and store it in frame as frame.thisp. */
> >+        thisp = JSVAL_TO_OBJECT(vp[1]);
> >+        frame.thisp = js_ComputeThis(cx, thisp, sp - argc);
> 
> Could set thisp, not frame.thisp, here and in matching else or disjoint cases
> below, so that the frame.thisp addressing and store were commoned after the
> enclosing if/else.

The original thisp will be used later on:

    /* Default return value for a constructor is the new object. */
    if (flags & JSINVOKE_CONSTRUCT)
        frame.rval = OBJECT_TO_JSVAL(thisp);

Is it save to assume |thisp == frame.thisp| in a constructor? Then we could
skip js_ComputeThis() in a constructor. Especially I don't know about the
JSFUN_BOUND_METHOD case.

An optimizing compiler may common or duplicate statements (if it saves
a jump), no matter how you write it.

> >+            thisp= parent;
> 
> Nit about space before =, no doubt a typo.

Not exactly a typo. I use this style to make a more visible distinction
to == in own code. But I didn't intend to use it here, of course.

> >+        } else if (JSVAL_IS_OBJECT(vp[1])) {
> >+            thisp= (JSObject *) vp[1];
> 
> JSVAL_TO_OBJECT(vp[1])

Would save the JSVAL_CLRTAG(), which isn't likely to be optimized away,
but JSVAL_TO_OBJECT() is cleaner code, so I'll use it.

> >+                JS_ASSERT(JSVAL_IS_BOOLEAN(v));                                   \
> 
> Stray backslash after spaces from copy/paste.

Worse, v should be vp[1] here.

> Does it help GCC or other compilers to put vp[1] into a local variable?

Should be in a register anyways and may even cause older gcc not to use
a register. But js is compiled with -O only by default and gcc does no
substantial optimization at -O1, so it will help.

I was curious about the particular compiler output at -On here; will attach
it in case anybody else is interested in. The simpler code seems to help
gcc (3.4) even at -O2 to concentrate on other optimizations.

(In reply to comment #6)

> Andreas, do you want to take assignment of this bug?  I'll help review and get
> the patch checked in.

I don't care if it is assigned to [e-mail] or to me (but cannot change
it myself). Most things I can do I have already done.

Comment 8

[Andreas]

Attachment: updated patch

Changes:
- Addressed the comments above
- Fixed a (uint8) fun->flags in js.c
- Commoned duplicated conversion to object inside NoSuchMethod()
- Unpacked one instance of PRIMITIVE_TO_OBJECT() in js_Invoke()
- Some minor changes in js_Invoke()

Comment 9

[Andreas]

Attachment: JFYI: gcc 3.4 output for a code snippet at -O2, -O1 and -O0 (partly commented)

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 218810 [details] [diff] [review]
updated patch

Thanks again, Andreas.  Thanks also for fixing those old brace-style deviations in jsnum.c.  If you could, please request review from me for future patches by setting the review? flag when attaching.  A few more mostly-style comments:

>+#define JSFUN_FLAGS_MASK      0x07f8    /* overlay JSFUN_* attributes --
>+                                           note that bit #15 is used by jsfun.h
>+                                           to flag interpreted functions */

Please change "by jsfun.h" to "internally".  jsapi.h is public API and I'd prefer that it not potentially draw naive users into depending on private .h files, even a little bit.

>+    if (JSVAL_IS_BOOLEAN((jsval)obj)) {
>+        v = OBJECT_TO_JSVAL(obj);

Here and other places, I argue that the then clause should be 'v = (jsval)obj;' to match the condition.  The same argument applies as before: something unusual is going on, obj is not a valid JSObject *, rather it's a tagged jsval, so it should not be put through OBJECT_TO_JSVAL (even though that macro happens to do no untagging).

>+#define JSFUN_INTERPRETED 0x8000 /* special flag:
>+                                    use u.i if set, u.n if unset */

Lose the "special flag:" and pull the second line of the comment up to fit.

>+
>+#define FUN_INTERPRETED(fun)    ((fun)->flags & JSFUN_INTERPRETED)
>+#define FUN_NATIVE(fun)         (FUN_INTERPRETED(fun) ?                       \
>+                                 NULL : (fun)->u.n.native)
>+#define FUN_SCRIPT(fun)         (FUN_INTERPRETED(fun) ?                       \
>+                                 (fun)->u.i.script : NULL)
>

I see no reason for continuation lines:

#define FUN_INTERPRETED(fun) (fun)->flags & JSFUN_INTERPRETED)
#define FUN_NATIVE(fun)      (FUN_INTERPRETED(fun) ? NULL : (fun)->u.n.native)
#define FUN_SCRIPT(fun)      (FUN_INTERPRETED(fun) ? (fun)->u.i.script : NULL)
12345678901234567890123456789012345678901234567890123456789012345678901234567890

Nit-pick, but it helps readability.


>+    jsval *vp, v, thispv;

I would prefer thisv as the name, since thisp is so named to avoid some compilers barfing on the C++ reserved word while adding only the trailing p-for-pointer.  So thisv is sufficient to avoid conflict, and minimal given v-for-value.

>     /* Load thisp after potentially calling NoSuchMethod, which may set it. */
>-    thisp = JSVAL_TO_OBJECT(vp[1]);
>+    thispv = vp[1];

Comment should track thispv or thisv name change.

Control flow analysis by eliding unrelated lines:

>+        frame.thisp = js_ComputeThis(cx, thisp, vp + 2);
>     } else {
>+        /* Compute the 'this' parameter and store it in frame as frame.thisp. */
>+        if (fun->flags & JSFUN_BOUND_METHOD) {
>+            frame.thisp = js_ComputeThis(cx, thisp, vp + 2);
>+        } else if (JSVAL_IS_OBJECT(thispv)) {
>+            frame.thisp = js_ComputeThis(cx, thisp, vp + 2);
>+        } else {
>+            if (JSVAL_IS_STRING(thispv)) {
>+                if (fun->flags & JSFUN_THISP_STRING) {
>+                    frame.thisp = thisp = (JSObject *) thispv;
>+                    goto init_frame;
>+                }
>+                thisp = js_StringToObject(cx, JSVAL_TO_STRING(thispv));
>+            } else if (JSVAL_IS_INT(thispv)) {
>+                if (fun->flags & JSFUN_THISP_NUMBER) {
>+                    frame.thisp = thisp = (JSObject *) thispv;
>+                    goto init_frame;
>+                }
>+                thisp = js_NumberToObject(cx, (jsdouble)JSVAL_TO_INT(thispv));
>+            } else if (JSVAL_IS_DOUBLE(thispv)) {
>+                if (fun->flags & JSFUN_THISP_NUMBER) {
>+                    frame.thisp = thisp = (JSObject *) thispv;
>+                    goto init_frame;
>+                }
>+                thisp = js_NumberToObject(cx, *JSVAL_TO_DOUBLE(thispv));
>+            } else {
>+                JS_ASSERT(JSVAL_IS_BOOLEAN(thispv));
>+                if (fun->flags & JSFUN_THISP_BOOLEAN) {
>+                    frame.thisp = thisp = (JSObject *) thispv;
>+                    goto init_frame;
>+                }
>+                thisp = js_BooleanToObject(cx, JSVAL_TO_BOOLEAN(thispv));
>+            }
>+            frame.thisp = thisp;
>+        }
>+    }
>+    if (!frame.thisp) {

So all paths that reach this if must set frame.thisp, and thisp is a dead value here.  Therefore why not update thisp (now that you have thispv/thisv) and let the store of thisp to frame.thisp happen at init_frame: or below it?  Apart from compiler vicissitudes, it would eliminate the chained assignments above, simplifying the data flow and making things more readable.

>+        ok = JS_FALSE;
>+        goto out2;
>     }
>
>+  init_frame:
> /*
>  * Threaded interpretation via computed goto appears to be well-supported by
>  * GCC 3 and higher.  IBM's C compiler when run with the right options (e.g.,
>  * -qlanglvl=extended) also supports threading.  Ditto the SunPro C compiler.
>+ * Currently it's broken for JS_VERSION < 160, though.

Could you please add " this isn't worth fixing" after "though"?  Thanks.

>                 /* Compute the 'this' parameter now that argv is set. */
>+                if (!JSVAL_IS_OBJECT(vp[1])) {
>+                    PRIMITIVE_TO_OBJECT(cx, vp[1], obj2);
>+                    if (!obj2)
>+                        goto out;
>+                    vp[1]= OBJECT_TO_JSVAL(obj2);

Space before = prevailing style nit.

/be

Comment 11

[Andreas]

(In reply to comment #10)
> So all paths that reach this if must set frame.thisp, and thisp is a dead value
> here.

No, it's not dead. From my comment #7:

| The original thisp will be used later on:
| 
|     /* Default return value for a constructor is the new object. */
|     if (flags & JSINVOKE_CONSTRUCT)
|         frame.rval = OBJECT_TO_JSVAL(thisp);
| 
| Is it save to assume |thisp == frame.thisp| in a constructor? Then we could
| skip js_ComputeThis() in a constructor. Especially I don't know about the
| JSFUN_BOUND_METHOD case.

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #11)
> (In reply to comment #10)
> | The original thisp will be used later on:
> | 
> |     /* Default return value for a constructor is the new object. */
> |     if (flags & JSINVOKE_CONSTRUCT)
> |         frame.rval = OBJECT_TO_JSVAL(thisp);
> | 
> | Is it save to assume |thisp == frame.thisp| in a constructor? Then we could
> | skip js_ComputeThis() in a constructor.

Yes, you can skip js_ComputeThis for constructors, the set of js_Invoke callers that pass JSINVOKE_CONSTRUCT is internal to the engine and all bind vp[1] to a new object of the nominal class to construct.

> Especially I don't know about the
> | JSFUN_BOUND_METHOD case.
> 

A bound method can't be used as a constructor, because its |this| parameter will always be bound to the callee's parent object.

/be

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #11)
> (In reply to comment #10)
> | The original thisp will be used later on:
> | 
> |     /* Default return value for a constructor is the new object. */
> |     if (flags & JSINVOKE_CONSTRUCT)
> |         frame.rval = OBJECT_TO_JSVAL(thisp);

Note from cvs history how this used to occur in js_ComputeThis.  It should be true that thisp is not recomputed for constructors.  So it ought to be possible to use thisp as a common temporary that retires to frame.thisp at a single store instruction.

/be

Comment 14

[Andreas]

(In reply to comment #13)
> Note from cvs history how this used to occur in js_ComputeThis.  It should be
> true that thisp is not recomputed for constructors.  So it ought to be possible
> to use thisp as a common temporary that retires to frame.thisp at a single
> store instruction.

Old code assumed that delegation would be possible for constructors:
http://lxr.mozilla.org/classic/source/js/src/jsinterp.c#513

But it used the delegated thisp as default return value for constructors,
not the original one as in current code. This change has been introduced by
version 3.224 (bug 325960):
http://bonsai.mozilla.org/cvsview2.cgi?command=DIFF&subdir=mozilla%2Fjs%2Fsrc&file=jsinterp.c&rev1=3.223&rev2=3.224&whitespace_mode=show&diff_mode=context

I assume that it will not cause problems if we use the potentially modified
thisp as it has been done until 10 weeks ago.

Comment 15

[Brendan Eich [:brendan]]

(In reply to comment #14)
> (In reply to comment #13)
> > Note from cvs history how this used to occur in js_ComputeThis.  It should be
> > true that thisp is not recomputed for constructors.  So it ought to be possible
> > to use thisp as a common temporary that retires to frame.thisp at a single
> > store instruction.
> 
> Old code assumed that delegation would be possible for constructors:
> http://lxr.mozilla.org/classic/source/js/src/jsinterp.c#513

Right, and 'new With' would expose this delegation via the return value of the With constructor.  I view that as a bug ;-).

> But it used the delegated thisp as default return value for constructors,
> not the original one as in current code. This change has been introduced by
> version 3.224 (bug 325960):
> http://bonsai.mozilla.org/cvsview2.cgi?command=DIFF&subdir=mozilla%2Fjs%2Fsrc&file=jsinterp.c&rev1=3.223&rev2=3.224&whitespace_mode=show&diff_mode=context
> 
> I assume that it will not cause problems if we use the potentially modified
> thisp as it has been done until 10 weeks ago.

Better still if we avoid js_ComputeThis for constructor invocation.  This means an added test, though -- unless you see a way to combine with an existing test of (flags & JSINVOKE_CONSTRUCTING).

/be

Comment 16

[Brendan Eich [:brendan]]

(In reply to comment #15)
> Right, and 'new With' would expose this delegation via the return value of the
> With constructor.  I view that as a bug ;-).

Even more amusing, and still an unfixed bug:

js> o = {toString:function(){return "hi there"}}
hi there
js> w = new With(o)
typein:2: Error: cyclic __proto__ value

/be

Comment 17

[Andreas]

Attachment: updated patch

Changes:
- Addressed the comments above
- Skip js_ComputeThis() if flags & JSINVOKE_CONSTRUCT
- Adapted to changes in the tree

Known problem:
Still doesn't use the *original* String/Number/Boolean prototype (now that
the fix for bug 304376 is checked in) to look up methods for primitive
values. But this doesn't seem to work at all (will comment in bug 304376).

Is js_GetClassPrototype(cx, NULL, INT_TO_JSID(JSProto_String), &obj)
supposed to return the *original* String prototype? It does not.

(In reply to comment #15)
> Better still if we avoid js_ComputeThis for constructor invocation.  This means
> an added test, though -- unless you see a way to combine with an existing test
> of (flags & JSINVOKE_CONSTRUCTING).

Skipping js_ComputeThis for constructor invocation is easy. Will also turn
the "cyclic __proto__ value" error into a "deprecated __parent__ usage"
warning (but I don't know what is supposed to happen, so I can't say if
correct behaviour results).

Comment 18

[Brendan Eich [:brendan]]

Now that bug 304376 is fixed (good and hard ;-), I will do whatever is needed to help land an updated patch for this bug.

/be

Comment 19

[Andreas]

Attachment 219038 [details] [diff] is still up to date.
js_GetClassPrototype() works as expected now.

Comment 20

[Brendan Eich [:brendan]]

Attachment: patch i'm about to check in

with all credit to Andreas by first name and email address.  (Besides letting CVS resolve some conflicts, I fiddled spacing in jsfun.h and fixed jsstr.c tagify to handle a string |this| passed in via (jsval)obj.)

Andreas, could you tell me your last name so I can credit you fully?  Also, care to apply for CVS commit access?  I love your work!

/be

Comment 21

[Brendan Eich [:brendan]]

Fix committed.  Looking for gains on Tinderbox performance metrics (and no orange or red, my favorite colors yesterday :-/).  Thanks again, Andreas.

/be

Comment 22

[timeless]

this added a considerable number of warning output to the builds because you changed JSFunctionSpec but didn't change the consumers.

Comment 23

[Igor Bukanov]

(In reply to comment #22)
> this added a considerable number of warning output to the builds because you
> changed JSFunctionSpec but didn't change the consumers.
> 

See bug 338678.

Comment 24

[Brendan Eich [:brendan]]

Attachment: very tardy comment fix to jsnum.h and jsstr.h

For posterity, in case this is wanted on a 1.8 branch.

/be