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.

Sign up for GitHub

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

Jump to bottom

nwjs seems to leak memory via tray menu items #6583

Closed
jakedouglas opened this issue Apr 25, 2018 · 27 comments
Closed

nwjs seems to leak memory via tray menu items #6583

jakedouglas opened this issue Apr 25, 2018 · 27 comments
Assignees
@rogerwang
Labels
bug P2 triaged WIP

Comments

@jakedouglas
Copy link

NWJS Version : v0.30.1
Operating System : macOS 10.13.4. Other platforms might be affected as well.

The documentation is not explicit about how a tray/menu should be refreshed/manipulated after creation. However, I have tried the following approaches that all seem to result in increasing memory usage:

  1. Simply reassigning tray.menu a new Menu object that contains new MenuItem objects
  2. Using menu.removeAt() to remove all menu items before adding new ones
  3. Using tray.remove(), creating a new Tray object, then assigning a new Menu object
  4. Approaches 2 and 3 combined

Expected behavior

Memory usage should not grow indefinitely as the tray menu is refreshed.

Actual behavior

Memory usage continues to grow as the tray menu is refreshed.

How to reproduce

<!DOCTYPE html>
<html>
  <head>
    <title>Hi</title>
  </head>
  <body style="width: 100%; height: 100%;">
    <script>
      const tray = new nw.Tray({ title: 'Tray' });

      setInterval(() => {
        const menu = new nw.Menu();

        menu.append(new nw.MenuItem({ type: 'normal', label: 'A' }));
        menu.append(new nw.MenuItem({ type: 'normal', label: 'B' }));
        menu.append(new nw.MenuItem({ type: 'normal', label: 'C' }));

        tray.menu = menu;
      }, 10)
    </script>
  </body>
</html>
@Christywl
Copy link
Contributor

Try the sample on Linux/Windows/Mac with nwjs-sdk-v0.30.1, the issue is reproduced.

@rogerwang rogerwang added the WIP label May 2, 2018
@rogerwang
Copy link
Member

This is fixed in git and will be available in the next nightly build.

@jakedouglas
Copy link
Author

Thank you! Will this be in a release soon?

@rogerwang
Copy link
Member

rogerwang commented May 2, 2018 via email

@rogerwang
Copy link
Member

btw, the build is here and it's in release quality:
https://dl.nwjs.io/live-build/nw30/05-03-2018/0f1a574-2c58808-684c6a5-4ae2873/v0.30.3/

@jakedouglas
Copy link
Author

Thanks!

@rogerwang
Copy link
Member

Is the nightly build working for you?

@jakedouglas
Copy link
Author

Sorry, I should have checked earlier. It's still leaking. Collections seem to be happening resulting in temporary drops, but the baseline continues to increase. Note that the example seems to produce the leak more quickly when the nwjs window is focused.

Test case after ~1 minute:

image

Test case after ~10 minutes:

image

@rogerwang
Copy link
Member

Thanks. What about Windows and Linux there?

@jakedouglas
Copy link
Author

It seems to still be leaking on Windows and Linux as well.

@rogerwang
Copy link
Member

It works for me. Here is the screenshot after running for 10mins, with the nightly build posted above and your example. @Christywl could you please verify as well?
screen shot 2018-05-09 at 9 15 41 am

@jakedouglas
Copy link
Author

My mistake, I was accidentally using option 4 from the issue description to produce the screenshot and Windows/Linux results above.

However, using only the provided example, I still seem to have a leak, albeit slower. This was taken after ~25 minutes:

image

What else can I provide?

@rogerwang
Copy link
Member

Numbers like that doesn't necessarily mean there is a leak. Chances are that the GC hasn't kicked in.

And it still works for me after running for an hour:
screen shot 2018-05-09 at 12 19 21 pm

btw, you could also compare the number there with 0.30.2. At least the menu and menuitems leaks are fixed in the nightly.

@Christywl
Copy link
Contributor

I check on Mac with the nightly build v0.30.3:
the window isn't focused(~30 minutes):
screen shot 2018-05-09 at 13 29 15
the window is focused(~30 minutes):
screen shot 2018-05-09 at 13 55 47

@rogerwang
Copy link
Member

@Christywl thanks. Does it keeps growing?

The specific number doesn't matter because it's decided by GC strategy of v8, which in turn depends on system configuration...

@Christywl
Copy link
Contributor

yes, it keeps growing, but is slower than 0.30.1.

@rogerwang
Copy link
Member

