-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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 deepEqual Set and Map worst case #14258
Conversation
I added benchmarks and reworked the code again and fixed a regression for deepEqual with Map by doing so. deepEqual with Map got a bit ugly but I think there are not many other possibilities. I also simplified two more statements. I still have to check the coverage but PTAL already. Update: I moved the benchmarks to the top. |
@BridgeAR things are hectic with trying to release 6.11.2 & 8.2.0, so it might take a few more days... |
b6ccc6f
to
fdbd79a
Compare
fdbd79a
to
37e0950
Compare
I finished the code and the coverage is kept at 100% and I heavily tested weird edge cases. I decided not to add a memory set for loose equal keys even though as it would complicate things again and the numbers should be good enough even without that optimization. I am running the benchmarks for maps right now and I am adding them to the main entry as soon as they are done. The numbers for sets are already there. |
This change improves the algorithm for the worst case from O(n^2) to O(n log n) by using a lazily initiated set with object or not strict equal primitives keys. In addition a few comments got fixed and a statement simplified. PR-URL: nodejs#14258 Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: nodejs#14258 Reviewed-By: Refael Ackermann <[email protected]>
37e0950
to
462b58e
Compare
PR-URL: #14258 Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: #14258 Reviewed-By: Refael Ackermann <[email protected]>
The first commit (5203bb0) doesn’t land cleanly on 8.x; if you can, please follow the guide and raise a backport PR, if you don’t think it’s worth it let me know and we’ll add the |
@BridgeAR Correct me if I’m wrong, but the first commit seems to depend on a semver-major one, semantically, so it can’t be backported. If you can backport the second one and this one cherry-picks cleanly after that, great. :) |
@addaleax yes, I was to quick with sending the message and I already removed that from the entry again^^. |
Lands cleanly now :) |
This change improves the algorithm for the worst case from O(n^2) to O(n log n) by using a lazily initiated set with object or not strict equal primitives keys. In addition a few comments got fixed and a statement simplified. PR-URL: #14258 Reviewed-By: Refael Ackermann <[email protected]>
Should this be landed on v6.x? |
@MylesBorins Set and Map support was added in 8.0 as a semver-major. Due to that backporting is not possible. |
This change improves the algorithm for the worst case from O(n^2)
to O(n log n) by using a lazily initiated set for object keys and a special handling for loose equal primitives.
In addition a few comments got fixed and a few statements simplified.
There is still 100% coverage and I heavily tested weird cases.
Benchmarks:
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
assert