-
Notifications
You must be signed in to change notification settings - Fork 305
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
Performance: fix memory leak #5460
Performance: fix memory leak #5460
Conversation
We have a lot of bug reports related to high memory usage and bad performance. This PR is an attempt to identify how we can fix these problems by creating a minimized reproduction of real-world usage scenarios. See `memory.test.ts` for the reproduction. In short, we use new record/replay infrastructure to replay events from a recording of my own coding session yesterday. When replaying these events, it's clear that the "external" memory usage of the agent process quickly grows up to 2gb due to tree-sitter parsing that we run on every document content change. When we disable tree-sitter parsing, the "external" memory usage stays stable below <1mb.
ec3378f
to
f1acdf0
Compare
We don't run this test in CI.
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 dont really know how it works to give concrete feedback but the test results lgtm. Approve to unblock!
@@ -8,6 +8,9 @@ import { type WrappedParser, createParser, getParser } from './parser' | |||
|
|||
const parseTreesPerFile = new LRUCache<string, Tree>({ | |||
max: 10, | |||
// Important: we need to call `Tree.delete()` to free up memory. Without | |||
// this, we leak memory. See CODY-3616. | |||
disposeAfter: tree => tree.delete(), |
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.
@abeatrix the short explanation of this fix is that tree-sitter trees are not garbage collected by default, they are stored on the Node.js "external" heap and we have to explicitly free them with .delete()
. FYI @valerybugakov
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.
🚀
Fixes CODY-3616
Previously, the Cody extension in all IDEs (including VS Code and JetBrains) had a memory leak causing the process to consume multiple GB of memory after long usage. This PR fixes this issue by calling
tree.delete()
on tree-sitter ASTs after they are evicted from an LRU cache.The fix itself is a one-liner. Most of the diff in this PR is adding the reproduction in
memory.test.ts
, which is a test that we skip by default but I think it's helpful to keep ti in the repo as documentation on how to reproduce memory leaks in the future.Test plan
memory.test.ts
to download and convert Cody NES recordings.Changelog