fix: handle duplicate signal dependencies gracefully #12261
Merged
Conversation
This file contains 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
🦋 Changeset detectedLatest commit: fa2bab3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
trueadm
changed the title
visitor another approach tune tune
trueadm
force-pushed
the
remove-duplicate-deps
branch
from
1610602 to
bb9c448
Compare
Rich-Harris
reviewed
Jul 2, 2024
|
I do think we need to get a test in here — I was looking at this code the other day thinking we could possibly make it a bit more obvious what's going on, but we'll be prone to regressions if we make any changes to it |
I'll try again. |
|
@Rich-Harris Was able to reproduce the same issue with a test! :) |
Rich-Harris
approved these changes
Jul 2, 2024
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.
Follow up to #12245. Fixes #12230. This also permits duplicate dependencies and instead we avoid processing duplicates in
remove_reactions. This means far less work is done on the fast normal path, and instead we just do a bit more work when removing signals.This PR faster in almost all the benchmarks than
mainfor me, and the ones it is not it's off by a small fraction which shows that the codepath likely isn't being hit.