-
Notifications
You must be signed in to change notification settings - Fork 82
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
Memory leak in v5? (works in v4.10) #894
Comments
@alecor191 Thanks for the report. We have another similar report as well. Would you mind calling an explicit |
If you don't mind, also post another profiler output graph. |
Based on some heap analysis, we've identified some points to refactor. |
I've opened #895. I'll also be curious to know if explicitly calling |
I've received an update from another client that the explicit The PR from yesterday reduces the heap allocation when ODP is expressly disabled. |
Thanks a lot @mikechu-optimizely for the details. We're currently reviewing our code and noticed that we're creating an Optimizely client instance for each request in our API server. From the docs I understand that it should be sufficient to create a single instance and then reuse it across requests. If so, calling I.e. my question is: Is my understanding correct, that what we observed above is really related to the fact that we created many Optimizely client instances and didn't close them. And by moving to a single instance, we not only reduce wasting resources (allocation of a client instance per request), but also shouldn't suffer from the memory increase observed above. |
Hi @alecor191. I'm generally in favor of reusing a single In a FaaS scenario where the main thread may be out of your control and disposed, instantiating and later calling We'll be putting out a v5.0.1 which addresses a point of memory optimization that we found in load tests (along with some dependabot version bumps). |
Thanks @mikechu-optimizely for the insights. In our case we have a Node app running as Kubernetes pod that we control. We just changed the implementation to use a single instance for the lifetime of the app. We also bumped (also happy to upgrade to 5.0.1 once available). |
Thanks for the collab on this. 👍 |
I just tested 5.0.1 locally and the conditional loading through odpOptions.disabled:true is helping with memory. A super off-the-wall idea: I wonder if Just an update... |
I just published the 5.0.1 version. The key update is not instantiating the ODP classes when createInstance({
// ....
odpOptions: {
disabled: true,
},
}) |
I am just off a client call and setting We'll be enhancing our load testing strategy as well a reviewing the reports from ClinicJS to pin down the best implementation across our supported JS contexts. (I can't wait for Feel free to comment more as needed. |
Is there an existing issue for this?
SDK Version
5.0.0
Current Behavior
When we upgraded to
optimizely-sdk
v5.0.0, we started observing massive memory consumption increase on our NodeJS services.Expected Behavior
Memory consumption of v5.0.0 should be comparable to v4.x.
Steps To Reproduce
We use the following config:
Here some typical memory consumption of our app when using
optimizely-sdk
version4.10.0
: Memory consumption goes up to ~500MB and then remains stable. You'll see 2 nearly overlapping lines as we have 2 instance of the app running at a given point in time.If we upgrade to
5.0.0
(no other change) and start up our app, memory consumption goes beyond 2GB (sometimes even >4GB):SDK Type
Node
Node Version
v20
Browsers impacted
No response
Link
No response
Logs
No response
Severity
Blocking development
Workaround/Solution
I'm marking it as "blocking" because we can't ship our app with
5.0.0
. The workaround is to use4.x
.Recent Change
No. Just upgrade to
5.0.0
.Conflicts
No response
The text was updated successfully, but these errors were encountered: