-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: optimize ReasoningBlock performance by isolating timer re-renders #8000
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
- Extract timer logic into memoized ElapsedTime component - Prevent full component tree re-renders every second - Only render markdown when content exists - Fixes #7999: CPU spike from 1Hz re-renders in long conversations
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.
| ) | ||
| }) | ||
|
|
||
| ElapsedTime.displayName = "ElapsedTime" |
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.
Great job isolating the timer re-renders! However, this component could benefit from test coverage. Given this is a performance-critical fix, consider adding tests to verify that:
- ElapsedTime only re-renders when its props change
- The parent ReasoningBlock doesn't re-render on timer updates
- Timer cleanup works correctly
| * - Timer runs while reasoning is active (no persistence) | ||
| * - Timer is isolated in a memoized component to prevent full re-renders | ||
| */ | ||
| 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.
Consider also wrapping the ReasoningBlock component itself with React.memo. While you've isolated the timer, the parent could still benefit from memoization to prevent unnecessary re-renders when none of its props have changed:
| const id = setInterval(tick, 1000) | ||
| return () => clearInterval(id) | ||
| } else { | ||
| setElapsed(0) |
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 resetting elapsed to 0 here intentional? This might cause a brief flash if the component transitions from active to inactive and back quickly. Consider keeping the last elapsed value when inactive, or only resetting when the component unmounts.
| {(content?.trim()?.length ?? 0) > 0 && ( | ||
| {hasContent && ( | ||
| <div className="px-3 italic text-vscode-descriptionForeground"> | ||
| <MarkdownBlock markdown={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.
Following up on nabilfreeman's comment about fast output models - have you considered adding debouncing for the markdown content updates? Fast streaming could still trigger many re-renders of the MarkdownBlock even with your timer isolation fix.
| ts: number | ||
| isStreaming: boolean | ||
| isLast: boolean | ||
| metadata?: any |
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.
Minor type safety improvement: The prop is typed as . Consider defining a proper interface for better type safety:
- 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 by @hannesrudolph
|
We should instead look into the way we handle streaming for normal messages and see how is it different from the reasoning blocks, there might be an optimization that is missing from it. |
Summary
This PR addresses Issue #7999 by fixing the performance regression in the ReasoningBlock component that was causing CPU spikes due to 1 Hz re-renders during streaming.
Problem
After commit bbd3d98, the ReasoningBlock component was causing:
Solution
The fix isolates timer updates to prevent cascading re-renders:
Changes
ElapsedTimecomponent wrapped withReact.memohasContentcheckTesting
Performance Impact
Fixes #7999
Important
Optimizes
ReasoningBlockperformance by isolating timer re-renders and debouncing content updates to reduce CPU usage.ElapsedTimecomponent to prevent fullReasoningBlockre-renders.ReasoningBlockto limit re-renders during streaming.MarkdownBlockbased on content presence.ElapsedTimecomponent usingReact.memoto handle timer logic.debouncefor content updates inReasoningBlock.This description was created by
for 635eb98. You can customize this summary. It will automatically update as commits are pushed.