| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
Might be worth benchmarking 1 / value < 0 also.
Sorry, something went wrong.
There was a problem hiding this comment.
@ljharb doesn't that have a different meaning? That condition is true if value is -Infinity but we want to know if value is -0.
Sorry, something went wrong.
There was a problem hiding this comment.
Actually 1 / -Infinity is -0, so a < 0 check would evaluate to false for value = -Infinity. I think having the stricter check is better.
Sorry, something went wrong.
There was a problem hiding this comment.
Ah yes, good call. Don't mind me :-)
Sorry, something went wrong.
|
@bmeurer is Object.is already properly optimized in Turbofan? |
Sorry, something went wrong.
|
Object.is is currently implemented as C++ runtime function call, so it's not really optimized. Looking at the original code, we could however optimize this easily, especially for the case where one side is -0. But I'd probably have TurboFan turn it into 1 / value === -Infinity as well. :-) |
Sorry, something went wrong.
|
I have a proof-of-concept for Object.is optimization in V8 here, which needs some polishing, but is generally doing the job. Improves the interesting Object.is(o, -0)case by like 11x on a micro-benchmark. I guess this PR still makes sense independent of the V8 fix, since that will only land in Node 9. But maybe you wanna leave a comment that this can be changed back to Object.is once V8 6.3 landed? |
Sorry, something went wrong.
|
Code comment added. |
Sorry, something went wrong.
|
FYI: Object.is optimized just landed, on track for 6.3. |
Sorry, something went wrong.
The Object.is builtin provides an entry point to the abstract operation SameValue, which properly distinguishes -0 and 0, and also identifies NaNs. Most of the time you don't need these, but rather just regular strict equality, but when you do, Object.is(o, -0) is the most readable way to check for minus zero. This is for example used in Node.js by formatNumber to properly print -0 for negative zero. However since the builtin thus far implemented as C++ builtin and TurboFan didn't know anything about it, Node.js considering to go with a more performant, less readable version (which also makes assumptions about the input value) in nodejs/node#15726 until the performance of Object.is will be on par (so hopefully we can go back to Object.is in Node 9). This CL ports the baseline implementation of Object.is to CSA, which is pretty straight-forward since SameValue is already available in CodeStubAssembler, and inlines a few interesting cases into TurboFan, i.e. comparing same SSA node, and checking for -0 and NaN explicitly. On the micro-benchmarks we go from testNumberIsMinusZero: 1000 ms. testObjectIsMinusZero: 929 ms. testObjectIsNaN: 954 ms. testObjectIsSame: 793 ms. testStrictEqualSame: 104 ms. to testNumberIsMinusZero: 89 ms. testObjectIsMinusZero: 88 ms. testObjectIsNaN: 88 ms. testObjectIsSame: 86 ms. testStrictEqualSame: 105 ms. which is a nice 10x to 11x improvement and brings Object.is on par with strict equality for most cases. Drive-by-fix: Also refactor and optimize the SameValue check in the CodeStubAssembler to avoid code bloat (by not inlining StrictEqual into every user of SameValue, and also avoiding useless checks). Bug: v8:6882 Change-Id: Ibffd8c36511f219fcce0d89ed4e1073f5d6c6344 Reviewed-on: https://chromium-review.googlesource.com/700254 Reviewed-by: Jaroslav Sevcik <jarin@chromium.org> Commit-Queue: Benedikt Meurer <bmeurer@chromium.org> Cr-Commit-Position: refs/heads/master@{#48275}
PR-URL: nodejs#15726 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
PR-URL: nodejs/node#15726 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
PR-URL: #15726 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
PR-URL: #15726 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
|
should this land in LTS? If so it will need to bake a bit longer. Please change labels as appropriate |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Benchmark results:
CI: https://ci.nodejs.org/job/node-test-pull-request/10378/
Checklist
Affected core subsystem(s)