Faster isEqual#3022
Open
jgonggrijp wants to merge 7 commits into
Open
Conversation
Together with the next commit, this approach was suggested by @jashkenas in private communication.
Together with the previous commit, this approach was suggested by @jashkenas in private communication. lower the limit
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
With #3015, I removed the unlimited recursion that could cause a crash in
_.flattenand_.isEqualif deeply nested objects were passed. As of version 1.13.8, objects of arbitrary depth are supported.This uncovered a performance issue in
_.isEqual: the cycle detection algorithm had quadratic runtime complexity in the depth of the compared objects. This complexity had always been known, but the impact was never noticable because the depth of recursion was limited by the depth of the stack. With the latter limit removed, it was suddenly possible to create very noticable delays by feeding extremely deep objects to_.isEqual. This is why I set the regression testing depth limit for_.isEqualto 30k levels, rather than 100k levels as I had done for_.flatten. Saves a factor of 10 in wasted time and carbon emissions.The security impact is limited, because you need to feed absurdly large objects to
_.isEqualin order to cause noticable delays and even then, you cannot force a complete denial of service. Still, I felt there was room for improvement, if only because it would be nice to be able to use_.isEqualfor arbitrarily deep comparisons.The cycle detection is based on looking up whether an object on one side of the comparison has been seen before and if so, whether this occurs in the same way on the other side of the comparison. The pre-existing, poorly performing way to do this was by making a linear search through all parent objects. For systems that support it,
Mapis the obvious way to speed up these lookups. Unfortunately, Underscore also supports systems withoutMap.Initially, I used a "dirty" trick that offers the same performance as
Mapwithout actually relying onMap. It was based on temporarily modifying the visited objects, so it was possible to recognize objects that I had seen before and make associations between objects. This is similar to how you might polyfillWeakMap. I would undo the modifications before returning and the trick worked, but there were too many potential problems with objects that had been write-protected withObject.seal(), integrity-checking accessors, and so forth. This trick is no longer part of the diff, but those who are curious can view it in 9326ceb.I discussed this with @jashkenas. He suggested that for
Map-less systems, we could simply stick to the linear search and throw an error when the cycle detection was about to become overly expensive. The limit should be generous enough that everything that was possible until version 1.13.7 was still possible. I agreed with this approach and the final diff in this pull request is based on it. I somewhat arbitrarily chose a limit of 50M cycle detection comparisons, which corresponds to at least 7000 levels of nesting depth. Based on limited testing, 50M cycle detection comparisons could produce delays in the order of tens to hundreds of milliseconds. Before the recursion fix,isEqualwould already crash before 4500 levels of nesting depth.In order to be able to choose an approach based on whether
Mapis available or not, I abstracted out the cycle-tracking logic to an object that is created in the newcycleTrackerhelper function. This is a rather large chunk of new code. Thanks to the abstraction, however, the changes toisEqualitself are quite digestible.I added a note about the error-throwing behavior in legacy systems to the documentation. The note is quite large and dense, so I'm a bit concerned that users might find it overwhelming and miss the point as a result. I welcome suggestions on how that text can be made lighter.
There are 9 files in the diff, but only the
index.html,modules/isEqual.jsandtest/objects.jsrequire review. The remaining files are automatically generated frommodules/isEqual.js(and all the other modules).FYI @ByamB4 @GammaGames @buildwithricky
PS: From the commit dates, it might seem as if I did all the work in the past few hours. This is not the case: I started the branch months ago. I rebased the branch shortly before creating this pull request, which bumped the commit dates.