-
Notifications
You must be signed in to change notification settings - Fork 1k
fix: excessive cloning in runSubscribersWithMutation
#899
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
base: main
Are you sure you want to change the base?
fix: excessive cloning in runSubscribersWithMutation
#899
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses a critical performance bottleneck in runSubscribersWithMutation that was causing execution times of ~810ms with potential to hang the browser. The optimization achieves a 15x performance improvement by eliminating unnecessary cloning and replacing expensive JSON.stringify comparisons with reference equality checks.
Key changes:
- Moved cloning from input parameters (per-subscriber) to output mutations (only when mutations occur)
- Replaced
JSON.stringifyequality checks with reference equality checks for change detection - Maintained functional correctness while dramatically reducing clone operations in the hot path
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| structuredClone_(messages), | ||
| structuredClone_(state), | ||
| ); | ||
| const mutation = await executor(subscriber, messages, state); |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This optimization removes the defensive cloning that protects against subscribers mutating the input parameters directly. While subscribers should only mutate via return values, passing the same reference to multiple subscribers means that if a subscriber mutates the messages or state parameters in-place (even accidentally), those mutations will leak to subsequent subscribers.
The old approach cloned the inputs for each subscriber, providing isolation. Consider whether this risk is acceptable, or if there should be documentation/runtime checks to enforce that subscribers don't mutate inputs directly.
| const mutation = await executor(subscriber, messages, state); | |
| const messagesForSubscriber = structuredClone_(messages); | |
| const stateForSubscriber = structuredClone_(state); | |
| const mutation = await executor( | |
| subscriber, | |
| messagesForSubscriber, | |
| stateForSubscriber, | |
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intentional.
|
The failing job is unrelated to the change in this PR, reported separately here #897 |
429b64b to
c381b62
Compare
This PR fixes #883
The
runSubscribersWithMutationfunction has a critical performance bottleneck causing long execution time with the potential to hang the browser completely.Impact
Root Cause
This is in the hot path, called 2× per streaming event (once for
onEvent, once for specific event handler), but it:JSON.stringify()The Fix
Expected Results