Skip to content

assert: improve performance #13973

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

Closed
BridgeAR wants to merge 3 commits into from
Closed

assert: improve performance #13973

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7727eeb
benchmark: fix and extend assert benchmarks
BridgeAR Jun 28, 2017
a3e6194
assert: refactor to reduce unecessary code paths
BridgeAR Jun 28, 2017
6e456f1
test: remove obsolete argument
BridgeAR Jun 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
benchmark: fix and extend assert benchmarks 
The benchmarks had the strict and non strict labels switched.
This is fixed and the benchmarks were extended to check more
possible input types and function calls.
  • Loading branch information
@BridgeAR
BridgeAR committed Jul 8, 2017
commit 7727eeb54d1fb52d06c367865cb61f04eb29f04d
35 changes: 29 additions & 6 deletions benchmark/assert/deepequal-buffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,39 +1,62 @@
'use strict';
const common = require('../common.js');
const assert = require('assert');

const bench = common.createBenchmark(main, {
n: [1e3],
len: [1e2],
method: ['strict', 'nonstrict']
n: [1e5],
Copy link
Copy Markdown
Member

@Trott Trott Jun 30, 2017

Choose a reason for hiding this comment

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

@mscdex @nodejs/benchmarking Is this sort of benchmark change OK? Do we usually do changes like this, or do we more typically add additional cases (so n: [1e3, 1e5])?

Copy link
Copy Markdown
Contributor

@refack refack Jun 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Also IMHO even 1e5 is too low for a CPU bound op.
(Some of the p values are too high)

Copy link
Copy Markdown
Member

@gareth-ellis gareth-ellis Jun 30, 2017

Choose a reason for hiding this comment

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

I'd be tempted to suggest adding as an extra - unless something has been found to be not valid with it being 1e3.
@joyeecheung implemented this benchmark in the first place, was there any particular reason for choosing 1e3 and not a larger number?
The other thing to consider is the length of time to run the benchmark, we should be careful about adding more variants which would add to longer run times for the benchmark suite

Copy link
Copy Markdown
Member Author

@BridgeAR BridgeAR Jun 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Using different n is not really useful as far as I can tell. And as @refack pointed out using 1e5 is still a low value. Otherwise it's difficult to get a higher significance if I'm correct.

len: [1e2, 1e4],
method: [
'deepEqual',
'deepStrictEqual',
'notDeepEqual',
'notDeepStrictEqual'
]
});

function main(conf) {
const n = +conf.n;
const len = +conf.len;
var i;

const data = Buffer.allocUnsafe(len);
const data = Buffer.allocUnsafe(len + 1);
const actual = Buffer.alloc(len);
const expected = Buffer.alloc(len);
const expectedWrong = Buffer.alloc(len + 1);
data.copy(actual);
data.copy(expected);
data.copy(expectedWrong);

switch (conf.method) {
case 'strict':
case 'deepEqual':
bench.start();
for (i = 0; i < n; ++i) {
// eslint-disable-next-line no-restricted-properties
assert.deepEqual(actual, expected);
}
bench.end(n);
break;
case 'nonstrict':
case 'deepStrictEqual':
bench.start();
for (i = 0; i < n; ++i) {
assert.deepStrictEqual(actual, expected);
}
bench.end(n);
break;
case 'notDeepEqual':
bench.start();
for (i = 0; i < n; ++i) {
// eslint-disable-next-line no-restricted-properties
assert.notDeepEqual(actual, expectedWrong);
}
bench.end(n);
break;
case 'notDeepStrictEqual':
bench.start();
for (i = 0; i < n; ++i) {
assert.notDeepStrictEqual(actual, expectedWrong);
}
bench.end(n);
break;
default:
throw new Error('Unsupported method');
}
Expand Down
73 changes: 73 additions & 0 deletions benchmark/assert/deepequal-object.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
'use strict';

const common = require('../common.js');
const assert = require('assert');

const bench = common.createBenchmark(main, {
n: [1e6],
size: [1e2, 1e3, 1e4],
method: [
'deepEqual',
'deepStrictEqual',
'notDeepEqual',
'notDeepStrictEqual'
]
});

function createObj(source, add = '') {
return source.map((n) => ({
foo: 'yarp',
nope: {
bar: `123${add}`,
a: [1, 2, 3],
baz: n
}
}));
}

function main(conf) {
const size = +conf.size;
// TODO: Fix this "hack"
const n = (+conf.n) / size;
var i;

const source = Array.apply(null, Array(size));
const actual = createObj(source);
const expected = createObj(source);
const expectedWrong = createObj(source, '4');

switch (conf.method) {
case 'deepEqual':
bench.start();
for (i = 0; i < n; ++i) {
// eslint-disable-next-line no-restricted-properties
assert.deepEqual(actual, expected);
}
bench.end(n);
break;
case 'deepStrictEqual':
bench.start();
for (i = 0; i < n; ++i) {
assert.deepStrictEqual(actual, expected);
}
bench.end(n);
break;
case 'notDeepEqual':
bench.start();
for (i = 0; i < n; ++i) {
// eslint-disable-next-line no-restricted-properties
assert.notDeepEqual(actual, expectedWrong);
}
bench.end(n);
break;
case 'notDeepStrictEqual':
bench.start();
for (i = 0; i < n; ++i) {
assert.notDeepStrictEqual(actual, expectedWrong);
}
bench.end(n);
break;
default:
throw new Error('Unsupported method');
}
}
119 changes: 119 additions & 0 deletions benchmark/assert/deepequal-prims-and-objs-big-array-set.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
'use strict';

const common = require('../common.js');
const assert = require('assert');

const primValues = {
'null': null,
'undefined': undefined,
'string': 'a',
'number': 1,
'boolean': true,
'object': { 0: 'a' },
'array': [1, 2, 3],
'new-array': new Array([1, 2, 3])
};

const bench = common.createBenchmark(main, {
prim: Object.keys(primValues),
n: [25],
len: [1e5],
method: [
'deepEqual_Array',
'deepStrictEqual_Array',
'notDeepEqual_Array',
'notDeepStrictEqual_Array',
'deepEqual_Set',
'deepStrictEqual_Set',
'notDeepEqual_Set',
'notDeepStrictEqual_Set'
]
});

function main(conf) {
const prim = primValues[conf.prim];
const n = +conf.n;
const len = +conf.len;
const actual = [];
const expected = [];
const expectedWrong = [];
var i;

for (var x = 0; x < len; x++) {
actual.push(prim);
expected.push(prim);
expectedWrong.push(prim);
}
expectedWrong.pop();
expectedWrong.push('b');

// Note: primitives are only added once to a set
const actualSet = new Set(actual);
const expectedSet = new Set(expected);
const expectedWrongSet = new Set(expectedWrong);

switch (conf.method) {
case 'deepEqual_Array':
bench.start();
for (i = 0; i < n; ++i) {
// eslint-disable-next-line no-restricted-properties
assert.deepEqual(actual, expected);
}
bench.end(n);
break;
case 'deepStrictEqual_Array':
bench.start();
for (i = 0; i < n; ++i) {
assert.deepStrictEqual(actual, expected);
}
bench.end(n);
break;
case 'notDeepEqual_Array':
bench.start();
for (i = 0; i < n; ++i) {
// eslint-disable-next-line no-restricted-properties
assert.notDeepEqual(actual, expectedWrong);
}
bench.end(n);
break;
case 'notDeepStrictEqual_Array':
bench.start();
for (i = 0; i < n; ++i) {
assert.notDeepStrictEqual(actual, expectedWrong);
}
bench.end(n);
break;
case 'deepEqual_Set':
bench.start();
for (i = 0; i < n; ++i) {
// eslint-disable-next-line no-restricted-properties
assert.deepEqual(actualSet, expectedSet);
}
bench.end(n);
break;
case 'deepStrictEqual_Set':
bench.start();
for (i = 0; i < n; ++i) {
assert.deepStrictEqual(actualSet, expectedSet);
}
bench.end(n);
break;
case 'notDeepEqual_Set':
bench.start();
for (i = 0; i < n; ++i) {
// eslint-disable-next-line no-restricted-properties
assert.notDeepEqual(actualSet, expectedWrongSet);
}
bench.end(n);
break;
case 'notDeepStrictEqual_Set':
bench.start();
for (i = 0; i < n; ++i) {
assert.notDeepStrictEqual(actualSet, expectedWrongSet);
}
bench.end(n);
break;
default:
throw new Error('Unsupported method');
}
}
55 changes: 0 additions & 55 deletions benchmark/assert/deepequal-prims-and-objs-big-array.js
View file
Open in desktop

This file was deleted.

27 changes: 24 additions & 3 deletions benchmark/assert/deepequal-prims-and-objs-big-loop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,33 +16,54 @@ const primValues = {
const bench = common.createBenchmark(main, {
prim: Object.keys(primValues),
n: [1e6],
method: ['strict', 'nonstrict']
method: [
'deepEqual',
'deepStrictEqual',
'notDeepEqual',
'notDeepStrictEqual'
]
});

function main(conf) {
const prim = primValues[conf.prim];
const n = +conf.n;
const actual = prim;
const expected = prim;
const expectedWrong = 'b';
var i;

// Creates new array to avoid loop invariant code motion
switch (conf.method) {
case 'strict':
case 'deepEqual':
bench.start();
for (i = 0; i < n; ++i) {
// eslint-disable-next-line no-restricted-properties
assert.deepEqual([actual], [expected]);
Copy link
Copy Markdown
Contributor

@refack refack Jun 28, 2017

Choose a reason for hiding this comment

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

Why not do

const tesee = assert[conf.method];

Instead of the whole switch?

Copy link
Copy Markdown
Member Author

@BridgeAR BridgeAR Jun 29, 2017

Choose a reason for hiding this comment

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

Well, the values do change at least partly so we can't just remove the switch. I can combine the ones that have the same arguments if you want me to.

Copy link
Copy Markdown
Contributor

@refack refack Jun 29, 2017

Choose a reason for hiding this comment

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

Im ±0, your call.

Copy link
Copy Markdown
Member Author

@BridgeAR BridgeAR Jun 29, 2017

Choose a reason for hiding this comment

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

I think I prefer to stick to the way it is as that way it's simpler to add more tests for the same function with different inputs.

}
bench.end(n);
break;
case 'nonstrict':
case 'deepStrictEqual':
bench.start();
for (i = 0; i < n; ++i) {
assert.deepStrictEqual([actual], [expected]);
}
bench.end(n);
break;
case 'notDeepEqual':
bench.start();
for (i = 0; i < n; ++i) {
// eslint-disable-next-line no-restricted-properties
assert.notDeepEqual([actual], [expectedWrong]);
}
bench.end(n);
break;
case 'notDeepStrictEqual':
bench.start();
for (i = 0; i < n; ++i) {
assert.notDeepStrictEqual([actual], [expectedWrong]);
}
bench.end(n);
break;
default:
throw new Error('Unsupported method');
}
Expand Down
Loading