-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Performance: improve objectWithoutPropertiesLoose
on V8
#16357
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/56543 |
Please change |
Thanks, I've updated the PR. |
On macOS Safari the initial is faster than updated: https://jsbench.me/1hltu3eit5/1 I migrated the benchmark to jsbench.me because the other one pops up click baits. |
sadly those benchmarks are not valid, as v8 is going to optimize some of this code if we are looking at performance
try running it without the extra loop there seem to not be measurable time difference between both pieces of code |
Weid, JSC is usually happy with
Do you have a link to any relevant discussion?
What do you mean by without the loop? Do you have a better benchmark you could share to shed light on the measurements you see? Wrt to the benchmark I have, I added a loop to ensure the function is optimized asap. This helper is what The other aspect that I didn't like is the |
I've built a small benchmark to test on each engine, I can replicate the results you see with JSC. Despite the slowdown on JSC, I'd still argue that this implementation is better as the gains for V8 are more drastic, and more V8 users also means we should give more weight to that segment. Note that with this new implementation, V8 and JSC perform roughly the same, whereas the old one was considerably slower on V8 than on JSC. But if you have more details on those upcoming V8 changes, please do share, I'd be happy to learn more.
|
Thanks! helpers.objectWithoutPropertiesLoose = helper("7.0.0-beta.0")`
export default function _objectWithoutPropertiesLoose(source, excluded) {
if (source == null) return {};
var target = {};
for (var key in source) {
if ({}.hasOwnProperty.call(source, key)) {
if (excluded.indexOf(key) < 0) target[key] = source[key];
}
}
return target;
}
`; |
I have integrated one change from the snippet above, moving For more details on the |
Some corrections, I've inspected JSC's output further and I think I was wrong when I said it didn't do escape analysis. I've added colors to the disassembly and I think it is able to optimize the I also previously wrote that chrome's market share is 65%, but including edge in the mix means V8 has a 70% usage currently, so I'd still recommend merging this one. You have all the information, but if you don't feel like merging the PR feel free to close it. |
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.
Thank you for your detailed explanation!
In fact, we currently have terser
enabled for most complex helpers, which compresses Object.prototype.hasOwnProperty
into {}.hasOwnProperty
. I'm surprised this affects performance.
https://jsbench.me/8jluk7693l/1
I did a simple test and it seems it doesn't affect performance in v8.
Yeah it's possible that V8 is able to perform ellision on that value through escape analysis. It's also possible that it's not, and the |
I don't like market-share-based arguments (if we optimize for V8, then it gives even more advantage to V8 leading to it having a higher market share). However, given that the performance improvement here is higher than the perf regression in SM/JSC, I'm fine with this PR. |
objectWithoutPropertiesLoose
on V8
I don't love having the ecosystem that much in the hands of google either, but JSC is already vastly more performant than V8 anyway, and is still more performant than V8 for this particular case even with the change. |
I've been doing some benchmarks and I've seen
_objectWithoutPropertiesLoose
pop up a few times. I looked at the implementation and noticed it could be improved a bit, allocatingObject.keys()
to iterate through it is a bit inefficient. I have changed the implementation to usefor-in
and.hasOwnProperty
. Benchmarks: https://jsben.ch/lSR7EI'm not familiar with your codebase, I have tentatively used
"core-js-pure/features/object/has-own.js"
but I'm not sure if that corresponds toObject.prototype.hasOwnProperty
.