Bug 69320 - Some JSValue cleanup
Summary: Some JSValue cleanup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-03 23:13 PDT by Geoffrey Garen
Modified: 2011-10-04 19:37 PDT (History)
2 users (show)

See Also:


Attachments
Patch (44.91 KB, patch)
2011-10-03 23:29 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Patch (45.14 KB, patch)
2011-10-04 12:05 PDT, Geoffrey Garen
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2011-10-03 23:13:56 PDT
Some JSValue cleanup
Comment 1 Geoffrey Garen 2011-10-03 23:29:07 PDT
Created attachment 109584 [details]
Patch
Comment 2 Darin Adler 2011-10-04 09:38:59 PDT
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.
Comment 3 Darin Adler 2011-10-04 09:39:30 PDT
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.
Comment 4 Geoffrey Garen 2011-10-04 11:41:33 PDT
> 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.
Comment 5 Darin Adler 2011-10-04 11:44:21 PDT
(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.
Comment 6 Geoffrey Garen 2011-10-04 12:03:54 PDT
> Is there a need to include the “std::” here?

Yes, since it's our custom not to put using declarations in headers.
Comment 7 Geoffrey Garen 2011-10-04 12:05:13 PDT
Created attachment 109661 [details]
Patch
Comment 8 Geoffrey Garen 2011-10-04 12:06:06 PDT
(Added explanation to ChangeLog, renamed "v" to "jsValue".)
Comment 9 Darin Adler 2011-10-04 12:19:29 PDT
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.
Comment 10 Geoffrey Garen 2011-10-04 17:12:06 PDT
> > 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.
Comment 11 Geoffrey Garen 2011-10-04 17:19:58 PDT
> > 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.
Comment 12 Geoffrey Garen 2011-10-04 19:37:46 PDT
Committed r96673: <http://trac.webkit.org/changeset/96673>