Bug 69320 – Some JSValue cleanup

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geoffrey Garen]

Some JSValue cleanup

[Geoffrey Garen]

Created attachment 109584 [details]
Patch

[Darin Adler]

Comment on attachment 109584 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109584&action=review

The assumption in the old code was that getNumber and getBoolean were more efficient than would be possible for separate isNumber/uncheckedGetNumber calls and the same for boolean. Did that turn out not to be true?

> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:486
> +            JSValue v = toJS(exec, value);

Not a huge fan of “v” in a function that also has another local named “value”.

> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:488
> +                return std::numeric_limits<double>::quiet_NaN();

Is there a need to include the “std::” here?

Is there no function that returns NaN if something is not a number? Seems a rather common case.

[Darin Adler]

Despite the fact that I started making some comments, anyone else is welcome to review this. I was planning to do so but not right now.

[Geoffrey Garen]

> The assumption in the old code was that getNumber and getBoolean were more efficient than would be possible for separate isNumber/uncheckedGetNumber calls and the same for boolean. Did that turn out not to be true?

get* used to be an optimization when every value operation was a virtual function call: get* would combine two virtual calls into one. Now, with non-virtual, inlined functions, get* isn't faster, and may be slightly slower.

> Is there no function that returns NaN if something is not a number? Seems a rather common case.

NaN is uncommon. Typically, JS operations will convert to number rather that producing NaN.

[Darin Adler]

(In reply to comment #4)
> > The assumption in the old code was that getNumber and getBoolean were more efficient than would be possible for separate isNumber/uncheckedGetNumber calls and the same for boolean. Did that turn out not to be true?
> 
> get* used to be an optimization when every value operation was a virtual function call: get* would combine two virtual calls into one. Now, with non-virtual, inlined functions, get* isn't faster, and may be slightly slower.

Sounds good. The kind of thing that the change log should state, since it’s key to the motivation behind the patch and explains why this wasn’t done before.

[Geoffrey Garen]

> Is there a need to include the “std::” here?

Yes, since it's our custom not to put using declarations in headers.

[Geoffrey Garen]

Created attachment 109661 [details]
Patch

[Geoffrey Garen]

(Added explanation to ChangeLog, renamed "v" to "jsValue".)

[Darin Adler]

Comment on attachment 109661 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109661&action=review

> Source/JavaScriptCore/interpreter/Interpreter.cpp:4052
> +        else if (scrutinee.isDouble() && scrutinee.asDouble() == static_cast<int32_t>(scrutinee.asDouble()))
> +            vPC += codeBlock->immediateSwitchJumpTable(tableIndex).offsetForValue(static_cast<int32_t>(scrutinee.asDouble()), defaultOffset);

Do the repeated scrutinee.asDouble() and static_cast<int32_t>(scrutinee.asDouble()) expressions get optimized as we would wish? I noticed you kept the local variables in another case like this elsewhere in the patch.

> Source/JavaScriptCore/runtime/JSValueInlineMethods.h:399
>          return asValue() == jsBoolean(true);

isTrue() above uses JSValue(JSTrue) while this uses jsBoolean(true). Is there an advantage to one over the other? If not, I suggest unifying.

> Source/JavaScriptCore/runtime/NumberPrototype.cpp:462
> +        unsigned c = static_cast<unsigned>(x);
> +        if (c == x && c < 36) {

In the old code path, this would all have been done without ever converting to a double in the case where the value was an Int32.

But if we are really serious about optimizing radix 36 then I think we should probably do something less general to get the value of the radix too, maybe even some inlining.

[Geoffrey Garen]

> > Source/JavaScriptCore/interpreter/Interpreter.cpp:4052
> > +        else if (scrutinee.isDouble() && scrutinee.asDouble() == static_cast<int32_t>(scrutinee.asDouble()))
> > +            vPC += codeBlock->immediateSwitchJumpTable(tableIndex).offsetForValue(static_cast<int32_t>(scrutinee.asDouble()), defaultOffset);
> 
> Do the repeated scrutinee.asDouble() and static_cast<int32_t>(scrutinee.asDouble()) expressions get optimized as we would wish? I noticed you kept the local variables in another case like this elsewhere in the patch.

I used to have a superstition about reading values out of unions, but whatever compiler version caused that to be an optimization problem, it seems to have passed. I just looked at some generated code for clang++ 3.0, and it optimized perfectly. It's possible that local variables in other cases can be removed.

[Geoffrey Garen]

> > Source/JavaScriptCore/runtime/JSValueInlineMethods.h:399
> >          return asValue() == jsBoolean(true);
> 
> isTrue() above uses JSValue(JSTrue) while this uses jsBoolean(true). Is there an advantage to one over the other? If not, I suggest unifying.

In theory, they should all compile down to the same bits, but the isTrue() way of doing this is more direct, so I switched to it.

[Geoffrey Garen]

Committed r96673: <http://trac.webkit.org/changeset/96673>