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

Memory leak in v5? (works in v4.10) #894

Closed
1 task done
alecor191 opened this issue Feb 5, 2024 · 12 comments
Closed
1 task done

Memory leak in v5? (works in v4.10) #894

alecor191 opened this issue Feb 5, 2024 · 12 comments

Comments

@alecor191
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

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:

  const optimizelyClient = Optimizely.createInstance({
    datafile,
    sdkKey: ...,
    datafileOptions: {
      autoUpdate: true,
      updateInterval: 60000, // 1 minute
    },
    logLevel: 'WARNING',
    sdkKey: undefined, // Ensure the SDK doesn't try to fetch the datafile on its own
    datafileOptions: {
      autoUpdate: false, // Ensure the SDK doesn't also try to auto-update on its own
    },
    eventBatchSize: 10,
    eventFlushInterval: 30000,
    odpOptions: {
      disabled: true,
    },
  });

Here some typical memory consumption of our app when using optimizely-sdk version 4.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.

optimizely-4

If we upgrade to 5.0.0 (no other change) and start up our app, memory consumption goes beyond 2GB (sometimes even >4GB):

optimizely-5

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 use 4.x.

Recent Change

No. Just upgrade to 5.0.0.

Conflicts

No response

@mikechu-optimizely
Copy link
Contributor

@alecor191 Thanks for the report. We have another similar report as well.

Would you mind calling an explicit optimizelyClient.close()?

@mikechu-optimizely
Copy link
Contributor

If you don't mind, also post another profiler output graph.

@mikechu-optimizely
Copy link
Contributor

Based on some heap analysis, we've identified some points to refactor.

@mikechu-optimizely
Copy link
Contributor

I've opened #895.

I'll also be curious to know if explicitly calling .close() addresses the memory leak.

@mikechu-optimizely
Copy link
Contributor

I've received an update from another client that the explicit .close() call addresses the problem. We're also updating our documentation to stress this importance, especially in high traffic JS scenarios.

The PR from yesterday reduces the heap allocation when ODP is expressly disabled.

@alecor191
Copy link
Author

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 .close() on shutdown of the web server will likely have little to no effect, as the service may run for hours/days and handle thousands of calls to the Optimizely client, right?

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.

@mikechu-optimizely
Copy link
Contributor

Hi @alecor191.

I'm generally in favor of reusing a single optimizelyClient instance across requests as long as the server main thread is guaranteed to remain active.

In a FaaS scenario where the main thread may be out of your control and disposed, instantiating and later calling .close() within the route call may be a better option. In the standard NodeJS scenario as you provided, it's best to ensure .close() is called to wind down the background batching processes.

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).

@alecor191
Copy link
Author

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 optimizely-sdk back up to v5. We'll monitor memory consumption in production in the coming days. I'll let you know in case we still see any anomaly.

(also happy to upgrade to 5.0.1 once available).

@mikechu-optimizely
Copy link
Contributor

Thanks for the collab on this. 👍

@mikechu-optimizely
Copy link
Contributor

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 import { createInstance } from "@optimizely/optimizely-sdk/lite" (which we use for edge deployments) is more memory conserving.

Just an update...

@mikechu-optimizely
Copy link
Contributor

I just published the 5.0.1 version. The key update is not instantiating the ODP classes when

createInstance({
    // ....
    odpOptions: {
      disabled: true,
    },
})

@mikechu-optimizely
Copy link
Contributor

I am just off a client call and setting disabled: true for those that do not use ODP is an accurate solution for handling memory.

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 Symbol.dispose and Symbol.asyncDispose to come to server & clients)

Feel free to comment more as needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants