Skip to content
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

Merged
merged 1 commit into from
Nov 21, 2024
Merged

Conversation

beyang
Copy link
Member

@beyang beyang commented Nov 18, 2024

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

@PriNova
Copy link
Collaborator

PriNova commented Nov 18, 2024

@beyang
Benchmarking #5953 against parent commit results in a significant decrease in performance of ~32% with a test file of 153 LOCs (complex MarkDown).

The results are as follows:

Before 5953:

 RERUN  webviews/components/MarkdownFromCody.tsx x2

 ✓ webviews/components/MarkdownFromCody.bench.tsx (2) 7642ms
   ✓ MarkdownFromCoding Benchmarks (2) 7640ms
     name                   hz      min     max     mean      p75      p99    p995    p999     rme  samples
   · simple markdown    575.96   0.9412  6.9830   1.7362   1.9816   4.9281  5.1614  6.9830  ±3.62%      576   fastest
   · complex markdown  17.1838  46.8364  101.37  58.1944  59.7748  88.2357  101.37  101.37  ±2.82%      100


 BENCH  Summary

  simple markdown - webviews/components/MarkdownFromCody.bench.tsx > MarkdownFromCoding Benchmarks
    33.52x faster than complex markdown

After 5953:

RERUN  x1

 ✓ webviews/components/MarkdownFromCody.bench.tsx (2) 9513ms
   ✓ MarkdownFromCoding Benchmarks (2) 10263ms
     name                   hz      min      max     mean      p75     p99     p995     p999     rme  samples
   · simple markdown    553.18   1.0384  13.9507   1.8077   1.7401  6.1468  12.3092  13.9507  ±5.90%      557   fastest
   · complex markdown  11.6925  69.0036   162.28  85.5247  88.1649  160.60   162.28   162.28  ±3.44%      100


 BENCH  Summary

  simple markdown - webviews/components/MarkdownFromCody.bench.tsx > MarkdownFromCoding Benchmarks
    47.31x faster than complex markdown

Analysis:

  1. Performance Impact Analysis:

    • Simple markdown shows ~4% performance decrease
    • Complex markdown shows ~32% performance decrease
    • The gap between simple and complex cases has widened
  2. Statistical Significance:

    • RME (Relative Margin of Error) increased:
      • Simple: 3.62% → 5.90%
      • Complex: 2.82% → 3.44%
    • Sample sizes remained similar:
      • Simple: 576 → 557
      • Complex: 100 → 100
  3. Time measurements:

    • Mean execution time increased:
      • Simple: 1.73ms → 1.80ms
      • Complex: 58.19ms → 85.52ms

@beyang
Copy link
Member Author

beyang commented Nov 19, 2024

@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 beyang disabled auto-merge November 19, 2024 19:41
@PriNova
Copy link
Collaborator

PriNova commented Nov 20, 2024

@beyang
I made an additional performance test on this patch compared to the baseline of pre-feature implementation. The results are very similar before the patch and with feature implementation:

Analysis points:

* Hz difference: ~31.5% lower than baseline
* Mean execution time: ~46% higher than baseline
* RME remains very stable (only 0.04% difference)

Pre-feature (copied from above):

 RERUN  webviews/components/MarkdownFromCody.tsx x2

 ✓ webviews/components/MarkdownFromCody.bench.tsx (2) 7642ms
   ✓ MarkdownFromCoding Benchmarks (2) 7640ms
     name                   hz      min     max     mean      p75      p99    p995    p999     rme  samples
   · simple markdown    575.96   0.9412  6.9830   1.7362   1.9816   4.9281  5.1614  6.9830  ±3.62%      576   fastest
   · complex markdown  17.1838  46.8364  101.37  58.1944  59.7748  88.2357  101.37  101.37  ±2.82%      100


 BENCH  Summary

With this patch:

 RERUN  x2

 ✓ webviews/components/MarkdownFromCody.bench.tsx (2) 10249ms
   ✓ MarkdownFromCoding Benchmarks (2) 10246ms
     name                   hz      min     max     mean      p75     p99    p995    p999     rme  samples
   · simple markdown    608.34   0.9930  6.2213   1.6438   1.7088  4.9464  5.3630  6.2213  ±3.59%      609   fastest
   · complex markdown  11.7673  73.1362  167.89  84.9811  87.0385  140.83  167.89  167.89  ±2.86%      100


 BENCH  Summary

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.

@ichim-david
Copy link
Collaborator

@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
https://github.com/wooorm/lowlight/blob/5caa24559b7ff858e3009f10545ee19ef9c64d00/lib/common.js
vs main:
https://github.com/wooorm/lowlight/blob/main/lib/common.js
which is how I did in my work.

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.

@beyang
Copy link
Member Author

beyang commented Nov 20, 2024

I've looked through the benchmarks more closely, and I think there are 2 separate issues here:

  • One is avoiding the flickering issue caused by a previous version of highlight.js
  • The other is assessing the impact of slightly slower syntax highlighting

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:

@ichim-david
Copy link
Collaborator

@beyang prior to my initial pull request #5953 was this commit

"lowlight": "^3.1.0",

It loaded lowlight 3.1 which brings highlight 11.9 which was loading within the Cody web.
highlight-js

There are 29 extra languages loaded now as before my pull requests (now a total of 54 from 25)
https://gist.github.com/ichim-david/a7354fec9a293998770eb41752da815c

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.
latest-web-analyze

If you notice certain slowdowns based on some metrics you could remove some of the languages from here
https://github.com/sourcegraph/cody/blob/main/vscode/webviews/utils/highlight.ts#L56
since now you are in full control of what syntaxes you want to have loaded.

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.

@beyang
Copy link
Member Author

beyang commented Nov 21, 2024

super helpful, thanks for the detailed analysis @ichim-david

@beyang beyang merged commit 25d3bc0 into main Nov 21, 2024
17 of 18 checks passed
@beyang beyang deleted the bl/patch-highlightjs branch November 21, 2024 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants