Closed Bug 749536 Opened 10 years ago Closed 9 years ago

Can we speed up JS_ValueToBoolean


(Core :: JavaScript Engine, defect)

Not set





(Reporter: bzbarsky, Assigned: efaust)


(Blocks 1 open bug)



(1 file, 1 obsolete file)

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.
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.
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.
Attached patch Add ToBoolean() (obsolete) — Splinter Review
Boris: using this looks like it drops xhr.withCredentials = false; runtimes by 25%.
Assignee: general → efaust
Attachment #645152 - Flags: review?(luke)
> Boris: using this looks like it drops xhr.withCredentials = false; runtimes by 25%.

profilers++ and efaust++.  ;)
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 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.
Attachment #645152 - Flags: review?(luke)
Attachment #645152 - Attachment is obsolete: true
Attachment #645466 - Flags: review?(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?
Attachment #645466 - Flags: review?(luke) → review+
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.