Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.2k
assert: improve deepEqual Set and Map worst case#14258
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
BridgeAR commented Jul 15, 2017 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
BridgeAR commented Jul 18, 2017 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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. |
refack commented Jul 18, 2017
@BridgeAR things are hectic with trying to release 6.11.2 & 8.2.0, so it might take a few more days... |
b6ccc6f to fdbd79aComparefdbd79a to 37e0950CompareBridgeAR commented Jul 19, 2017
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. |
refack commented Jul 19, 2017
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 462b58eComparerefack commented Jul 19, 2017
PR-URL: #14258 Reviewed-By: Refael Ackermann <[email protected]>
PR-URL: #14258 Reviewed-By: Refael Ackermann <[email protected]>
addaleax commented Jul 27, 2017
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 commented Jul 27, 2017 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
addaleax commented Jul 27, 2017
@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. :) |
BridgeAR commented Jul 27, 2017
@addaleax yes, I was to quick with sending the message and I already removed that from the entry again^^. |
addaleax commented Jul 28, 2017
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]>
MylesBorins commented Aug 16, 2017
Should this be landed on v6.x? |
BridgeAR commented Aug 18, 2017
@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