409476 - Unnecessary JSBool-returning, unused-JSContext*-argument function witch hunt

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Bug 409324 comment 7 points out js_ValueToBoolean; I suspect there are probably more but have no evidence to back up this belief.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: js_ValueToBoolean mods

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 294295 [details] [diff] [review]
js_ValueToBoolean mods

>+        b = js_ValueToBoolean(FETCH_OPND(-1));                                \

Er, this was stupid -- should have had a |v = | before the FETCH_OPND, or better yet just left it on the previous line.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 294295 [details] [diff] [review]
js_ValueToBoolean mods

Note that in the antediluvian past, converting to boolean could call a method, so the whole cx first param and ok r.v. were obligatory. Those days will never return, sigh (this means any object that can convert to false must be magic to the runtime -- see class boolean in the ES4 RI).

> Boolean(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
> {
>-    JSBool b;
>     jsval bval;
> 
>-    if (argc != 0) {
>-        if (!js_ValueToBoolean(cx, argv[0], &b))
>-            return JS_FALSE;
>-        bval = BOOLEAN_TO_JSVAL(b);
>-    } else {
>+    if (argc != 0)
>+        bval = BOOLEAN_TO_JSVAL(js_ValueToBoolean(argv[0]));
>+    else
>         bval = JSVAL_FALSE;

This should use ?: now to common the bval= LHS.

>+js_ValueToBoolean(jsval v)
> {
>-    JSBool b;
>-    jsdouble d;
>-
>-    if (JSVAL_IS_NULL(v) || JSVAL_IS_VOID(v)) {
>-        b = JS_FALSE;
>-    } else if (JSVAL_IS_OBJECT(v)) {
>-        b = JS_TRUE;
>-    } else if (JSVAL_IS_STRING(v)) {
>-        b = JSSTRING_LENGTH(JSVAL_TO_STRING(v)) ? JS_TRUE : JS_FALSE;
>-    } else if (JSVAL_IS_INT(v)) {
>-        b = JSVAL_TO_INT(v) ? JS_TRUE : JS_FALSE;
>-    } else if (JSVAL_IS_DOUBLE(v)) {
>-        d = *JSVAL_TO_DOUBLE(v);
>-        b = (!JSDOUBLE_IS_NaN(d) && d != 0) ? JS_TRUE : JS_FALSE;
>-    } else {
>-        JS_ASSERT(JSVAL_IS_BOOLEAN(v));
>-        b = JSVAL_TO_BOOLEAN(v);
>+    if (JSVAL_IS_NULL(v) || JSVAL_IS_VOID(v))
>+        return JS_FALSE;
>+    if (JSVAL_IS_OBJECT(v))
>+        return JS_TRUE;
>+    if (JSVAL_IS_STRING(v))
>+        return JSSTRING_LENGTH(JSVAL_TO_STRING(v)) ? JS_TRUE : JS_FALSE;

Pre-existing code, I know, but verbose and (if the compiler is weak) sub-optimal. Just return JSSTRING_LENGTH(...) != 0 here. See below for the pre-existing counter-example.

>+    if (JSVAL_IS_INT(v))
>+        return JSVAL_TO_INT(v) ? JS_TRUE : JS_FALSE;

Likewise, return JSVAL_TO_INT(v) != 0.

>+    if (JSVAL_IS_DOUBLE(v)) {
>+        jsdouble d = *JSVAL_TO_DOUBLE(v);
>+        return !JSDOUBLE_IS_NaN(d) && d != 0;

Here's the right way, already there.

>+++ jsinterp.c	21 Dec 2007 23:08:59 -0000
>@@ -211,27 +211,17 @@
>             ok = js_ValueToNumber(cx, v, &d);                                 \
>             if (!ok)                                                          \
>                 goto out;                                                     \
>         }                                                                     \
>     JS_END_MACRO
> 
> #define POP_BOOLEAN(cx, v, b)                                                 \
>     JS_BEGIN_MACRO                                                            \
>-        v = FETCH_OPND(-1);                                                   \
>-        if (v == JSVAL_NULL) {                                                \
>-            b = JS_FALSE;                                                     \
>-        } else if (JSVAL_IS_BOOLEAN(v)) {                                     \
>-            b = JSVAL_TO_BOOLEAN(v);                                          \
>-        } else {                                                              \
>-            SAVE_SP_AND_PC(fp);                                               \
>-            ok = js_ValueToBoolean(cx, v, &b);                                \
>-            if (!ok)                                                          \
>-                goto out;                                                     \
>-        }                                                                     \
>+        b = js_ValueToBoolean(FETCH_OPND(-1));                                \
>         sp--;                                                                 \
>     JS_END_MACRO

On modern hardware this shouldn't matter, but it might: predicted branch and no spills plus common branch opcodes popping ready-made booleans (or nulls) will now go through a function call. Do some speed tests, see if you want to keep the inline if-else optimizations here.

> xml_setting_setter(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
> {
>     JSBool b;
>     uint8 flag;
> 
>     JS_ASSERT(JSVAL_IS_INT(id));
>-    if (!js_ValueToBoolean(cx, *vp, &b))
>-        return JS_FALSE;
>+    b = js_ValueToBoolean(*vp);
> 
>     flag = JS_BIT(JSVAL_TO_INT(id));
>     if (b)
>         cx->xmlSettingFlags |= flag;
>     else
>         cx->xmlSettingFlags &= ~flag;
>     return JS_TRUE;

Note that b is now single-use, so eliminate as you do elsewhere in jsxml.c.

/be

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Better patch

It depresses me greatly that removing the ifs from POP_BOOLEAN increases sunspider runtime by 100ms, with consistent slowdowns across the board in nearly all tests.

This patch is 20ms faster than current trunk, although changes in individual portions of sunspider are mixed: some are speedups, some are slowdowns.  Test runtimes are short enough it's not clear they're not just test-specific fallout, tho.

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)
> Created an attachment (id=294322) [details]
> Better patch
> 
> It depresses me greatly that removing the ifs from POP_BOOLEAN increases
> sunspider runtime by 100ms, with consistent slowdowns across the board in
> nearly all tests.

Offline compilers without whole-program or profile guidance are like that.

> This patch is 20ms faster than current trunk, although changes in individual
> portions of sunspider are mixed: some are speedups, some are slowdowns.  Test
> runtimes are short enough it's not clear they're not just test-specific
> fallout, tho.

You mean test-specific noise, or something related to this patch?

Comments next.

/be

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 294322 [details] [diff] [review]
Better patch

>+    bval = (argc != 0)
>+           ? BOOLEAN_TO_JSVAL(js_ValueToBoolean(argv[0]))
>+           : JSVAL_FALSE;

Since Boolean is defined as a function with nargs of 1 (see the JS_InitClass call in js_InitBooleanClass) you don't even need the ?: predicated on argc -- just use js_ValueToBoolean(argv[0]) unconditionally.

> #define POP_BOOLEAN(cx, v, b)                                                 \
>     JS_BEGIN_MACRO                                                            \
>         v = FETCH_OPND(-1);                                                   \
>+        if (JSVAL_IS_BOOLEAN(v))                                              \
>             b = JSVAL_TO_BOOLEAN(v);                                          \
>+        else if (v == JSVAL_NULL)                                             \
>+            b = JS_FALSE;                                                     \
>+        else                                                                  \
>+            b = js_ValueToBoolean(v);                                         \

(- lines removed)

You could add || v == JSVAL_VOID in the same condition as v == JSVAL_NULL to match js_ValueToBoolean's testing, and to catch fairly common (in my experience) tests of undefined vars via 'if (v) ...'. 

Since there's a common LHS in all the then and else clauses, could use

        b = JSVAL_IS_BOOLEAN(v)
            ? JSVAL_TO_BOOLEAN(v)
            : (JSVAL_IS_NULL(v) || JSVAL_IS_VOID(v))
            ? JS_FALSE
            : js_ValueToBoolean(v);

instead (note JSVAL_IS_{NULL,VOID} macros too).

r+a=me with these changes, and provided the Sunspider variations are noise.

/be

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Clean SpiderMonkey vs. patched SpiderMonkey comparison

There are clearly big gains here some places: nbody executes in 9.5% less time (!, I don't see anything obvious at a glance), a bunch of 1-3% speedups, a bunch of <1% changes, several 2-3% slowdowns.  It's a mixed bag individually, definite speedup overall.  I think we'll need finer grain info to be able to do no worse on specific tests, and certainly there are better gains to be made doing something other than the argument-gimmicking or comparison-ordering happening here.

Comment 8

[Jeff Walden [:Waldo]]

Attachment: With last nits

Comment 9

[Robert Sayre]

Attachment: comparison

I see overall improvement, but no big gains, and also some losses.

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 294324 [details] [diff] [review]
With last nits

>+    if (JSVAL_IS_BOOLEAN(v))
>+        return v == JSVAL_TRUE;
>+    if (JSVAL_IS_NULL(v) || JSVAL_IS_VOID(v))
>+        return JS_FALSE;
>+    if (JSVAL_IS_OBJECT(v))
>+        return JS_TRUE;
>+    if (JSVAL_IS_STRING(v))
>+        return JSSTRING_LENGTH(JSVAL_TO_STRING(v)) != 0;
>+    if (JSVAL_IS_INT(v))
>+        return v != JSVAL_ZERO;
>+    JS_ASSERT(JSVAL_IS_DOUBLE(v));
>+    {
>+        jsdouble d = *JSVAL_TO_DOUBLE(v);
>+        return !JSDOUBLE_IS_NaN(d) && d != 0;
>     }

Nit: not prevailing style to use a block like that -- could put d at body scope, uninitialized. But this raises a bigger issue:

For completeness/bonus-points, let's measure dynamic frequency of types (as classified by this if/else) seen here, and the subset tested in VALUE_TO_BOOLEAN in jsinterp.c. It would be interested to get both histograms, dumped from each GC to a /tmp/bool.hist file. Separate patch for the ad-hoc instrumentation should do.

SunSpider is not perfect, but getting its histograms, and comparing with a random gmail + bugzilla + surfing long-lived session's histograms could give us more confidence that we have the right test orders. Now we're just going on gut checks and my old code.

/be

Comment 11

[Brendan Eich [:brendan]]

#define TYPEOF(cx,v)    (JSVAL_IS_NULL(v) ? JSTYPE_NULL : JS_TypeOfValue(cx,v))

    ++js_v2b_hist[TYPEOF(cx, v)];

in js_ValueToBoolean, and

        ++V2B_hist[TYPEOF(cx, v)];                                            \

in VALUE_TO_BOOLEAN, both at the top, plus extern decls, static FILE * opened once, and fprintfs in jsGC near other such crud...

/be

Comment 12

[Brendan Eich [:brendan]]

Forgot

#define JSTYPE_NULL     JSTYPE_LIMIT

and dimension the histogram arrays [JSTYPE_LIMIT+1].

/be

Comment 13

[Brendan Eich [:brendan]]

Never mind the last comment, we added JSTYPE_NULL at some point -- JSTYPE_LIMIT is the dimension to use, and JS_TYPE_STR(type) gives the const char * to use in those fprintfs.

/be

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Change the signature, leave nitting for a different bug

We could go around in circles micro-optimizing this; I think we should just change the signature and leave perf tweaks for a new bug, where we can disambiguate improvements from the signature change from improvements in type-comparison ordering.  Simple improvements should not be stop-energied into massive amounts of pain.

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 298022 [details] [diff] [review]
Change the signature, leave nitting for a different bug

Didn't mean to stop-energy anyone into stopping, sorry for having that effect. I've got a bug on jsval tagging, it'll cover test order if other bugs don't. Thanks for patching.

/be

Comment 16

[Igor Bukanov]

The checked in patch triggered a series of regressions in the test suite:

~/m/ff/mozilla/js/tests $ ./jsDriver.pl -e smdebug  -l ecma/Boolean/15.6.4.2-2.js -t
-*- getting test list from user specified source.
-*- Testing engine 'smdebug'
-*- getting spidermonkey engine command.
-*- got './../src/Linux_All_DBG.OBJ/js '
-#- Executing 1 test(s).
-*- executing: ./../src/Linux_All_DBG.OBJ/js  -f ./shell.js -f ./ecma/shell.js -f ./ecma/Boolean/shell.js -f ./ecma/Boolean/15.6.4.2-2.js -f ./js-test-driver-end.js
*-* Testcase ecma/Boolean/15.6.4.2-2.js failed:
Expected exit code 0, got 0
Testcase terminated with signal 5
Complete testcase output was:
Assertion failure: JSVAL_IS_BOOLEAN(v), at jsbool.c:98

-*- exit code 0, exit signal 5.
-*- Writing output to results-2008-01-20-023754-smdebug.html.
-#- Wrote failures to 'results-2008-01-20-023754-smdebug-failures.txt'.
-#- Wrote results to 'results-2008-01-20-023754-smdebug.html'.
-#- 1 test(s) failed

Comment 17

[Igor Bukanov]

Comment on attachment 298022 [details] [diff] [review]
Change the signature, leave nitting for a different bug

>Index: jsbool.c
>===================================================================
> static JSBool
> Boolean(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
> {
>-    JSBool b;
>     jsval bval;
> 
>-    if (argc != 0) {
>-        if (!js_ValueToBoolean(cx, argv[0], &b))
>-            return JS_FALSE;
>-        bval = BOOLEAN_TO_JSVAL(b);
>-    } else {
>-        bval = JSVAL_FALSE;
>-    }
>+    bval = (argc != 0)
>+           ? BOOLEAN_TO_JSVAL(js_ValueToBoolean(argv[0]))
>+           : JS_FALSE;

That has to be JSVAL_FALSE, not JS_FALSE! Or use

>+    bval = BOOLEAN_TO_JSVAL(argc != 0 && js_ValueToBoolean(argv[0]));


> JSBool
>-js_ValueToBoolean(JSContext *cx, jsval v, JSBool *bp)
>+js_ValueToBoolean(jsval v)
> {
>-    JSBool b;
>-    jsdouble d;
>+    if (JSVAL_IS_NULL(v) || JSVAL_IS_VOID(v))
>+        return JS_FALSE;
>+    if (JSVAL_IS_OBJECT(v))
>+        return JS_TRUE;
>+    if (JSVAL_IS_STRING(v))
>+        return JSSTRING_LENGTH(JSVAL_TO_STRING(v)) != 0;
>+    if (JSVAL_IS_INT(v))
>+        return JSVAL_TO_INT(v) != 0;
>+    if (JSVAL_IS_DOUBLE(v)) {
>+        jsdouble d;

Maybe a better approach would be to switch over the value tag?

Comment 18

[Jeff Walden [:Waldo]]

(In reply to comment #17)
> That has to be JSVAL_FALSE, not JS_FALSE!

Blah; fixed.

> Maybe a better approach would be to switch over the value tag?

Yeah, I was leaning that way, but I wanted to make it easier to distinguish perf changes from that from ones just due to reducing args, etc.

Comment 19

[Igor Bukanov]

(In reply to comment #18)
> (In reply to comment #17)
> > That has to be JSVAL_FALSE, not JS_FALSE!
> 
> Blah; fixed.

I have spotted it when running the test suite testing a patch from another bug. I run the tests in the following way:

1. After updating js/tests I generate the list of the base failures using a session like:

~/m/ff/mozilla/js/tests $ ./jsDriver.pl -e smdebug -L slow-n.tests performance.tests
-#- Executing 2446 test(s).
-#- Wrote failures to 'results-2008-01-19-162251-smdebug-failures.txt'.
-#- Wrote results to 'results-2008-01-19-162251-smdebug.html'.
~/m/ff/mozilla/js/tests $ mv results-2008-01-19-162251-smdebug-failures.txt base-failures.txt

2. I run the test suite with a patch applied excluding the base  failures:

~/m/ff/mozilla/js/tests $ ./jsDriver.pl -e smdebug -L slow-n.tests performance.tests base-failures.txt
-#- Executing 2248 test(s).
-#- Wrote results to 'results-2008-01-20-121132-smdebug.html'.

3. The above runs for 5 mintes on my relatively slow notebook. If there are any regressions, the test driver will report a line like:

-#- Wrote failures to 'results-2008-01-20-021649-smdebug-failures.txt'. 

Then this is the file to look for.

Comment 20

[Jeff Walden [:Waldo]]

Yeah; I remember running the test suite when I wrote the most recent version of the patch, and I basically hadn't touched the tree in which I kept the changes since then (excepting some changes to jsopcode.c for the object-decompilation bug before deciding a new tree was in order, which I carefully reverted by referring to cvs diff output).  I then made the requested changes and posted the patch; I don't have any idea how that change managed to creep into the patch.

Comment 21

[Brian Crowder]

Comment on attachment 298022 [details] [diff] [review]
Change the signature, leave nitting for a different bug

Waiting for post-1.9

Comment 22

[Brian Crowder]

This was landed in mozilla-central, and also seems to have landed in CVS (Jan 19, 2008):

http://hg.mozilla.org/mozilla-central/index.cgi/rev/dcf463537f43

Comment 23

[Brian Crowder]

We need another bug on the nits described in earlier comments, hopefully Jeff can handle that when he returns.