749536 - Can we speed up JS_ValueToBoolean

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Looking at a profile of some WebGL stuff, the JS_ValueToBoolean to deal with boolean arguments is somewhere around 6% of total time.  A good part of this is the overhead of two function calls (to JS_ValueToBoolean and thence to js_ValueToBoolean).  Specifically, 2.6% of the time is in JS_ValueToBoolean itself, and there's nothing there _except_ call overhead!

Assuming we don't want to inline js_ValueToBoolean itself, could we at least move the guts into an inline function and call _that_ from JS_ValueToBoolean?

But even better would be a somewhat better API that doesn't involve out params for an infallible function.  Possibly with either the whole thing or some sort of common case (in my situation, the common case is isBoolean(), actually) inlined.

Comment 1

[Jeff Walden [:Waldo]]

We should do a JS::ToBoolean that returns bool, I think.  I might go all the way and even define it inline so the compiler can optionally be smart, but I guess that'd be up to perf-testing to evaluate.

Comment 2

[Boris Zbarsky [:bzbarsky]]

We can start with the bool return value and just the one function call, and I can tell you how much time that saves, if desired... ;)

Inlining is generally a perf win, though, for my stuff, because just having that cross-library call already costs.

Comment 3

[Eric Faust [:efaust]]

Attachment: Add ToBoolean()

Boris: using this looks like it drops xhr.withCredentials = false; runtimes by 25%.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Boris: using this looks like it drops xhr.withCredentials = false; runtimes by 25%.

profilers++ and efaust++.  ;)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 645152 [details] [diff] [review]
Add ToBoolean()

>   static inline bool converter(JSContext* cx, JS::Value v, jstype* retval) {
>+    (void) cx;

I think I'd prefer:

  static inline bool converter(JSContext* /* unused */, JS::Value v,
                               jstype* retval) {

if we just want to avoid unused-argument warnings.

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 645152 [details] [diff] [review]
Add ToBoolean()

Review of attachment 645152 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/PrimitiveConversions.h
@@ +80,5 @@
>  struct PrimitiveConversionTraits<bool> {
>    typedef JSBool jstype;
>    typedef bool intermediateType;
>    static inline bool converter(JSContext* cx, JS::Value v, jstype* retval) {
> +    (void) cx;

Could you instead just remove the name of the 'cx' parameter?

::: js/src/jsapi.h
@@ +2573,5 @@
>  extern JS_PUBLIC_API(bool)
>  ToNumberSlow(JSContext *cx, JS::Value v, double *dp);
> +
> +extern JS_PUBLIC_API(bool)
> +ValueToBoolean(const JS::Value &v);

For symmetry, can you name it ToBooleanSlow?

@@ +2597,5 @@
> +ToBoolean(const Value &v)
> +{
> +    if (v.isBoolean()) {
> +        return v.toBoolean();
> +    }

No braces on single-line then.

Also, unlike ToString and ToNumber, I suspect that code might actually pass non-primitive booleans.  Is there a some code we could measure to see what % of JS_ValueToBoolean calls actually pass a bool?  Integer, null, undefined and object seem like they could common.

Comment 7

[Eric Faust [:efaust]]

Attachment: Comments addressed.

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 645466 [details] [diff] [review]
Comments addressed.

Review of attachment 645466 [details] [diff] [review]:
-----------------------------------------------------------------

I see the patch is for IM.  To minimize the IM-VM diff, I think it would be good to land this on m-c and just fix up the few IM-specific uses when m-c merges (they are pretty simple).

::: js/src/jsapi.cpp
@@ +466,5 @@
>          if (ok)
>              *vp = DOUBLE_TO_JSVAL(d);
>          break;
>        case JSTYPE_BOOLEAN:
> +        *vp = BOOLEAN_TO_JSVAL(ToBoolean(v));

Since you are touching, BooleanValue(ToBoolean(v)).

::: js/src/jsapi.h
@@ +2610,5 @@
> +    if (v.isDouble()) {
> +        double d;
> +
> +        d = v.toDouble();
> +        return !MOZ_DOUBLE_IS_NaN(d) && d != 0;

double d = v.toDouble();

::: js/src/jsinterp.cpp
@@ -908,2 @@
>          } else {                                                              \
> -            b = !!js_ValueToBoolean(*vp);                                     \

Looks like you can remove vp->isNull() as well.  Given that, all we really have is:
  b = ToBoolean(regs.sp[-1]));
Can you kill the macro?

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/04e144fd16fe