384151 - Consolidating primitive-to-object conversions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

[This is a spin-off of a patch from bug 382165 to detach the refactoring from GC-safety changes]

Currently in few places SM contains code like:

if (ISVAL_IS_STRING(v))
  obj = js_StringToObject(JSVAL_TO_STRING(v));
else if if (ISVAL_IS_BOOLEAN(v))
  obj = js_BooleanToObject(JSVAL_TO_BOOLEAN(v));
else if if (ISVAL_IS_INT(v))
  obj = js_NumberToObject(JSVAL_TO_INT(v));
else if if (ISVAL_IS_DOUBLE(v))
  obj = js_NumberToObject(*JSVAL_TO_DOUBLE(v));

It would be nice to consolidate all such fragments behind a single function which can also share the code between  js_SomethigToObject boxing function as all such functions are essentially the same.

The new function should also avoid allocation of new jsdouble GC thing which the current code does whenever it boxes non-int double.

Comment 1

[Igor Bukanov]

Attachment: implementation v1

The patch adds js_PrimitiveToObject that replaces all js_SomethingToObject functions. To simplify code farther the patch changes js_ComputeThis to call js_PrimitiveToObject and adjusts js_ComputeThis accordingly.

The patch also removes JSObject **objp argument from js_GetArgsProperty as the interpreter no longer uses the returned object since implementing bug 363530.

Comment 2

[Igor Bukanov]

Comment on attachment 268088 [details] [diff] [review]
implementation v1

Slightly better patch will follow.

Comment 3

[Igor Bukanov]

Attachment: implementation v2

The new version replaces the switch in js_Invoke via a flag table for smaller code.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 268094 [details] [diff] [review]
implementation v2

Looks good, except where relevant minor comments from bug 382165 comment 4 have not been addressed.

r=me with those fixed.

/be

Comment 5

[Igor Bukanov]

(In reply to comment #4)
>...
> relevant minor comments from bug 382165 comment 4 have
> not been addressed.

Sorry about this, a new patch is comming.

Comment 6

[Igor Bukanov]

Attachment: implementation v2b

Addressing the missed nits.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 268211 [details] [diff] [review]
implementation v2b


>+        } else if (!JSVAL_IS_OBJECT(vp[1])) {
>+            /*
>+             * We check if the function accepts a primitive value as |this|.
>+             * For that we use a table that maps value's tag into the
>+             * corresponding function flag.
>+             */
>+            uintN flagToTest;

Nit: blank line here.

>+            JS_STATIC_ASSERT(JSVAL_INT == 1);
>+            JS_STATIC_ASSERT(JSVAL_DOUBLE == 2);
>+            JS_STATIC_ASSERT(JSVAL_STRING == 4);
>+            JS_STATIC_ASSERT(JSVAL_BOOLEAN == 6);
>+            static const uint16 PrimitiveTestFlags[] = {
>+                JSFUN_THISP_NUMBER,     /* INT     */
>+                JSFUN_THISP_NUMBER,     /* DOUBLE  */
>+                JSFUN_THISP_NUMBER,     /* INT     */
>+                JSFUN_THISP_STRING,     /* STRING  */
>+                JSFUN_THISP_NUMBER,     /* INT     */
>+                JSFUN_THISP_BOOLEAN,    /* BOOLEAN */
>+                JSFUN_THISP_NUMBER      /* INT     */
>+            };

Nice.


>+    /*
>+     * Initialize the rest of frame, except for sp (set by SAVE_SP later).
>+     *
>+     * To set thisp we use an explicit cast and not JSVAL_TO_OBJECT as vp[1]

Minor nit (helps wrap more nicely): comma after JSVAL_TO_OBJECT, and...

>+     * can be a primitive value here for native functions with

"specified" before "with" (as in JSFunctionSpec).


>+ * For a call with arguments argv including argv[-1] (nominal |this|) and
>+ * argv[-2] (callee) replace null |this| with callee's parent, replace
>+ * primitive values with the corresponding wrpping object and censor

wrapping misspelled -- also could say "equivalent wrapper objects" instead of "corresponding wrapping object" (note plural objects).

r=me with these picked. Thanks,

/be

Comment 8

[Igor Bukanov]

Attachment: implementation v2c

Patch with the latest nits addressed.

Comment 9

[Igor Bukanov]

I ciommitted the patch from comment 8 to the trunk:

Checking in jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.324; previous revision: 3.323
done
Checking in jsbool.c;
/cvsroot/mozilla/js/src/jsbool.c,v  <--  jsbool.c
new revision: 3.26; previous revision: 3.25
done
Checking in jsbool.h;
/cvsroot/mozilla/js/src/jsbool.h,v  <--  jsbool.h
new revision: 3.10; previous revision: 3.9
done
Checking in jsfun.c;
/cvsroot/mozilla/js/src/jsfun.c,v  <--  jsfun.c
new revision: 3.186; previous revision: 3.185
done
Checking in jsfun.h;
/cvsroot/mozilla/js/src/jsfun.h,v  <--  jsfun.h
new revision: 3.36; previous revision: 3.35
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.351; previous revision: 3.350
done
Checking in jsinterp.h;
/cvsroot/mozilla/js/src/jsinterp.h,v  <--  jsinterp.h
new revision: 3.54; previous revision: 3.53
done
Checking in jsnum.c;
/cvsroot/mozilla/js/src/jsnum.c,v  <--  jsnum.c
new revision: 3.79; previous revision: 3.78
done
Checking in jsnum.h;
/cvsroot/mozilla/js/src/jsnum.h,v  <--  jsnum.h
new revision: 3.28; previous revision: 3.27
done
Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.355; previous revision: 3.354
done
Checking in jsobj.h;
/cvsroot/mozilla/js/src/jsobj.h,v  <--  jsobj.h
new revision: 3.65; previous revision: 3.64
done
Checking in jsstr.c;
/cvsroot/mozilla/js/src/jsstr.c,v  <--  jsstr.c
new revision: 3.145; previous revision: 3.144
done
Checking in jsstr.h;
/cvsroot/mozilla/js/src/jsstr.h,v  <--  jsstr.h
new revision: 3.36; previous revision: 3.35
done