search

QuinnDamerell / Baconit

A beatiful, powerful, reddit client for Windows 10.
https://www.reddit.com/r/BaconitDev/
GNU General Public License v2.0
242 stars 81 forks source link

Reduce the Memory Footprint of Baconit #17

Open QuinnDamerell opened 8 years ago

QuinnDamerell commented 8 years ago

While I was hacking way at code I didn't have much time to keep track of memory. I (or someone) needs to run through the app and do some debugging on the memory usage to see what we can do.

mconnew commented 8 years ago

I had a very quick look at a profiled run of BaconIt and a large part of the problem is how you make requests to the reddit servers. In NetworkManager.MakeGetRequest, you call HttpClient.SendRequestAsync, which by default will buffer the entire response body before it returns. You then call HttpContent.ReadAsStringAsync. As you are using the native HttpClient, this will allocate a COM string in native code, and then marshal it to managed code in effect doing double allocation of the string. Adding this to the fact that the original data is buffered in an IBuffer on the native side, you effectively have triple allocation to obtain your response as a string. You would have less memory allocation if you did the following:

  1. Use the overload of HttpClient.SendRequestAsync which takes an HttpCompletionOption and specify ResponseHeadersRead. This will prevent the response being buffered in an IBuffer.
  2. Use the method ReadAsInputStreamAsync on the IHttpContent object. You can then wrap this as a .Net Stream using the winRT interop wrapper extension methods. This removes your two string allocations (native and managed).
  3. Use the JSON.Net mechanism to deserialize from a stream.
QuinnDamerell commented 8 years ago

This is amazing! Thanks so much! I will take a crack at this a little late or someone else can take a look if they beat me to it!

This is why I love open source, I am by no means an expert on any of these APIs (or even profiling) so any more advise, help with perf, or help with memory is greatly appreciated!

mconnew commented 8 years ago

One downside of the approach I suggested is that JSON.Net will read from the stream synchronously, which will potentially cause an extra thread being used blocking on the read. If this is running on the UI thread, this could affect responsiveness of the app. There is an open issue JamesNK/Newtonsoft.Json#650 to add Async support to JSON.Net but it hasn't been started yet because it's a complicated task.
There is CPU performance problem with the WebView component where it keeps firing an event which looks related to redrawing the window repeatedly and is using up quite a bit of CPU. The call stack suggests a timer expired which caused the code to be called. Opening the same page in the edge browser directly doesn't have the same problem. I haven't had time to dig in further, and don't know when I'll get a chance, but thought I'd let you know there's an issue there.

QuinnDamerell commented 8 years ago

Thanks for the heads up, I made sure it was running on another thread. I actually ran into that problem a while back with the serializing and figured it out back then.

I just threw in an implementation of what you are talking about, take a look at let me know what you think:(https://github.com/QuinnDamerell/Baconit/commit/51d52d56236647abb7cc1e595b6d25981a27c68a)

As for the WebView issue, I was hunting something similar a while back but gave up. If you have any leads or need any help feel free to let me know, I would love to help. If you start to think it is a platform issue give me some evidence and I can start a thread with the IE team about it. :)

mconnew commented 8 years ago

The implementation looks good. I just did another quick profile and while the memory usage does still spike, it doesn't spike as high as it used to, and the GC recovers the memory very quickly. Previously you were allocating a lot on the LOH which takes a long time to be cleaned up and requires an expensive gen 2 GC to happen. There is still one area where you are allocating large strings, and that's in the settings. On my machine, the settings are being read into a string which is 400K in size. You can do the same serializing to/from a stream for the local settings and reduce that memory footprint. You also have a small risk of data corruption/loss in the settings as you rewrite it. If a crash happens while writing, you blow away the file. If you saved to a temporary file then copied that file over the top of the original file, you can ensure that you never end up with a half written file. I don't know how durable that file needs to be though.

QuinnDamerell commented 8 years ago

Thanks for the settings suggestion, I have updated that also. Most of the important settings are in the roaming data and I haven't seen any trouble the file getting corrupt, so I left that issue for now. Thanks for pointing it out!

ambertx commented 8 years ago

I've just noticed that on my Surface Pro 3, the latest version of Baconit has a memory leak somewhere. When opening a post with a link to a website it was taking ~60% cpu and was up to 5Gb of RAM before I closed it.

Finding the problem code is a bit above me at the moment, just thought i'd mention it