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

[contextLines]: reduce memory usage #12175

Closed
JonasBa opened this issue May 23, 2024 · 3 comments · Fixed by #12221
Closed

[contextLines]: reduce memory usage #12175

JonasBa opened this issue May 23, 2024 · 3 comments · Fixed by #12221
Assignees
Labels
Package: node Issues related to the Sentry Node SDK Type: Improvement

Comments

@JonasBa
Copy link
Member

JonasBa commented May 23, 2024

Problem Statement

ContextLinesIntegration reads files from error stack traces in order to show the surrounding source lines. When new errors occur, the SDK will read these files regardless of their size and store them in an LRU cach indefinitely (or until they are evicted) so that subsequent contexts can be resolved faster.

This can have a large impact on the amount of memory an application uses (especially in the case of bundling), which I suspect is also causing longer GC sweeps.

Solution Brainstorm

  • leverage process.getMemoryUsage to determine a smarter LRU cache size or the maximum file size that we are willing to read
  • periodically clear the LRU cache and reclaim memory
  • use readline to extract only the lines we are interested in

It is hard to say how effective 1 and 2 would be, made up limitations like these are brittle at best + we risk breaking the product in its current form without providing an explanation.

3 could bring a substantial improvement. If we assume that a particular line of code is likely to crash again (as opposed to a line from anywhere in that file), then the surrounding context lines could also be cached.

A good next step would be to benchmark readline vs readfile to see the performance characteristics and evaluate if we could reasonably replace it. One benefit of readline would be that we can avoid copying the file in order to split it by line, which I suspect is also the culprit of high memory usage right now.

@AbhiPrasad AbhiPrasad added the Package: node Issues related to the Sentry Node SDK label May 23, 2024
@AbhiPrasad
Copy link
Member

We should skip minified files via some of kind heuristic: https://github.com/mitsuhiko/might-be-minified. We can then store all of those in a LRU as well.

@timfish
Copy link
Collaborator

timfish commented May 23, 2024

https://github.com/mitsuhiko/might-be-minified

That regex is wild!

Can you not get a reasonably reliable heuristic by just looking at the line lengths?
Is there any point including context lines if they're over ~500 characters?

@AbhiPrasad
Copy link
Member

That regex is wild!

lol

hmm maybe a regex is too complicated. I think we try first to detect .min.js, and then look at line length like you said.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: node Issues related to the Sentry Node SDK Type: Improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants