-
Notifications
You must be signed in to change notification settings - Fork 30k
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 performance #13973
assert: improve performance #13973
Conversation
f72aa63
to
b8451b0
Compare
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]); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im ±0, your call.
There was a problem hiding this comment.
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.
@@ -423,8 +423,7 @@ assert.throws(makeBlock(thrower, TypeError)); | |||
assert.ok(e instanceof TypeError, 'type'); | |||
} | |||
assert.strictEqual(true, threw, | |||
'a.throws with an explicit error is eating extra errors', | |||
a.AssertionError); | |||
'a.throws with an explicit error is eating extra errors'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄
I'm gonna dig and find how did that happen. Probably an interesting story.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not much of a story, it has always been there, nobody notices for 8 years
4f679fd#diff-02063f30815b78f7986ee0c212140b8eR110
Quick sanity: https://ci.nodejs.org/job/node-test-commit-linuxone/6915/ |
benchmark/assert/deepequal-buffer.js
Outdated
@@ -1,39 +1,63 @@ | |||
'use strict'; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: unnecessary whitespace changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it preferred to not use a line break after use strict
in general or would you just want to keep that as a separate concern?
Not a requirement for landing but helpful if someone has the inclination to run |
@Trott I checked the coverage and I was able to remove even more code paths and move another check further up. Now the coverage is back to 100%. So PTAL. Those changes are reflected in these benchmarks:
[refack: folded benchmark results] |
benchmark/assert/deepequal-buffer.js
Outdated
const bench = common.createBenchmark(main, { | ||
n: [1e3], | ||
n: [1e5], |
There was a problem hiding this comment.
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]
)?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
I stumbled open a few more tiny improvements (tiny buffers and different sized buffers are checked faster now) and turbofan still can't handle try catch well. |
lib/assert.js
Outdated
} | ||
if (len < 300) { | ||
var offset = 0; | ||
while (offset < len) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not for
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shrug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
% /benchmark/
changes decision
lib/assert.js
Outdated
} | ||
return true; | ||
} | ||
} else if (isArguments(actualTag) || isArguments(expectedTag)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a special case for compatibility so the link to the original PR should be kept IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
lib/assert.js
Outdated
return false; | ||
} | ||
if (isObjectOrArrayTag(actualTag)) { | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a comment about returning undefined means we need to check further?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
benchmark/assert/throws.js
Outdated
method: [ | ||
'doesNotThrow', | ||
'throws', | ||
'throws TypeError', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe get rid of the space in the argument values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be a underscore be fine instead? The first part is the function name and that's why I'd like some kind of distinction.
Pre-land CI: https://ci.nodejs.org/job/node-test-commit/10890/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actual changes LGTM and make a nice improvement - since it changes a lot of code a CITGM run would also be appreciated.
@BridgeAR I tried to resolve the conflicts, but it's too delicate. So please rebase. |
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.
The lazy loading is not needed as the errors themself lazy load assert. Therefore the circle is working as intended even without this lazy loading. Improve Array, object, ArrayBuffer, Set and Map performance in all deepEqual checks by removing unecessary code paths and by moving expensive checks further back. Improve throws and doesNotThrow performance by removing dead code and simplifying the overall logic.
assert.strictEqual can either have two or three arguments, not four.
83a6d77
to
6e456f1
Compare
Rebased |
The lazy loading is not needed as the errors themself lazy load assert. Therefore the circle is working as intended even without this lazy loading. Improve Array, object, ArrayBuffer, Set and Map performance in all deepEqual checks by removing unecessary code paths and by moving expensive checks further back. Improve throws and doesNotThrow performance by removing dead code and simplifying the overall logic. PR-URL: nodejs#13973 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@refack Just for context, what’s the motivation for leaving out benchmarks? Anyway, if anything in here can be backported to v8.x, it would need to be backported manually (guide). We’re already in a situation where a lot of changes to |
They were done in this PR to prove it actually has a positive performance gain, but It would perturb https://benchmarking.nodejs.org/ so we spun off #14147 to be discussed just in the context of benchmarking changes. |
P.S. should probably land with #14258 |
@addaleax I am going to backport this later on. |
Lands cleanly now :) |
The lazy loading is not needed as the errors themself lazy load assert. Therefore the circle is working as intended even without this lazy loading. Improve Array, object, ArrayBuffer, Set and Map performance in all deepEqual checks by removing unecessary code paths and by moving expensive checks further back. Improve throws and doesNotThrow performance by removing dead code and simplifying the overall logic. PR-URL: #13973 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
While refactoring the assert in #13862 I stumbled open a couple of improvements. This change is only about performance changes and not about style.
I added a few more benchmarks and fixed the description of the old ones.
Benchmarks
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
assert