Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

fix: Fix Chrome stack overflow during highlighting #64072

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

varungandhi-src
Copy link
Contributor

@varungandhi-src varungandhi-src commented Jul 25, 2024

Using the spread operator with large arrays can trigger a
stack overflow in Chrome/V8. For example, see:

In a highlighting context, we can have 10k-100k occurrences
in a file, so let's avoid using the spread operator.

Fixes https://linear.app/sourcegraph/issue/GRAPH-772

Test plan

Manually tested against sample file.

CleanShot 2024-07-25 at 11 10 43@2x

Changelog

  • Fixes a Chrome-specific stack overflow when highlighting large files.

@cla-bot cla-bot bot added the cla-signed label Jul 25, 2024
@github-actions github-actions bot added team/graph Graph Team (previously Code Intel/Language Tools/Language Platform) team/product-platform labels Jul 25, 2024
Copy link
Member

@eseliger eseliger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonder what a flamegraph of this looks like, and if some sort of chunking (like push 1k at a time) could help in case it's bad - but approving for the fix.

@varungandhi-src
Copy link
Contributor Author

Wonder what a flamegraph of this looks like, and if some sort of chunking (like push 1k at a time) could help in case it's bad

This operation should only be happening once per blob, so I don't think it's worth micro-optimizing the pushes; the lookups are much more expensive.

@varungandhi-src varungandhi-src merged commit 2644e24 into main Jul 25, 2024
16 checks passed
@varungandhi-src varungandhi-src deleted the vg/fix-stackoverflow branch July 25, 2024 03:50
sourcegraph-release-bot pushed a commit that referenced this pull request Jul 25, 2024
Using the spread operator with large arrays can trigger a
stack overflow in Chrome/V8. For example, see:
- nodejs/node#16870

In a highlighting context, we can have 10k-100k occurrences
in a file, so let's avoid using the spread operator.

Fixes https://linear.app/sourcegraph/issue/GRAPH-772

## Test plan

Manually tested against sample file.

![](https://github.com/user-attachments/assets/e096c664-063e-44ed-a991-72629af36651)

## Changelog

- Fixes a Chrome-specific stack overflow when highlighting large files.

(cherry picked from commit 2644e24)
varungandhi-src added a commit that referenced this pull request Jul 25, 2024
…64074)

Using the spread operator with large arrays can trigger a
stack overflow in Chrome/V8.

In a highlighting context, we can have 10k-100k occurrences
in a file, so let's avoid using the spread operator.

Fixes https://linear.app/sourcegraph/issue/GRAPH-772

## Test plan

Manually tested against sample file.

![CleanShot 2024-07-25 at 11 10 43@2x](https://github.com/user-attachments/assets/e096c664-063e-44ed-a991-72629af36651)

## Changelog

- Fixes a Chrome-specific stack overflow when highlighting large files.
 <br> Backport 2644e24 from #64072

Co-authored-by: Varun Gandhi <[email protected]>
@camdencheek
Copy link
Member

Good find -- thank you for the fix!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport 5.5.x cla-signed team/graph Graph Team (previously Code Intel/Language Tools/Language Platform) team/product-platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants