4.46 KB, patch
|Details | Diff | Splinter Review|
It turns out that both nsString and nsCString have recyclers... So, if you call nsString::Recycle() the nsString instance is placed on a nsDeque. Unfortunately, once strings are in the recycler, they are never used again :-( They just hang out in the nsDeque for the lifetime of the app and ultimately leak! I'm attaching a patch which removes the recycler lists from both nsString and nsCString. -- rick
Created attachment 17358 [details] [diff] [review] Patch to remove recyclers from nsString and nsCString
This particular bit of stupidity is my fault. The intent was to allow folks to call CreateString(), which (does) retrieve a string from the recycle deque. RickP: when I run the top 100, I don't see this getting called at all. Is there a situation where it get's called a bunch? Mail perhaps?
Wait a second. If you read the code closely, which I just did, it shows that the static string recycler is destroyed at shutdown. It in turn creates a deallocator which deletes all the strings in the deque. RPotts: are you sure this is a leak at all? (Again, I'm not even sure it's used).
hey rick, I was observing three things: 1. nsHTMLDocument recycles 2 nsStrings - The last-modified-date and the referer (if they are set). More strings are recycled if either SetLastModified(...) or SetReferrer(...) are called. However, in the simple page transition case I did not see these methods being called. 2. nsString::CreateString(...) is never called. 3. Even if nsString::CreateString(...) were called, it would return NULL. The code in nsString::CreateString() fails to capture the result from GetRecycler().CreateString(). So, unfortunately, result will always be NULL :-( When I found this, I was focusing on inter-page bloat. It looks like the strings do get deleted from the recycler at shutdown, but during the lifetime of the app, they just build up... It is not a huge leak - it is just another case where a few things get lost during a page transition... Unfortunately, I don't see any *huge* leaks during page transitions - just lots of small ones... -- rick
The patch looks good. I don't actually see any callers of |CreateString| anywhere in the app (which is good, since you wouldn't really want |nsString|s allocated on the heap very often). The pattern appears in the IMAP parser, but it implements this function itself. Maybe we could totally get rid of |CreateString| at the same time? Any caller should really use the more idiomatic |new nsString| in any case, if that's what they mean. Why obfuscate? I'm happy to sr= this as is, but would like it even more if you went the extra step and totally chopped out |CreateString|. sr=scc (but feel free to attach a new patch that completely removes |CreateString|)
Is this related to bug #65219 that was just marked fixed?
Yeah, I think this is a duplicate. Rick(s) ... I'm going to mark it a duplicate of bug #65219, for which I just checked in dbaron's patch. *** This bug has been marked as a duplicate of 65219 ***
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.