Optimize the rendering loop in the disassembler widget#263
Optimize the rendering loop in the disassembler widget#263
Conversation
There was a problem hiding this comment.
Pull request overview
This PR optimizes the rendering loop in the disassembler widget by implementing scroll coalescing, highlight throttling, and refactoring the document update mechanism to reduce UI flicker and improve performance.
Key changes include:
- Added scroll coalescing using a QTimer to batch multiple scroll events into a single scroll operation
- Implemented highlight throttling to defer expensive highlighting operations when the cursor position changes rapidly
- Refactored
refreshDisasmto build HTML content in a single string and set it once, rather than inserting HTML line-by-line
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| src/widgets/DisassemblyWidget.h | Added timer members for scroll coalescing and highlight throttling, plus state tracking variables |
| src/widgets/DisassemblyWidget.cpp | Implemented optimization timers, refactored document rendering to use bulk HTML setting, converted breakpoint lookups to QSet for O(1) performance, and replaced signal connect/disconnect pattern with guard flags |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (wordChanged || mLastHighlightedWord.isEmpty()) { | ||
| extraSelections.append(createSameWordsSelections(mDisasTextEdit, curHighlightedWord)); | ||
| mLastHighlightedWord = curHighlightedWord; | ||
| } |
There was a problem hiding this comment.
The optimization to skip recomputing same-word selections has a critical flaw. When a word is cached (mLastHighlightedWord is set), the extraSelections list does not include the previous same-word selections. This means calling setExtraSelections will remove all previously highlighted words. The same-word selections need to be either re-appended from a cache or always recomputed.
| [this](int count) { | ||
| mPendingScrollLines += count; | ||
| if (!mScrollCoalesceTimer.isActive()) { | ||
| mScrollCoalesceTimer.start(16); // ~1 frame | ||
| } | ||
| }); |
There was a problem hiding this comment.
The scroll coalescing timer restarts on every scroll event (line 167), but the timer interval is only 16ms. If continuous scroll events arrive faster than 16ms apart, the pending scroll will keep accumulating without ever executing. This could lead to a delayed or "jumpy" scroll experience. Consider using a different strategy such as immediate execution for the first scroll and throttling subsequent ones, or using a shorter fixed delay.
| // Block all signals and disable UI updates during document rebuild | ||
| const QSignalBlocker docBlocker(mDisasTextEdit->document()); | ||
| const QSignalBlocker scrollBlocker(mDisasTextEdit->verticalScrollBar()); | ||
| mDisasTextEdit->setUpdatesEnabled(false); |
There was a problem hiding this comment.
QSignalBlocker is used on mDisasTextEdit->document() and the scrollbar, but the cursorPositionChanged signal comes from mDisasTextEdit itself (connected at line 176). This signal blocker won't prevent cursorPositionChanged from firing when the document is modified. The mIgnoreCursorPositionChanged flag should be set to true before these operations and reset to false afterward.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
2 participants
Checklist
Description