-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
util,assert: improve comparison performance #22258
Conversation
@@ -5,30 +5,53 @@ const { isArrayBufferView } = require('internal/util/types'); | |||
const { internalBinding } = require('internal/bootstrap/loaders'); | |||
const { isDate, isMap, isRegExp, isSet } = internalBinding('types'); | |||
|
|||
function objectToString(o) { | |||
return Object.prototype.toString.call(o); | |||
const ReflectApply = Reflect.apply; |
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.
FWIW as a reviewer I'd still expect the "tamper safe"ing to be in a different PR. That said, changing it now here would be very frustrating to do likely so I'm fine with it here - just future personal preference.
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'll see if I move those out due to this becoming semver-major.
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 only changed the single part as it is indeed more work to pull these changes out.
lib/internal/util/comparisons.js
Outdated
// barrier including the Uint8Array operation takes the advantage of the faster | ||
// binary compare otherwise. The break even point was at about 300 characters. | ||
function areSimilarTypedArrays(a, b, max) { | ||
function areSimilarFloatArrays(a, b) { | ||
const len = a.byteLength; |
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 the caching of a.byteLength
still helpful/needed?
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.
No, but it should not hurt either?
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.
Then can you remove it :)?
// Skip testing the part below and continue with the keyCheck. | ||
return keyCheck(val1, val2, true, memos); | ||
} | ||
// Fast path for non sparse arrays (no key comparison for indices |
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.
How does this if
check assert this is a fast path and the array is not sparse?
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.
From what I can tell:
- this just checks that all the keys are indexed properties and that the last key is the last index
- it relies on the
Object.keys
key ordering in V8 - AFAIK there is no guarantee on keys but there is on the order of other stuff e.g. getOwnPropertyNames (I am fine with this and just tracking it in case it changes in V8). - It fails with setting negative indices, for example:
var o = {}; o[-1] = 3; o[1] = 5; o.length = 2
. We should add a test for this particular case. For what it's worth I'm fine with that case being slow since it's uncommon and sparse arrays are really confusing perf-wise anyway.
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.
Alternatively, we can see if we can check it directly from V8 - something like UseSparseVariant
- or use %HasComplexElements
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.
The keys are ordered by spec as follows:
- The keys that are integer indices, in ascending numeric order.
- All string keys, in the order in which they were added to the object.
- All symbol keys, in the order in which they were added to the object.
The order makes this possible: Object.keys(arr)[arr.length - 1] === '' + (arr.length - 1)
. By checking the key at arr.length - 1
it should be the same key as the key length. Otherwise the key would have a position lower than that. If Object.keys(arr).length === arr.length
on top of that, we also know that the array has no extra keys besides potentially symbols.
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.
@BridgeAR as I said above as far as I know OrdinaryOwnPropertyKeys
is used in .getOwnPropertyNames
but not in .keys
- that said I don't particularly mind it since I don't see a future in which V8 changes the iteration order of objects anyway since it's so heavily relied on.
I think negative indices are not a problem since they're not technically indices from the spec PoV and would be enumerated elsewhere.
Upon further inspection - it looks like this works fine for all the cases - care to add a comment?
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.
By spec Object.keys
calls EnumerableOwnPropertyNames
and that calls OwnPropertyKeys
and that calls OdinaryOwnPropertyKeys
:)
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.
Neat, I wonder when it changed - I remember this discussion when we did the io changes for ES2015 and it was guaranteed for one and not the other. Good to know it's a guarantee now - thanks.
lib/internal/util/comparisons.js
Outdated
} | ||
if (isDate(val1)) { | ||
if (val1.getTime() !== val2.getTime()) { | ||
if (getTime(val1) !== getTime(val2)) { |
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.
If someone subclasses Date
and they have an overridden getTime()
would this work?
If not, I am still +1 for it, but it would require this to be a very explicit semver-major.
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'll change it to a separate commit.
if (keys.length !== objectKeys(val2).length) { | ||
return false; | ||
} | ||
if (keys.length === val1.length) { |
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.
Nice changes in isArrayBufferView!
lib/internal/util/comparisons.js
Outdated
return areSimilarTypedArrays(val1, val2); | ||
} | ||
if (isDate(val1) && isDate(val2)) { | ||
return getTime(val1) === getTime(val2); |
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.
Ditto about getTime with a subclass (again +1 on the change, just making sure it's semver-major)
} | ||
|
||
// Cheap key test | ||
let i = 0; |
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.
Where is i
used outside of the loop here? It confused me since I expected it to be used at some capacity elsewhere. I don't think making i
local to the loop would hurt performance.
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.
It is used in two loops. I could define a second variable but this seemed nicer.
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 find it a bit more confusing than defining it in the loop for the reason above but don't feel strongly about it - so if you think it's nicer I concede.
@@ -451,10 +526,7 @@ function mapHasEqualEntry(set, map, key1, item1, strict, memo) { | |||
} | |||
|
|||
function mapEquiv(a, b, strict, memo) { | |||
if (a.size !== b.size) |
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 did you inline the size checks rather than have them in mapEquiv (and setEquiv)?
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 moved them to the beginning of the checks. Now, sets and maps will fail early in case the size does not match. Earlier they had significantly more work to reach this check.
lib/internal/util/comparisons.js
Outdated
@@ -489,35 +561,44 @@ function mapEquiv(a, b, strict, memo) { | |||
return true; | |||
} | |||
|
|||
function objEquiv(a, b, strict, keys, memos) { | |||
function objEquiv(a, b, strict, keys, memos, iterator) { |
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 sure about naming it iterator
but very much in favour of this change.
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 am open to suggestions.
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 not sure I like iteratorKind or iterationType are much better to be honest.
test/parallel/test-assert-deep.js
Outdated
@@ -890,3 +890,10 @@ assert.deepStrictEqual(obj1, obj2); | |||
); | |||
util.inspect.defaultOptions = tmp; | |||
} | |||
|
|||
// Basic array out of bounds check. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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 already covered.
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.
Left a bunch of nits I'd love to see resolved before landing but changes look good enough to not generate friction.
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.
Actually looking at my review again I want to make sure that sparse arrays with negative indices work. I just did a code search on GitHub and found some code using that pattern which made me nauseous but reconsider landing this without verification this case works.
The semver-major is also preemptive, I'm fine with landing as semver-patch or semver-minor with a citgm run and a TSC member sign off. |
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.
Ok, I checked it out and am convinced this works + @BridgeAR explained the part I wasn't certain about with negative array indices.
I'm fine with this landing as-is as semver-major or with the getDate
changes reverted as semver-patch.
(In case I haven't said it before - very nice changes!)
I just reverted the |
@benjamingr thanks a lot for the thorough review! |
This comment has been minimized.
This comment has been minimized.
This adds a smarter logic to compare object keys (including symbols) and it also skips the object key comparison for (typed) arrays, if possible. Besides that it adds a fast path for empty objects, arrays, sets and maps and fast paths for sets and maps with an unequal size. On top of that a few functions are now safer to call by using uncurryThis and by caching the actual function. Overall, this is a significant performance boost for comparisons.
c6f41bf
to
f86a91f
Compare
Rebased due to conflicts. CI https://ci.nodejs.org/job/node-test-pull-request/16427/ @nodejs/util @nodejs/testing PTAL |
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.
LGTM if CI is green
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.
Still LGTM as semver-patch with the getTime reverts.
Build is green with https://ci.nodejs.org/job/node-test-commit/20531/ and linux one rerun https://ci.nodejs.org/job/node-test-commit-linuxone/3974/ |
This adds a smarter logic to compare object keys (including symbols) and it also skips the object key comparison for (typed) arrays, if possible. Besides that it adds a fast path for empty objects, arrays, sets and maps and fast paths for sets and maps with an unequal size. On top of that a few functions are now safer to call by using uncurryThis and by caching the actual function. Overall, this is a significant performance boost for comparisons. PR-URL: nodejs#22258 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Landed in d164d9d 🎉 |
This adds a smarter logic to compare object keys (including symbols) and it also skips the object key comparison for (typed) arrays, if possible. Besides that it adds a fast path for empty objects, arrays, sets and maps and fast paths for sets and maps with an unequal size. On top of that a few functions are now safer to call by using uncurryThis and by caching the actual function. Overall, this is a significant performance boost for comparisons. PR-URL: #22258 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
This adds a smarter logic to compare object keys (including symbols) and it also skips the object key comparison for (typed) arrays, if possible. Besides that it adds a fast path for empty objects, arrays, sets and maps and fast paths for sets and maps with an unequal size. On top of that a few functions are now safer to call by using uncurryThis and by caching the actual function. Overall, this is a significant performance boost for comparisons. PR-URL: #22258 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
This improves the performance of
util.isDeepStrictEqual
andassert.deepStrictEqual
. It is mainly just code split from #22197 to reduce the overall change. This commit can also be backported. It should have similar results on older V8 versions.The complete key comparison got reworked and short cuts got in where possible.
I only benchmarked the strict mode as it should be the only important part. For the legacy mode the performance should stay similar to the one before (+ / - a few percent here and there). I used the benchmark from #22211.
Some of the functions used also got safer by instrumenting uncurryThis.
Benchmark
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes