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.

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

assert: improve assert()/assert.ok() performance #19292

Merged
merged 1 commit into from
Mar 15, 2018

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Mar 12, 2018

With included benchmark:

                            confidence improvement accuracy (*)    (**)   (***)
 assert/ok.js n=1000000000        ***    841.57 %       ±8.15% ±11.30% ±15.66%

Be aware that when doing many comparisions the risk of a false-positive
result increases. In this case there are 1 comparisions, you can thus
expect the following amount of false-positive results:
  0.05 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.01 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

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

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

@mscdex mscdex added assert Issues and PRs related to the assert subsystem. performance Issues and PRs related to the performance of Node.js. labels Mar 12, 2018
@nodejs-github-bot nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Mar 12, 2018
lib/assert.js Outdated
function innerOk(args, fn) {
var [value, message] = args;

function innerOk(fn, argc, value, message) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: argc is not really commonly used in JS and not everyone might know the terminology. So I suggest to switch to argLength.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated parameter name

@mscdex mscdex force-pushed the perf-assert-ok branch 2 times, most recently from 2ae9b1d to 66ce648 Compare March 13, 2018 18:51
PR-URL: nodejs#19292
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@mscdex mscdex merged commit 3c61b87 into nodejs:master Mar 15, 2018
@mscdex mscdex deleted the perf-assert-ok branch March 15, 2018 17:42
@MylesBorins
Copy link
Contributor

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

@BridgeAR
Copy link
Member

BridgeAR commented Oct 2, 2018

I added the label to prevent backporting this. It relies on a semver-major change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants