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? #479

Closed
msg7086 opened this issue Sep 15, 2018 · 9 comments

Comments

Projects
None yet
2 participants
@msg7086
Copy link

commented Sep 15, 2018

Got a few boxes running for long, and I noticed that SyncTrayzor is using a little more than necessary now.

synctrayzor

I have restarted the application on most of the nodes, but thought maybe I should leave 1 or 2 for debugging purpose. Most of them use 1.5-2.5GB private working set, and the sole purpose of syncthing is to sync some datasets between them.

@canton7

This comment has been minimized.

Copy link
Owner

commented Sep 15, 2018

Mind taking a look in your log? You'll find it under File -> Settings -> Logging -> Show SyncTrayzor Log File. You should see lines containing "SyncTrayzor.Services.MemoryUsageLogger" - mind pasting a few of those corresponding to high memory usage in Process Explorer?

This looks like it might be similar to #37: this was something to do with the embedded browser component, but we weren't able to narrow down what was causing it.

@msg7086

This comment has been minimized.

Copy link
Author

commented Sep 16, 2018

2018-09-16 00:46:22.6382 #6248 [Info] SyncTrayzor.Services.MemoryUsageLogger: Working Set: 144MiB. Private Memory Size: 88MiB. GC Total Memory: 2GiB 

Same number for the past 3 days, from the box that I screenshot with.

Feel free to give me diagnostic tools against that process if necessary.

@msg7086

This comment has been minimized.

Copy link
Author

commented Sep 16, 2018

Also this code prevents the detailed number from being logged. I checked the log on the one that has 1.5GB usage, showing GC total memory of 1GiB.

@msg7086

This comment has been minimized.

Copy link
Author

commented Sep 16, 2018

synct2

@msg7086

This comment has been minimized.

Copy link
Author

commented Sep 16, 2018

After sacrificing one of the process, I got some debug information for you.

Statistics:
              MT    Count    TotalSize Class Name
(...)
00007ffa84106fc0     1736       420168 System.Object[]
00007ffa8410e9a0     5331       426480 System.Signature
00007ffa8410c988     1649       640213 System.Byte[]
00007ffa8410f100     5845       654640 System.Reflection.RuntimeMethodInfo
00007ffa84107a98      709      1433372 System.Char[]
00007ffa84106948    18534      2394150 System.String
00000150cf5d9b80  1001798     71503266      Free
00007ffa840fbd30       20    134284224 System.IO.Stream[]
00007ffa8410b478 12280503   1277172312 System.IO.UnmanagedMemoryStream
@canton7

This comment has been minimized.

Copy link
Owner

commented Sep 16, 2018

Thanks for the sleuthing! This doesn't look like #37: that was characterised by high working set but low GC Total Memory. This does look like a memory leak somewhere in managed code, you're right.

(The following are notes that I made while investigating)

So it looks like we're leaking a very large number of very small UnmanagedMemoryStreams (104 bytes each).

One place where UnmanagedMemoryStream is used is in Assembly.GetManifestResourceStream, but I'm only calling that explicitly in 2 places, both load things larger than 104 bytes, and both properly dispose it. So it's not that.

It looks like FontFamily with a font from an embedded resource can also leak, but the only place I set the FontFamily explicitly is to Consolas, so we shouldn't be hitting that.

Attaching a memory profile, it looks like it's held by BitmapImage instances, holding images for the taskbar icon... They end up being kept alive like this:

roots

Reading the PackagePart source, it looks like PackagePart holds onto streams until they have been explicitly disposed -- no relying on the finalizer here -- so the thing which originally requested the stream isn't explicitly disposing it.

Digging further, the other sort of retention graph for instances is:

roots2

What's curious is that these BitmapImages should only ever be created once, and then reused. Indeed, looking at the dictionary above, it looks like there's a reference from the ResourceDictionary, as there should be. So it looks like something's recreating the ResourceDictionary...?

Although, this looks relevant.

Ah. Style's Setter with StaticResource explicitly allows a DeferredResourceStream. You end up here. And that's about where I lose the plot.

Ah, taking another tack -- seeing where the StaticResource is passed to -- we end up here. Specifically, here. It turns out that Icon doesn't take ownership of the Stream, so disposing the icon won't dispose that stream. Let's see if that makes a difference...

That seems to have worked - there are plenty of instances around, but the number of instances doesn't change over time, whereas it used to... So I think that's it!

@canton7

This comment has been minimized.

Copy link
Owner

commented Sep 16, 2018

I should really get another release out soon, and this will be included.

@msg7086

This comment has been minimized.

Copy link
Author

commented Sep 16, 2018

Unfortunately this only gets obvious after a long period of running. The one that has 2.5G of memory usage has been running for 6 months, and 1.5G's 3 months-ish. Guess we'll have to wait for a few weeks to verify if it actually fixes the issue. But I'm very impressed that you could find the root cause so quick. Good job!

@canton7

This comment has been minimized.

Copy link
Owner

commented Sep 16, 2018

@canton7 canton7 closed this in ee986a9 Oct 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.