Skip to content
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.

Sign up for GitHub

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

Jump to bottom

util: use faster -0 check #15726

Merged
merged 1 commit into from
Oct 4, 2017
Merged

util: use faster -0 check #15726

merged 1 commit into from
Oct 4, 2017

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Oct 2, 2017 •
edited
Loading

Benchmark results:

                                                       improvement confidence      p.value
 util/inspect.js option="" method="Number" n=90000000     38.95 %        *** 5.305807e-54

CI: https://ci.nodejs.org/job/node-test-pull-request/10378/

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)
  • util

@mscdex mscdex added performance Issues and PRs related to the performance of Node.js. util Issues and PRs related to the built-in util module. labels Oct 2, 2017
ljharb
ljharb reviewed Oct 2, 2017
View reviewed changes
lib/util.js Outdated
// Format -0 as '-0'. Strict equality won't distinguish 0 from -0.
if (Object.is(value, -0))
// Format -0 as '-0'. A `value === -0` check won't distinguish 0 from -0.
if (1 / value === -Infinity)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth benchmarking 1 / value < 0 also.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

@mscdex mscdex Oct 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually 1 / -Infinity is -0, so a < 0 check would evaluate to false for value = -Infinity. I think having the stricter check is better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, good call. Don't mind me :-)

@BridgeAR
Copy link
Member

BridgeAR commented Oct 2, 2017

@bmeurer is Object.is already properly optimized in Turbofan?

@bmeurer
Copy link
Member

bmeurer commented Oct 2, 2017 •
edited
Loading

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. :-)

@bmeurer
Copy link
Member

bmeurer commented Oct 2, 2017 •
edited
Loading

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?

@mscdex mscdex force-pushed the util-format-faster-negative-zero-check branch from 0f6cf3a to 4540b21 Compare October 2, 2017 21:33
@mscdex
Copy link
Contributor Author

mscdex commented Oct 2, 2017

Code comment added.

@mscdex mscdex force-pushed the util-format-faster-negative-zero-check branch from 4540b21 to 8e1d76d Compare October 2, 2017 21:34
@bmeurer
Copy link
Member

bmeurer commented Oct 3, 2017

@mscdex Thanks!

@bmeurer
Copy link
Member

bmeurer commented Oct 4, 2017

FYI: Object.is optimized just landed, on track for 6.3.

kisg pushed a commit to paul99/v8mips that referenced this pull request Oct 4, 2017
@bmeurer
[es2015] Optimize Object.is baseline and interesting cases.
d4da17c 
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 <[email protected]>
Commit-Queue: Benedikt Meurer <[email protected]>
Cr-Commit-Position: refs/heads/master@{#48275}
@mscdex
util: use faster -0 check
d545c94 
PR-URL: nodejs#15726
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
@mscdex mscdex force-pushed the util-format-faster-negative-zero-check branch from 8e1d76d to d545c94 Compare October 4, 2017 07:54
@mscdex mscdex merged commit d545c94 into nodejs:master Oct 4, 2017
@mscdex mscdex deleted the util-format-faster-negative-zero-check branch October 4, 2017 07:55
addaleax pushed a commit to addaleax/ayo that referenced this pull request Oct 4, 2017
@mscdex @addaleax
util: use faster -0 check
44f845e 
PR-URL: nodejs/node#15726
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 7, 2017
@mscdex @MylesBorins
util: use faster -0 check
5268414 
PR-URL: #15726
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 7, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Oct 11, 2017
@mscdex @MylesBorins
util: use faster -0 check
58c68c2 
PR-URL: #15726
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Benedikt Meurer <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
@MylesBorins MylesBorins added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels Oct 17, 2017
@MylesBorins
Copy link
Contributor

should this land in LTS? If so it will need to bake a bit longer.

Please change labels as appropriate

@MylesBorins MylesBorins removed baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb left review comments

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@bmeurer bmeurer bmeurer approved these changes

@Fishrock123 Fishrock123 Fishrock123 approved these changes

@cjihrig cjihrig cjihrig approved these changes

@tniessen tniessen tniessen approved these changes

@hiroppy hiroppy hiroppy approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees
No one assigned
Labels
performance Issues and PRs related to the performance of Node.js. util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.