-
Notifications
You must be signed in to change notification settings - Fork 26
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
Object creation benchmark #8
Comments
To be clear, the fact that the object creation benchmark doesn't test object creation was actually discovered by Jakob Kummerow of the V8 project in a comment to the blog post, and it was I who wrote the fix and nodejs/CTC#155 (comment) you linked above. I'm not sure if he is on GitHub though. Speaking of that, it seems like there was some merge conflicts: https://github.com/davidmarkclements/v8-perf/blob/master/bench/object-creation.js now again contains a |
It seems |
+1 to @TimothyGu 's changes. The new results look plausible.
Crankshaft could do that too, under the right circumstances ;-) |
the either we have to set this with define properties config object or by adding a prop |
hey @jakobkummerow - which circumstances can trigger this object inlining behaviour in Crankshaft? |
Probably mostly the circumstances in the Octane benchmark ;-) |
Fixed in #17. |
@hashseed thanks so much for in-depth review and fixes:
in nodejs/CTC#155 (comment) you wrote:
Node.js v8.2.1 with V8 5.8:
Node.js v9.0.0-v8-canary20170724c045f1f8dc with V8 6.2.4:
When is this optimization happening? I've tried https://gist.github.com/mcollina/b903a1ef99f872ceb8e51dc491be34b8 (accessing a property) and https://gist.github.com/mcollina/40e91ad55ae9c3b0131c2a8b79387dfe (passing an object to a function), and it leads to similar previous results compared to the previous benchmark. However https://gist.github.com/mcollina/0bbcca999f7c190d97009165b7913824 reduces the gap.
It seems that if the object has hardcoded properties, then Turbofan can optimize the object creation away. I think this is a change worth including in our report, as passing objects with hardcoded properties as an "option object" is a widely used pattern.
I would like to keep including the previous benchmark (or one of above) and rebrand it as "option objects", to document this new behavior which in my opinion it is a fantastic result.
Which benchmark do you think fit better the case?
cc @bmeurer @TimothyGu
The text was updated successfully, but these errors were encountered: