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

benchmark: add assert.deep[Strict]Equal benchmarks #11092

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

  • Move numbers into configuration
  • Add buffer comparison benchmark
  • Add assert.deepStrictEqual benchmarks

Refs: #10282 (review)

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)

benchmark, assert

@nodejs-github-bot nodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label Jan 31, 2017
@joyeecheung joyeecheung added the assert Issues and PRs related to the assert subsystem. label Jan 31, 2017
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Almost there

data.copy(actual);
data.copy(expected);

if (conf.strict === 'strict') {
Copy link
Member

Choose a reason for hiding this comment

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

In other benchmarks we've used the pattern:

switch (conf.strict) {
  case 'strict':
    /** do stuff **/
  case 'nonstrict':
    /** do stuff **/
  default:
    throw new Error('Unsupported method');
}

This makes it easier to plug in new choices later and makes a code a bit easier to follow

Copy link
Member

Choose a reason for hiding this comment

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

It's also possible to simplify this further by doing something like:

var method;
switch (conf.strict) {
  case 'strict':
    method = assert.deepStrictEqual;
    break;
  case 'nonstrict':
    method = assert.deepEqual;
    break;
  default:
    throw new Error('...');
}
bench.start();
for (i = 0; i < n; ++i)
  method(actual, expected);
bench.end(n);

Or even:

const methods = {
  'strict': assert.deepStrictEqual,
  'nonstrict': assert.deepEqual
};
const method = methods[conf.strict];
if (!method) throw new Error('...');
bench.start();
for (i = 0; i < n; ++i)
  method(actual, expected);
bench.end(n);

* Move numbers into configuration
* Add buffer comparison benchmark
* Add assert.deepStrictEqual benchmarks
@joyeecheung
Copy link
Member Author

@jasnell Thanks for the review, updated, PTAL.

@joyeecheung
Copy link
Member Author

Aside: one of the try runs of Float32Array benchmarks raises the assertion, could be a V8 bug but I am not able to reproduce it.

@jasnell
Copy link
Member

jasnell commented Feb 2, 2017

hmm... I'm not sure I'm seeing that. Worth pinging @nodejs/v8 tho just in case.

@joyeecheung
Copy link
Member Author

joyeecheung commented Feb 2, 2017

FYI: I was playing with different n and len in deepequal-typedarrays.js when the assertion was raised. Can't remember what they were but definitely not 1 and 1e6 as they are now. At least n is not 1. And I don't even know if it's the old deepEqual (or deepStrictEqual) or if it's the new one in #10282 that raised this. Sorry I don't have more clues :/ I closed the session too soon, so probably not really worth digging into.

@joyeecheung
Copy link
Member Author

@joyeecheung
Copy link
Member Author

Going to land this in 24 hours

@joyeecheung
Copy link
Member Author

Landed in 5e4545e

@joyeecheung joyeecheung closed this Feb 6, 2017
joyeecheung added a commit that referenced this pull request Feb 6, 2017
* Move numbers into configuration
* Add buffer comparison benchmark
* Add assert.deepStrictEqual benchmarks

PR-URL: #11092
Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit that referenced this pull request Feb 6, 2017
* Move numbers into configuration
* Add buffer comparison benchmark
* Add assert.deepStrictEqual benchmarks

PR-URL: #11092
Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 14, 2017
* Move numbers into configuration
* Add buffer comparison benchmark
* Add assert.deepStrictEqual benchmarks

PR-URL: nodejs#11092
Reviewed-By: James M Snell <jasnell@gmail.com>
@joyeecheung joyeecheung deleted the assert-benchmark branch February 19, 2017 17:42
krydos pushed a commit to krydos/node that referenced this pull request Feb 25, 2017
* Move numbers into configuration
* Add buffer comparison benchmark
* Add assert.deepStrictEqual benchmarks

PR-URL: nodejs#11092
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Mar 7, 2017

would need a backport PR to land on v4

jasnell pushed a commit that referenced this pull request Mar 7, 2017
* Move numbers into configuration
* Add buffer comparison benchmark
* Add assert.deepStrictEqual benchmarks

PR-URL: #11092
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
* Move numbers into configuration
* Add buffer comparison benchmark
* Add assert.deepStrictEqual benchmarks

PR-URL: #11092
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
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. benchmark Issues and PRs related to the benchmark subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants