Persona is no longer an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 749536 - Can we speed up JS_ValueToBoolean
: Can we speed up JS_ValueToBoolean
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Eric Faust [:efaust]
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: ParisBindings
  Show dependency treegraph
Reported: 2012-04-27 00:08 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2012-07-25 08:09 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Add ToBoolean() (22.12 KB, patch)
2012-07-23 17:55 PDT, Eric Faust [:efaust]
no flags Details | Diff | Splinter Review
Comments addressed. (22.04 KB, patch)
2012-07-24 13:31 PDT, Eric Faust [:efaust]
luke: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2012-04-27 00:08:21 PDT
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] (remove +bmo to email) 2012-04-27 00:21:50 PDT
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 [:bz] (still a bit busy) 2012-04-27 00:34:51 PDT
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] 2012-07-23 17:55:27 PDT
Created attachment 645152 [details] [diff] [review]
Add ToBoolean()

Boris: using this looks like it drops xhr.withCredentials = false; runtimes by 25%.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2012-07-23 18:35:08 PDT
> Boris: using this looks like it drops xhr.withCredentials = false; runtimes by 25%.

profilers++ and efaust++.  ;)
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2012-07-23 18:37:34 PDT
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] 2012-07-23 18:42:16 PDT
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] 2012-07-24 13:31:37 PDT
Created attachment 645466 [details] [diff] [review]
Comments addressed.
Comment 8 Luke Wagner [:luke] 2012-07-24 14:25:38 PDT
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 (Away 28th Oct -> 6th Nov) [:emorley] 2012-07-25 08:09:32 PDT

Note You need to log in before you can comment on or make changes to this bug.