Does it have upper limit? or it just fills up system memory? And what about the release version on this.

@jakedouglas
Copy link
Author

Do you mean that v8 might decide to continue using more memory until it's close to running out, and only GC then? The example has gone over 300mb on my system now after an hour or two. When should it stop growing in relation to available memory?

If this is normal GC behavior and not a leak, is there a way for us to specify the v8 GC settings to collect earlier? The current behavior is likely to give users the impression that our application is a memory hog.

image

@rogerwang
Copy link
Member

The GC strategy is a complex topic. It's hard to predict when GC will happen. If you'd like to control it, pass --js-flags=--expose_gc and call gc() when you need it.

There are various v8 flags to tune GC, see https://github.com/nwjs/v8/blob/nw30/src/flag-definitions.h#L625
See also https://github.com/nwjs/v8/blob/nw30/src/api.cc#L933

0.30.3 will be released today with the fix for Menu and MenuItem leak. We'll continue to find if there are any other leaks with your sample. You can use the heap profiling tool in devtools to see which JS objects are live in the heap after running it for several minutes. Please share your result here.

@rogerwang
Copy link
Member

btw, you might want to try this for testing purpose: --js-flags="--optimize_for_size --max_old_space_size=460 --gc_interval=100"

@jakedouglas
Copy link
Author

By the end of yesterday the example had grown to over 1GB. Today, on the same system, I can no longer reproduce that growth and my results are more like yours. I'm thoroughly confused as to what could have changed.

I can still reproduce the growth on another system (Windows). I am adding --js-flags=--expose_gc, and scheduling a call to gc() every 5 seconds. It does not appear to keep the process from growing indefinitely. Here's some information from that system:

image

image

Nothing obvious is standing out to me since the bulk of the leaked memory seems to be outside of the JS object system and I don't have any sense of what should or shouldn't be present. I've attached both heap snapshots so you can examine them. They're .txt files in order for them to be attached to the issue but they're actually JSON.

Let me know what else I can provide.

Heap_Snapshot_A.txt
Heap_Snapshot_B.txt

@rogerwang
Copy link
Member

It does not appear to keep the process from growing indefinitely

By "indefinitely" what's the maximum number seen there? And does it lead to out of memory crash? Is there any difference if the arguments are given as in #6583 (comment) ?

@rogerwang
Copy link
Member

btw, the heap snapshot shows only the live objects in JS heap. It doesn't include the objects that are dead and to be garbage collected. So a normal heap snapshot under increasing memory doesn't imply leak outside of the JS object system. It could be that GC just marked some objects but hasn't decided to return their memory to the system.

@jakedouglas
Copy link
Author

Makes sense. The process calling gc() repeatedly is now around 450mb. I haven't gotten a chance to see whether it OOMs yet. I'll try with the other GC flags you provided and see what happens.

@rogerwang
Copy link
Member

It could be that GC just marked some objects but hasn't decided to return their memory to the system.

To understand this, see 'mark-sweep' in http://jayconrod.com/posts/55/a-tour-of-v8-garbage-collection

And calling gc() doesn't guarantee v8 will perform a full GC. It just give v8 a hint, and v8 will make its decision with the application's request.

@jakedouglas
Copy link
Author

Using --js-flags="--optimize_for_size --max_old_space_size=460 --gc_interval=100", the process grew to ~980mb, then shrank to ~850mb for a while, then ~350mb before it began growing again to over 400mb.

We're going to move forward with v0.30.3, hoping that the problem we initially experienced will be improved or fixed in practice.

It does seem problematic to me though that memory usage is so difficult to reason about, and that default settings can result in occupying 1GB or more in an example application that does not intentionally hold any significant amount of data in memory. Our nwjs application is run by end users who notice things like ostensibly high memory usage, and it can give them a poor impression. Even if we get an opportunity to explain that the memory might be reclaimed if something else on their system needed it, it's not an easy conversation to have.

Thanks for your help!

@rogerwang
Copy link
Member

I agree with you that 1GB memory consumption is big but this is from an extreme benchmark that allocating objects every 10ms. The real world application may or may not behave like this. Note that the default v8 behavior on this is tuned for trade-off between garbage collection throughput, latency, and memory consumption. See more information here: https://v8project.blogspot.com/2016/10/fall-cleaning-optimizing-v8-memory.html

And it can always be tuned for your use case with appropriate js-flags for smaller memory footprint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rogerwang rogerwang

Labels
bug P2 triaged WIP
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@jakedouglas @rogerwang @Christywl