-
Notifications
You must be signed in to change notification settings - Fork 385
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve performance of Promises #383
Conversation
b024972
to
33ec9b4
Compare
// This is faster when TypeIsObject may commonly return false. | ||
return val === void 0 || val === null || val === true || val === false || | ||
typeof val === 'string' || typeof val === 'number' || | ||
typeof val === 'symbol'; // jshint ignore:line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
symbol
is a valid typeof value, so rather than the jshint comment, let's modify the project config to not complain about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
Thanks, this looks great! I have a number of minor comments but after those and a fresh rebase, this should be good to go. |
@@ -291,6 +291,13 @@ | |||
return x != null && Object(x) === x; | |||
}, | |||
|
|||
TypeIsNotObject: function (val) { | |||
// This is faster when TypeIsObject may commonly return false. | |||
return val === void 0 || val === null || val === true || val === false || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think return val == null || (typeof val !== 'object' && typeof val !== 'function')
is the simplest here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
val == null
is actually slower than an explicit test against null
and undefined
, when you look at v8's optimized assembly output. The form shown here was pretty extensively benchmarked. It might look a little weird, but it's pretty straightforward to read (a plus) and happens to be the fastest option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about return val === void 0 || val === null || (typeof val !== 'object' && typeof val !== 'function')
? How much faster are we talking about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my main concern is that new primitive typeof values (which there will likely be many) will require a whitelist approach to be continually updated, whereas a blacklist approach will work essentially forever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the key to the optimization is that undefined
, null
, true
, false
etc are all actually constant pointers which can be compared directly without dereferencing. Similarly number
is a tagged value (not a NaN) so it can also be a direct comparison. That makes those tests extremely fast if they return true. We only have to actually dereference for string
and symbol
(and I might be wrong about those, likely there are some optimizations for string) -- which is equivalent to the two dereferences needed for object
and function
in your case, except you've lost the fast cases for booleans and numbers.
Of course, this is only if the argument is completely unknown. If the function is inlined with a known type this compiles out to a constant in any case.
(There might be a special weirdness about typeof val === 'function'
-- when looking at generated code, v8 doesn't seem to do the same type-tracking of function types that it does for object types. I don't know if that actually comes into play in this case or not.)
(Also, looking at the current spec text, it says "The ECMAScript language types are Undefined, Null, Boolean, String, Symbol, Number, and Object." So the current version of this test tracks the spec more closely than your variant -- there's a case for each of the listed "ECMAScript language types". The typeof x === 'function'
case is a JS oddity you have to look much further down the spec document to understand -- and your version doesn't handle the last case in http://www.ecma-international.org/ecma-262/6.0/#sec-typeof-operator-runtime-semantics-evaluation correctly.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, let's do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the refactor here: 8dc13d7
The only "slowdown" should be in the case of when it is an object, in which case, it's two extra string comparisons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Factoring out the typeof x
might actually lead to a slowdown. typeof x == <string constant>
is optimized as a complete expression, breaking it up might defeat the optimization. I'll check that. (There are no actual string comparisons performed after the optimization.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Incidentally, http://mrale.ph/irhydra/2/ is a relatively-friendly tool that lets you see the compiled/optimized assembly.)
33ec9b4
to
d0ac45a
Compare
Ok, rebased with fixes from your comments applied. |
@cscott crap, sorry, I cherry picked two of your commits and pushed them up not realizing you'd already done the rebase. Would you mind one more rebase? :-) if not i'll merge manually |
We were previously applying TypeIsObject to non-objects, which was very slow. But it turns out that even when applied to object arguments, the explicit `typeof` test used here is faster.
…t path. This optimization greatly increases performance and reduces memory use in the common case where a Promise is only chained a single time via Promise#then.
This helps the optimizer determine the proper "object shape" for capability objects.
…ise. When the return value of Promise#then is ignored, we can save time and memory by never creating it.
d0ac45a
to
4608c65
Compare
rebased, no problem. |
[Performance] Improve performance of Promises
These patches improve the performance of Promises by roughly a factor of 2x, resulting in performance on the
doxbee sequential
benchmark equivalent to thecore-js
Promise implementation and within a factor of two of v8's native Promise implementation. It also results in about 50% less memory usage on that benchmark than eithercore-js
or v8 native Promises.The improvements to TypeIsObject and IsCallable also improve the performance of other parts of the shims.