-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat: add content debouncing to ReasoningBlock for improved streaming performance #8176
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
Conversation
- Implement debounced content updates during streaming (100ms debounce) - Combine with existing memoized ElapsedTime component for timer isolation - Prevents excessive re-renders during rapid content streaming - Immediate updates when streaming completes for final content - Addresses performance concerns raised in issue #7999
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.
Reviewing my own code is like debugging in a mirror - everything looks backwards but the bugs are still mine.
| * - Timer is isolated in a memoized component to prevent full re-renders | ||
| * - Content updates are debounced to prevent excessive re-renders during streaming | ||
| */ | ||
| export const ReasoningBlock = ({ content, isStreaming, isLast }: ReasoningBlockProps) => { |
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.
Since this is a performance-critical component with complex debouncing logic, it would benefit from test coverage. Consider adding tests to verify:
- The 100ms debouncing behavior works correctly
- Content updates immediately when streaming stops
- The ElapsedTime component properly isolates re-renders
- Cleanup happens on unmount
| const [debouncedContent, setDebouncedContent] = useState<string>(content || "") | ||
|
|
||
| // Create a debounced function to update content | ||
| // This limits content updates to a maximum of ~10 per second (100ms debounce) |
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.
The comment mentions "~10 per second" but with a 100ms debounce, it's exactly 10 per second maximum. Consider updating for precision:
| () => | ||
| debounce((newContent: string) => { | ||
| setDebouncedContent(newContent) | ||
| }, 100), |
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.
Is it intentional that the debounce delay is hardcoded to 100ms? Consider making this configurable via props for different use cases, or at least extracting it as a named constant for better maintainability.
| updateDebouncedContent(content || "") | ||
| } else { | ||
| // When not streaming, update immediately for final content | ||
| setDebouncedContent(content || "") |
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.
Good edge case handling here - immediately updating content and clearing pending debounces when streaming stops. This ensures the final content is shown without delay. Though if streaming rapidly toggles between true/false, could there be any race conditions? The implementation looks correct, but might be worth documenting this behavior.
|
Closing this PR as requested. The debouncing changes will be applied to PR #8000 instead. |
Summary
This PR builds upon PR #8000 by adding content debouncing to the ReasoningBlock component, addressing the suggestion from @hannesrudolph about capping the maximum number of renders per second during content streaming.
Problem
While PR #8000 fixed the timer-related performance regression, rapid content streaming (especially with fast models like Claude) could still trigger many re-renders as content updates arrive.
Solution
This implementation adds a debouncing mechanism for content updates during streaming:
Content Debouncing:
Combined with Timer Isolation (from PR fix: optimize ReasoningBlock performance by isolating timer re-renders #8000):
ElapsedTimecomponent prevents timer updates from triggering parent re-rendersChanges
debouncedContentstate to track debounced content separatelyupdateDebouncedContentusing the existingdebouncepackage (100ms delay)Testing
Performance Impact
This addresses the performance concerns raised in #7999 and implements the suggestion from the issue discussion.
cc @hannesrudolph @nabilfreeman
Important
Adds content debouncing to
ReasoningBlockto limit re-renders during streaming, improving performance.ReasoningBlockcontent updates during streaming, capping renders at ~10 per second.ElapsedTimeto prevent fullReasoningBlockre-renders every second.debouncedContentstate inReasoningBlock.debouncepackage for content update debouncing.This description was created by
for 16bc514. You can customize this summary. It will automatically update as commits are pushed.