Faster isEqual

[jgonggrijp]

With #3015, I removed the unlimited recursion that could cause a crash in _.flatten and _.isEqual if 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 _.isEqual to 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 _.isEqual in 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 _.isEqual for 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, Map is the obvious way to speed up these lookups. Unfortunately, Underscore also supports systems without Map.

Initially, I used a "dirty" trick that offers the same performance as Map without actually relying on Map. 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 polyfill WeakMap. 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 with Object.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, isEqual would already crash before 4500 levels of nesting depth.

In order to be able to choose an approach based on whether Map is available or not, I abstracted out the cycle-tracking logic to an object that is created in the new cycleTracker helper function. This is a rather large chunk of new code. Thanks to the abstraction, however, the changes to isEqual itself 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.js and test/objects.js require review. The remaining files are automatically generated from modules/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.

[jgonggrijp]

Bump! I realize these are complex changes, but they really need to be reviewed. Any review is better than no review at all. If you're reading along and you're curious to try GitHub's review features, even if you only recently learned to program and you don't know Underscore very well, please put on your bold socks and give it a go! Also, having multiple reviews is good, so don't hold back if you see that someone else did a review.

(Note: genAI reviews cannot replace human reviews, as far as I'm concerned. You can do a genAI review if you want but I will still want a human review after that.)

[colingm]

Is there a reason to loop pop that is going to add additional unnecessary computation vs just clearing both arrays by setting length to 0?

[jgonggrijp]

I guess not! Thanks.

[colingm]

okay awesome, just wasn't sure if there was something I was missing, thank you!