-
Notifications
You must be signed in to change notification settings - Fork 24
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
fix(kcodeblock): improve matching perf #2562
Conversation
…Lines` ...and extraxt normalizers to utilities, add unit tests.
✅ Deploy Preview for kongponents-sandbox ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kongponents ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Thanks for digging into this! Looks great!
We spoke offline, and I mentioned I'm gonna have a play with this in place (we/kuma-gui originally reported a performance issues with KCodeBlock in Slack).
I had a relatively quick scan of the code, but I for one am happy you've done this with some significant care, so I'm pretty confident it'll be a big improvement judging by the description and the overall approach (also thanks taking the time with the detailed description!)
I left a couple of Q's mainly for myself, so let me know on those. I'll try and give this a spin in our application today and pop back later 👍 , could end up being after the weekend but hopefully today.
P.S. But don't wait for me if you get an approval from elsewhere and want to go ahead and merge |
The old implementation already denounced the query input with a delay of 150ms. |
Oh I see! Maybe we could increase it a bit? Say to 1000ms? Maybe a bit too much hassle but am I right in thinking we'd only want the debounce when there are less than say 3 characters in the field and after say 3 characters not bother debouncing? Not totally sure. Anyway, was just a suggestion, if you like it maybe its for another PR, if not no bother 👍 |
A long debounce only makes it feel slow. I think current delay is find and I think in most cases it should be fast enough now. Also the speed depends on multiple factors, eg. Whether there are a lot of matches or does the code contain characters that need escaping, etc. |
Yep no prob 👍 |
Thanks @johncowen this has been fixed |
const debouncedHandleSearchInputValue = debounce(handleSearchInputValue, 150) | ||
// Use Lodash’s debounce function as it supports leading and trailing options. | ||
// Adding `leading: true` to the options ensures that the search is triggered immediately when the user types the first character, | ||
// mak |
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.
Maybe we should switch to lodash-es
's debounce instead of providing our own one for the rest of the codebase in the future.
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.
I think this would be ideal if you'd like to create a separate JIRA. We'd just need to check the usage in all components and ensure we aren't doing anything special (and that it's not exported)
In 58dec25 I added You can give it another try. @johncowen |
Ah perfect! So this is to only render the bits we need to see which would result in less DOM nodes on the page at one time? Lemme take this one for a spin! |
I had a quick look here at this next iteration, and I'm not totally sure if its any better or not. I couldn't see any DOM recycling happening when looking in Web Inspector, but I might have misunderstood where I should expect that. How can I validate the the DOM recycling/virtual scrolling thing is happening? Is it only in certain cases in certain areas? I'd understood it was only for the line numbers? Is that right? |
Where exactly did you look? You can verify this in the preview sandbox. If DOM recycling is functioning properly, you should see the line elements updating as you scroll. recycle.webm |
In our application not the sandbox
Ah awesome yeah, that was what I was expecting to see but didn't 🤔. Lemme try again in a bit |
Did you reinstalled the preview package (and make sure there's no cache or sth)? |
I previously had problems just installing via So I hack-ily checked out this branch separately, built it, copied the resulting source from |
const debouncedHandleSearchInputValue = debounce(handleSearchInputValue, 150) | ||
// Use Lodash’s debounce function as it supports leading and trailing options. | ||
// Adding `leading: true` to the options ensures that the search is triggered immediately when the user types the first character, | ||
// mak |
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.
I think this would be ideal if you'd like to create a separate JIRA. We'd just need to check the usage in all components and ensure we aren't doing anything special (and that it's not exported)
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.
lgtm
## [9.17.1](v9.17.0...v9.17.1) (2025-01-16) ### Bug Fixes * **kcodeblock:** improve matching perf ([#2562](#2562)) ([8525a29](8525a29))
🎉 This PR is included in version 9.17.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Preview package from this PR in consuming applicationIn consuming application project install preview version of kongponents generated by this PR:
|
Summary
KM-888
This PR introduces further performance enhancements to the
<KCodeBlock>
component while addressing multiple bugs.Key improvements
Correctly escape HTML and RegExp
Resolved that when highlighting matched characters in filtered code:
RegExp
in exact match mode, eg..
was actually highlighting all characters.Reduce redundant
updateMatchingLineNumbers
callsPrevented calls to
updateMatchingLineNumbers
whenisShowingFilteredCode
changes, avoiding unnecessary processing.Optimized line matching logic
Previously, for each match index, the implementation split all text from the beginning to
pos
ormatch.index
in order to get the line number for each match, leading to high computational costs (O(n*m)
, wheren
is code length andm
is the number of matches).The new approach precomputes line offsets and uses binary search to determine line numbers, significantly reducing overhead.
Support for multi-line regular expressions
Fixed a bug preventing regular expression patterns from matching across multiple lines.
Optimized character highlighting in filter mode
Escaping HTML for highlight output now involves additional work compared to the old implementation, but further optimizations have made the process significantly faster in most cases.
Efficient Highlight Processing: The previous approach split the filtered code into individual lines, highlighting each one separately. It not only failed to properly escape the input code but was also creating a new
RegExp
instance during every iteration. The new approach processes the entire code at once, usingRegExp.prototype.exec
to identify matches and perform both matching and escaping in a single pass.Selective escaping: Escapes only
<
and&
since the output HTML is isolated (v-html
), reducing escape complexity (compared to escaping a broader set of characters like<
,>
,'
,"
, and&
).Fast path for non-escaping cases: Introduced a quick pre-check using
/[<&]/
to determine if escaping is necessary, enabling a fast path for cases without special characters. The newescapeIfNeeded
function adopts this optimized approach, replacing the original escape implementation that executed upon component render. The previous method, which appliedreplaceAll
to the entire input for each character in the escape list, was sub-optimal. While the performance difference is negligible for short inputs, the new approach significantly improves highlighting speed in most use cases while maintaining correctness.Merging adjacent matches: For short queries or patterns like
/./
that generate many consecutive matches, adjacent match marks are merged. This reduces the number of extra elements and accelerates both calculation and rendering. This is achieved by wrapping the RegExp in a grouping construct(:...)+
. This improves speed for both calculation and rendering.Significant improvement in Rendering Performance
Following the optimization of the matching and highlighting algorithms, the primary bottleneck shifted to the rendering process. Using Chrome’s Performance Profiler, we identified that the most time-consuming operation was “Recalculate Style.” To address this, we replaceddisplay: grid
, which can be inefficient for a large number of elements, with a more static layout. Additionally,content-visibility
was applied for line number elements. These changes resulted in a substantial boost in rendering performance.content-visibility: auto
hides elements from the render tree and only renders them when they scroll into view or when their metrics are accessed programmatically (e.g.,getBoundingClientRect
,offsetHeight
, etc.). However, this can introduce unpredictable issues.One issue I encountered involves the interaction between our current DOM structure and 1Password’s heuristics. 1Password attempts to identify labels associated with form inputs and iterates through all line number elements and retrieves their layout information one by one. This process forces the browser to repeatedly calculate layouts for each previously hidden element styled with
content-visibility: auto
.In the case of a code input with a large number of lines, this behavior causes the page to freeze when focusing on the search input within the code block for the first time.
Ultimately, I introduced a new dependency,
virtua
, which provides virtual scrolling primitives to fully resolve the issue. And now the bottleneck switched back to the matching algorithm but it's already fast enough for most our use cases.Benchmarks
Input code:
Doesn't contain
<
or&
(Ops/s)
(Ops/s)
"true"
" "
"true"
" "
/true/
/./
/true/
/./
"true"
" "
/true/
/./