Repository navigation
Fix quadratic duplicate checks when merging comments - #673
Merged
Merged
Conversation
`NodeWithComments#merge_with` appended the other node's comments one at a time and called `Array#include?` on the growing `comments` array for each of them. Merging a tree with N distinct comments therefore performed about N²/2 comment comparisons. This is reached for file header comments, which are merged into the `MergeTree` for every root tree, and for comments on definitions that exist in both trees. Index the existing comments by text and only compare a merged comment with the indexed comments sharing its text. The check still goes through `Comment#==`, so the comments that survive a merge and their order are unchanged. A differential fuzz of 50,000 random mixes of plain, RBS and blank comments produced identical results for the old and new code. Merging 20,000 distinct header comments goes from 10s to 5ms, and merging a class carrying 20,000 comments in both trees goes from 32s to 18ms. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
soutaro
approved these changes
Oct 19, 2026
KaanOzkan
approved these changes
Oct 19, 2026
| res = tree1.merge(tree2) | ||
|
|
||
| assert_equal(["New comment"], res.comments.map(&:text)) | ||
| end |
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
3 participants
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.
Problem
NodeWithComments#merge_withappends the other node's comments one at a time and callsArray#include?on the growingcommentsarray for each of them. Merging a tree with N distinct comments therefore performs about N²/2 comment comparisons. This is reached for file header comments, which are merged into theMergeTreefor every root tree, and for comments on definitions that exist in both trees.This was reported through an internal security review as a CPU-exhaustion vector for any consumer that merges untrusted RBI text. Reproduction with 20,000 distinct comments:
Before the fix, doubling the comment count roughly quadrupled merge time.
Fix
Index the existing comments by text and only compare a merged comment with the indexed comments sharing its text. The comparison still goes through
Comment#==, so the comments that survive a merge and their order are unchanged. In particular the existing asymmetry between plain and RBS comments is preserved as is.A differential fuzz of 50,000 random mixes of
Comment,RBSCommentandBlankLineobjects, comparing the oldinclude?loop with the new code, produced identical results in every case.Tests
test_merge_many_distinct_comments_does_not_compare_every_pairmerges a tree with 2,000 distinct header comments into one with the same comments plus one more, asserts the merged comments and their order, and counts==calls on the comment objects. It failed before the fix with 4,002,000 checks against a bound of 6,000.Not in this PR
The
AttrandMethodoverrides in the same file dedupe sigs with the sameinclude?pattern, so a definition with many distinct sigs on one side is also quadratic.Sighas no hash key consistent with its==, so that is a larger change and is left for a follow-up.🤖 Generated with Claude Code