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

Critical memory issue since 1.0.21 #3242

Closed
MaximeGratens opened this issue Apr 25, 2024 · 15 comments
Closed

Critical memory issue since 1.0.21 #3242

MaximeGratens opened this issue Apr 25, 2024 · 15 comments
Assignees

Comments

@MaximeGratens
Copy link

MaximeGratens commented Apr 25, 2024

Describe the bug
Hello,
Since 1.0.21 and using the ini
To Reproduce
Steps to reproduce the behavior:

  1. Install sdk react 2 with remix
  2. Use the initiate node sdk inside loader
  3. Deploy in a production server and monitore

Expected behavior
A clear and concise description of what you expected to happen.
The memory or cpu of the server will increase until it will crash

Screenshots
If applicable, add screenshots to help explain your problem.
image

  • at 12 I deployed the 1.0.23 without the node setup

Additional context
Add any other context about the problem here.
We deploy the sdk 1.0.23 without the initiate node and you can see in the screen that the memory issue is fixed

@samijaber samijaber self-assigned this Apr 26, 2024
@mikehuebner
Copy link

Seeing this as well on 1.0.23, it just crashed our beanstalk application with a bunch of errors, removing the initializeNodeRuntime causes the below error. However, just removing the initializeNodeRuntime fixes the issue.

[Builder.io]:  Failed code evaluation: [Builder.io]: could not import `isolated-vm` module for safe script execution on Node server.      In certain Node environments, the SDK requires additional initialization steps. This can be achieved by   importing and calling `initializeNodeRuntime()` from "@builder.io/sdk-react/node/init". This must be done in   a server-only execution path within your application.   Please see the documentation for more information: https://builder.io/c/docs/integration-tips#enabling-data-bindings-in-node-environments    { code: 'state.posts' }

@MaximeGratens
Copy link
Author

Seeing this as well on 1.0.23, it just crashed our beanstalk application with a bunch of errors, removing the initializeNodeRuntime causes the below error. However, just removing the initializeNodeRuntime fixes the issue.

[Builder.io]:  Failed code evaluation: [Builder.io]: could not import `isolated-vm` module for safe script execution on Node server.      In certain Node environments, the SDK requires additional initialization steps. This can be achieved by   importing and calling `initializeNodeRuntime()` from "@builder.io/sdk-react/node/init". This must be done in   a server-only execution path within your application.   Please see the documentation for more information: https://builder.io/c/docs/integration-tips#enabling-data-bindings-in-node-environments    { code: 'state.posts' }

If you remove the initializeNodeRuntime and use symbols, your site's performance will be degraded.

@samijaber
Copy link
Contributor

We just published some perf improvements: reusing an Isolate VM context across all data binding evaluations. This should reduce memory usage during SSR.

Available at 1.0.25, does not require any change besides updating the version.

https://github.com/BuilderIO/builder/releases/tag/%40builder.io%2Fsdk-react%401.0.25

@samijaber
Copy link
Contributor

@MaximeGratens If you can try this new version and let me know how it fares performance-wise, that would be great.

@MaximeGratens
Copy link
Author

MaximeGratens commented Apr 29, 2024

I get this in local and production : TypeError: Cannot destructure property 'ivmIsolateOptions' of 'undefined' as it is undefined.

@kaceycleveland
Copy link

As a work around for now @MaximeGratens , can pass an empty object to initializeNodeRuntime:

initializeNodeRuntime({})

@kevintruby
Copy link

Wow, what timing! I was going to open a similar issue for the Vue SDK, but now it might make more sense to comment here instead.

I've been delaying a major upgrade from the @builder.io/sdk-vue package's 10/2023 0.7.0 release to the latest, because our E2E tests were experiencing periods of downtime when testing a staging site. Upon investigation it seemed the introduction of isolated-vm at version 0.7.1 or 0.7.2 (could never get 0.7.1 to work) alone resulted in a major memory leak.

It took some time to dig into what part of our content caused this, because some pages behaved fine. In our case, any version that included isolated-vm prior to this 1.0.25 update struggled with pages that included symbols that loop through a List input's contents and render images. Our use case was a symbol that could generate a row of CTAs for our e-commerce site.

I recreated a very narrow version of the issue with a quick Nuxt repo for SSR plus a lightweight Docker container that could be used to watch memory use over time, like so:

Screenshot 2024-04-29 at 5 47 33 PM

Will keep that around just in case, but I updated to 1.0.25 and the narrow reproduction no longer spikes. Applying this to my actual site's environment also appears to behave as expected. I'll know for sure after automated tests run on this build, but I feel confident after examining the local Docker container.

Figured this could be anecdotal evidence of the fix for other SSR environments 🤞

@MaximeGratens
Copy link
Author

MaximeGratens commented Apr 30, 2024

We're in production with version 1.0.25 and the initializeNodeRuntime({}) function;

So far, we haven't seen any crashes or memory limits (we're testing on a multiple instance with 512mb of ram).

The main page of our product takes between 1 and 2 seconds to load, which isn't very SEO-friendly and isn't what you're selling on your website. We can't use a cache because these pages are loaded from google or facebook ads with different query parameters each time.
image

We always have very slow pages and it's the same thing as @kevintruby, it's when we try to loop.

You have this inside issiue #3240

@MaximeGratens
Copy link
Author

image
Here is the last 6 hours with the the 1.0.25 in production.

As you can see we had a crash few minutes ago.

Also, the memory graph is completely jagged.

@samijaber
Copy link
Contributor

@kevintruby Thanks for sharing all of these insights. By reusing Isolate contexts across all data-binding evaluations, the latest change should get rid of memory leaks. Do let me know if there are any persistent issues when you deploy your Vue SDK test to production.

@MaximeGratens Thanks for testing the latest version. It sounds like this issue may be specific to the builder feature of iterating over an array of dynamically fetched data? I will respond in your other issue.

@samijaber
Copy link
Contributor

I am closing this issue as the specific memory leak caused by isolated-vm has been resolved by reusing a single IsolatedVM Context per render.

The unrelated performance issue tied to repeated blocks on data bindings can be tracked here #3240

@samijaber
Copy link
Contributor

Additionally: I patched initializeNodeRuntime so that you do not need to provide an empty object as a parameter.

See v1.0.26: https://github.com/BuilderIO/builder/blob/main/packages/sdks/output/react/CHANGELOG.md#1026

@MaximeGratens
Copy link
Author

MaximeGratens commented May 2, 2024

@samijaber I don't think the problem has been really solved.

2 grafs of 2 differents projects using builder and remix.

image
image

Both server are crashing after sometimes, we are using 1.0.26. This is the last 24 hours of our production server.

@khiemtong
Copy link

As another data point, I can confirm that this fix resolved our memory issues with NextJS@14.2.3 and @builder.io/sdk-react@1.0.25

@samijaber
Copy link
Contributor

Thanks folks. @MaximeGratens we are actively investigating the other sources of memory issues that you are facing and will respond to them in #3240

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

No branches or pull requests

6 participants