SdchDictionaryFetcher wastes a bunch of memory |
||
Issue descriptionSdchDictionaryFetcher loads an entire response into a std::string, and then when it's done, clears() the string. So what's the problem? string::clear is not guaranteed to free memory taken up by a string. Instrumenting Android shows that SdchDictionaryFetcher allocates a bunch of memory in creating this string, and it's never freed. Assuming identical strings aren't internally refcounted on Android (Something I don't believe the C standard allows, to make string[foo]='h' take linear time), then this means the OS is not freeing the memory. Experimentally, we tend to waste 100k-300k of RAM this way. There are a bunch of ways to fix this: 1) std::string::shrink_to_fit. Unfortunately, this is not yet allowed. It's also only a hint that it free to be ignored. 2) std::swap(dictionary_, std::string()) should work, but is a little silly. 3) Make dictionary_ a unique_ptr, and just delete it. 4) std::move the dictionary when passing it around. We could even remove a copy by doing this, if we stopped passing stuff around by reference, but that's a bit tricky, and isn't too common in Chrome. 5) Or we could pass it to the SdchManager as a unique_ptr. While the copy isn't a huge deal, I'd suggest we go with 5), just because it's simple, and bit clearer than all the other approaches except 3).
,
Jun 17 2016
And, for the record, it turns out SdchDictionary actually only keeps a copy of a subset of the dictionary string (It removes the SDCH headers), so I decided to keep the copy and go with option 3) instead of 5).
,
Jun 17 2016
,
Jun 17 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3bbfea923d0edbbde05223076f969ac6a44ba234 commit 3bbfea923d0edbbde05223076f969ac6a44ba234 Author: mmenke <mmenke@chromium.org> Date: Fri Jun 17 18:38:57 2016 Make SdchDictionaryFetcher free more memory when done. It was just calling std::string::clear() on the string it downloads dictionaries to when a fetch is complete, which may not free the memory taken up by the string. As the fetcher exists for the lifetime of the Profile, and this buffer can be a couple hundred k, this can waste a fair bit of RAM. BUG= 620853 Review-Url: https://codereview.chromium.org/2075033003 Cr-Commit-Position: refs/heads/master@{#400464} [modify] https://crrev.com/3bbfea923d0edbbde05223076f969ac6a44ba234/net/url_request/sdch_dictionary_fetcher.cc [modify] https://crrev.com/3bbfea923d0edbbde05223076f969ac6a44ba234/net/url_request/sdch_dictionary_fetcher.h
,
Jul 27 2016
You *might* also be able to shave a tiny bit of memory usage by calling reserve with the exact amount of space needed before calling append: some compilers may allocate less memory that way (not blind exponential growth).
,
Jul 27 2016
Thanks for the suggestion! I suspect that wouldn't make enough of a difference to be worth it - we destroy the string when we're done downloading it, so it shouldn't be around long enough for having, say, 512k instead of 400k to matter much. The code to move part of the string to its final location only does a single std::string(const string& str, size_t pos) call, which presumably (hopefully?) doesn't allocate extra memory on the end. |
||
►
Sign in to add a comment |
||
Comment 1 by xunji...@chromium.org
, Jun 16 2016