-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix[gen2]: try to not use a global isolate instance and create fresh instance for each request #4210
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
Conversation
…memory leaks - Removed global IVM_CONTEXT and related functions to ensure isolates are created anew for each request. - Updated `setIvm` to utilize options per request instead of globally. - Enhanced `runInNode` to initialize the isolate and context directly within the function, improving memory management and cleanup. - Added comments to clarify changes and rationale for the new approach. This refactor aims to enhance performance and reliability by eliminating potential memory leaks associated with persistent isolates.
…roved memory management - Added `IVM_OPTIONS` to store isolate options, defaulting to a memory limit of 128. - Updated `setIvm` to accept and store options for each request, enhancing flexibility. - Modified `runInNode` to utilize stored options when creating isolates, ensuring consistent memory management. - Improved comments for clarity on the new approach and its benefits. This refactor aims to further optimize memory usage and isolate management in the node runtime.
🦋 Changeset detectedLatest commit: e327221 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
View your CI Pipeline Execution ↗ for commit e327221
☁️ Nx Cloud last updated this comment at |
- Eliminated the unused `Context` import from `isolated-vm` in the node-runtime file. - This change simplifies the code and improves readability by removing unnecessary dependencies. This refactor contributes to maintaining a clean and efficient codebase.
sanyamkamat
left a comment
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.
- add changeset. Recommendation for Major upgrade as we are changing how we handle JVM garbage collection here.
samijaber
left a comment
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 agree with this PR: we should find a way to dispose of the IVM isolate at the end of a request to avoid memory leaks.
However, this PR goes to the other extreme and creates a new Isolate for every individual data binding. If the content item has 1,000 dynamic data bindings, it should be more performant to reuse the isolate instance for all of its data bindings.
I would suggest the following:
- double check the impact of this PR on pages with very large amounts of data bindings (1000s)
- if my assumption is correct, and the test shows a noticeable degradation in performance, then we should find a way to reuse the IVM isolate for all bindings within the same
<Content>render.
…d remove previous changeset
Description
The Strategy:
This guarantees memory cleanup because the entire memory block is freed by C++.
Testing
Notion doc for dev testing - https://www.notion.so/builderio/ENG-10865-testing-memory-leak-fix-2cb3d7274be5804988edd838631a6b62
Screenshot
Note
Switches Node runtime to create a fresh isolated-vm per request and dispose it after execution, addressing memory leaks; updates changeset to bump minor versions across SDKs.
packages/sdks/.../node-runtime.ts):ivm.Isolateper request using storedIsolateOptionsand dispose it infinally.Context/shared isolate; eliminatesetIsolateContext/getIsolateContext.setIvmto accept optional options and store them for per-request use.@builder.io/*SDKs and notes memory leak elimination.Written by Cursor Bugbot for commit e327221. This will update automatically on new commits. Configure here.