Summary: | Some JSValue cleanup | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Geoffrey Garen <ggaren> | ||||||
Component: | New Bugs | Assignee: | Geoffrey Garen <ggaren> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | darin, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Geoffrey Garen
2011-10-03 23:13:56 PDT
Created attachment 109584 [details]
Patch
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. 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. > 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. (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. > Is there a need to include the “std::” here?
Yes, since it's our custom not to put using declarations in headers.
Created attachment 109661 [details]
Patch
(Added explanation to ChangeLog, renamed "v" to "jsValue".) 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. > > 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.
> > 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.
Committed r96673: <http://trac.webkit.org/changeset/96673> |