ui: rendering performance follow-up#26097
Open
allozaur wants to merge 5 commits into
ggml-org:masterggml-org/llama.cpp:masterfrom
allozaur:allozaur/ui-rendering-performance-refactorallozaur/llama.cpp:allozaur/ui-rendering-performance-refactorCopy head branch name to clipboard
Open
ui: rendering performance follow-up#26097allozaur wants to merge 5 commits intoggml-org:masterggml-org/llama.cpp:masterfrom allozaur:allozaur/ui-rendering-performance-refactorallozaur/llama.cpp:allozaur/ui-rendering-performance-refactorCopy head branch name to clipboard
allozaur wants to merge 5 commits into
ggml-org:masterggml-org/llama.cpp:masterfrom
allozaur:allozaur/ui-rendering-performance-refactorallozaur/llama.cpp:allozaur/ui-rendering-performance-refactorCopy head branch name to clipboard
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Follow-up UI performance work focused on reducing per-render CPU cost during streaming by memoizing expensive parsing/formatting paths and improving tool-result lookup behavior in the chat message renderer.
Changes:
- Add bounded memoization caches for frequently re-run helpers (tool-call JSON parsing, partial JSON scanning, syntax highlighting, LaTeX preprocessing, search-results parsing, tool-result classification/line splitting).
- Replace per-tool-call linear search of tool result messages with an indexed lookup to avoid O(n^2) behavior across many tool calls.
- Add unit tests intended to prevent regressions in tool-call parsing and lookup performance.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/ui/tests/unit/parse-toolcalls-memo.test.ts | Adds unit tests for tool-call parsing memoization and tool-message lookup behavior. |
| tools/ui/src/lib/utils/search-results.ts | Memoizes parsing of tool-result search output and extraction of query from tool args. |
| tools/ui/src/lib/utils/parse-partial-json-args.ts | Adds a bounded cache around partial JSON arg parsing/scanning. |
| tools/ui/src/lib/utils/latex-protection.ts | Refactors LaTeX processing to reuse constants and memoizes preprocessing results. |
| tools/ui/src/lib/utils/code.ts | Adds a bounded cache for highlight.js output keyed by language/autoDetect/code. |
| tools/ui/src/lib/utils/agentic.ts | Uses Map/Set for faster tool message lookup; memoizes tool result parsing/classification and toolCalls JSON parsing. |
| tools/ui/src/lib/constants/latex-protection.ts | Introduces reusable LaTeX-related constants/regexes used by the preprocessing pipeline. |
| tools/ui/src/lib/components/app/chat/ChatMessages/ChatMessage/ChatMessageToolCall/ChatMessageToolCallBlockSearchResults.svelte | Minor cleanup of derived variable typing. |
| tools/ui/src/lib/components/app/chat/ChatMessages/ChatMessage/ChatMessageToolCall/ChatMessageToolCallBlockEditFile.svelte | Removes an extra blank line. |
| tools/ui/src/lib/components/app/chat/ChatMessages/ChatMessage/ChatMessageToolCall/ChatMessageToolCallBlockDefault.svelte | Removes unused type import and reorders derived declarations. |
Comments suppressed due to low confidence (2)
tools/ui/tests/unit/parse-toolcalls-memo.test.ts:31
- The "returns the same array reference" test doesn't actually assert memoization (or reference identity) and would pass even if JSON.parse runs every time. Consider asserting that JSON.parse is only invoked once for the same toolCalls JSON string across repeated deriveAgenticSections calls.
it('returns the same array reference for the same JSON string', () => {
// parseToolCalls is not exported, but deriveAgenticSections uses it
// internally. We verify memoization through behavior: calling
// deriveAgenticSections twice with the same toolCalls should not
// re-parse (which we verify by checking the returned sections
tools/ui/tests/unit/parse-toolcalls-memo.test.ts:124
- This test uses a wall-clock timing assertion (
elapsed < 100ms), which is prone to flakes across CI environments and doesn't directly assert the intended regression (avoiding per-tool-call Array#find). A deterministic check is to overridetoolMessages.findto throw and ensure deriveAgenticSections still succeeds.
// If the lookup were still O(n^2), this would be noticeably slow
const start = Date.now();
const sections = deriveAgenticSections(msg, toolMessages, [], false);
const elapsed = Date.now() - start;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
lovely! probably makes sense to push this through and rebase #26098 onto it once this lands? they're pretty complimentary. either way works for me – just holler. |
This file contains hidden or 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
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.
Overview
Follow-up PR with architectural improvements after #26053
Additional information
Requirements