-
Notifications
You must be signed in to change notification settings - Fork 358
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
patch highlight.js to address memory leak #6146
Conversation
@beyang The results are as follows: Before 5953:
After 5953:
Analysis:
|
@PriNova thanks for the info—that's super helpful. Could you run the same benchmark on this branch and see if that brings performance back to normal? |
@beyang
Pre-feature (copied from above):
With this patch:
In summary, this patch introduces a slightly faster Hz compared to the pre-feature baseline, but overall the results are similar to the post-feature + patch implementation. |
@beyang I have spoken with @PriNova to share his benchmark test so that I can perform these tests myself. My hunch is that the performance could get worse when there are more languages loaded. I have mentioned in my original pull request that I don't know which of the languages should be loaded and in the end the common languages were loaded alongside the ones I've mentioned in here #5874 (comment) There is indeed a difference in how highlight languages were used as part of the common dict If for some reason it's more performant todo it the 2.9 way I will add another pull request to modify but for now more testing is needed. After the testing of the number of languages loaded I will also test this patch so please don't dismiss this work yet as it might still prove useful. Personally, I'm not such a fan of the vitest bench because I had one value for the test, commented 2 languages and I got a higher number than before, some average numbers probably need to be added after at least 10 runs. |
I've looked through the benchmarks more closely, and I think there are 2 separate issues here:
Given where the absolute numbers are currently, I don't view a 30% increase in latency as a dealbreaker and the benefit of supporting additional languages is substantial. Regarding the flickering issue, we're using a patched newer version of highlight.js. I have not noticed it in this patch, though the issue was difficult to reproduce. We'll keep an eye on this, but my assessment is to stick with highlight.js (and merge this change) to support highlighting across more languages. If we find the flickering issue appearing again, we will revert all these commits in this order: |
@beyang prior to my initial pull request #5953 was this commit Line 1431 in a22ba43
It loaded lowlight 3.1 which brings highlight 11.9 which was loading within the Cody web. There are 29 extra languages loaded now as before my pull requests (now a total of 54 from 25) At a minimum this pull request #6118 shouldn't be rolled back since it fixes the csharp syntax highlighting and avoids the extra loading of highlight.js 11.9.0 which gets rid of an extra 278kb from the bundle. If you notice certain slowdowns based on some metrics you could remove some of the languages from here My plan is to perform a devtools traceback with and without this patch, and with and without the extra languages and without 2 versions of highlight.js loaded by Cody web and then I will know for certain the impact of this library as I don't really trust the vitest bench as an indicator of real-world performance impact. |
super helpful, thanks for the detailed analysis @ichim-david |
Pull in highlightjs/highlight.js@19819d5
This patch can be removed (
[email protected]
) once[email protected]
is released and we've updated to it.Test plan
N/A