Closed
Bug 654399
Opened 14 years ago
Closed 14 years ago
Prevent rapid memory growth on pages with ShareThis: cache the string buffer for location.hash
Categories
(Core :: DOM: Core & HTML, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla6
People
(Reporter: ferdinand.hertog, Assigned: bzbarsky)
References
()
Details
(Keywords: memory-footprint, Whiteboard: fixed-in-cedar)
Attachments
(1 file, 2 obsolete files)
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110502 Firefox/6.0a1
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110502 Firefox/6.0a1
When I open that page memory usage of Firefox grows from 50MB to over 300MB(it seems to plateau). Chrome doesn't do this and IE9 doesn't do this. I have tried this on a clean profile without extensions or plugins and it still happens. Firefox 3.6, Firefox 4.0 and Firefox nightly have this behavior and the newer the Firefox the more it leaks.
Reproducible: Always
I have saved the full page in a zip file if that is needed.
Updated•14 years ago
|
Comment 1•14 years ago
|
||
I tried this page under Massif on a Linux64 build. I ran for a bit over 1,000 seconds and got this graph for heap consumption:
MB
225.6^ ##
| @@# ::
| @@@# ::::
| :@@@@# @:::::
| :::@@@@# ::@:::::
| :::::@@@@# ::::@:::::
| @::::::@@@@# @:::::@:::::
| :@@::::::@@@@# ::@:::::@:::::
| :::@@::::::@@@@# ::::@:::::@:::::
| @@:::@@::::::@@@@# @:::::@:::::@:::::
| @@@@:::@@::::::@@@@# ::@:::::@:::::@:::::
| ::@@@@:::@@::::::@@@@# ::::@:::::@:::::@:::::
| ::::@@@@:::@@::::::@@@@# ::::::@:::::@:::::@:::::
| ::::::@@@@:::@@::::::@@@@# ::::::::@:::::@:::::@:::::
| @:::::::@@@@:::@@::::::@@@@# :: ::::::@:::::@:::::@:::::
| @@@:::::::@@@@:::@@::::::@@@@# ::::: ::::::@:::::@:::::@:::::
| :@@ @:::::::@@@@:::@@::::::@@@@# :::: :: ::::::@:::::@:::::@:::::
| @@:@@ @:::::::@@@@:::@@::::::@@@@# :: :: :: ::::::@:::::@:::::@:::::
| @:@@:@@ @:::::::@@@@:::@@::::::@@@@# :: :: :: ::::::@:::::@:::::@:::::
| @:@@:@@ @:::::::@@@@:::@@::::::@@@@# :: :: :: ::::::@:::::@:::::@:::::
0 +----------------------------------------------------------------------->s
0 1065.3
Definitely looks like a GC happened in the middle.
The snapshot taken at the peak usage point was dominated by a single
allocation point:
97.77% (231,262,896B) (heap allocation functions) malloc/new/new[], --alloc-fns, etc.
o1o> 84.59% (200,084,363B): nsStringBuffer::Alloc() (nsSubstring.cpp:209)
| o2o> 84.37% (199,574,102B): nsAString_internal::MutatePrep() (nsTSubstring.cpp:162)
| | o3o> 84.35% (199,537,048B): nsAString_internal::ReplacePrepInternal() (nsTSubstring.cpp:198)
| | | o4o> 84.29% (199,381,282B): nsAString_internal::Replace() (nsTSubstring.h:684)
| | | | o5o> 84.28% (199,364,478B): nsLocation::GetHash() (nsTSubstring.h:378)
| | | | | o6o> 84.28% (199,364,478B): NS_InvokeByIndex_P ()
| | | | | o7o> 84.28% (199,364,478B): XPCWrappedNative::CallMethod() (xpcwrappednative.cpp:3141)
| | | | | o8o> 84.28% (199,364,478B): XPC_WN_GetterSetter() (xpcprivate.h:2621)
| | | | | o9o> 84.28% (199,364,478B): js::Invoke() (jscntxtinlines.h:277)
| | | | | o10> 84.28% (199,364,478B): js::ExternalInvoke()
(jsinterp.cpp:806)
| | | | | o11> 84.28% (199,364,478B): js::ExternalGetOrSet() (jsinterp.cpp:846)
| | | | | o12> 84.28% (199,364,478B): js::JSProxyHandler::get() (jsproxy.cpp:133)
| | | | | o13> 84.28% (199,364,478B): js::JSProxy::get() (jsproxy.cpp:806)
| | | | | o14> 84.28% (199,364,478B): js::proxy_GetProperty() (jsproxy.cpp:923)
| | | | | o15> 84.27% (199,325,984B): InlineGetProp() (jsobj.h:1224)
roc, bz: any idea what that represents?
Something calling document.location.hash a lot, which allocates strings, and GC not running very frequently?
Comment 3•14 years ago
|
||
I instrumented by hand nsAString_internal::MutatePrep() to see how big the allocated string was each time it was called. I saw this sequence of sizes:
sz = 505
sz = 1132
sz = 2018
sz = 1009
repeat over and over again. So clearly something is happening repeatedly to cause the memory growth.
Comment 4•14 years ago
|
||
The 505, 1009, 2018 sequence looks like a "double the hash table" kind of thing.
Assignee | ||
Comment 5•14 years ago
|
||
So what GetHash does is it takes the given nsAString (aHash) and:
405 aHash.Assign(PRUnichar('#'));
406 aHash.Append(unicodeRef);
I see unicodeRef being 579 PRUnichars every time; the actual value looks like this:
"init/fpc=5568770-12fc8ba4ec9-156bff66-2/sessionID=1304742795995.65072/publisher=ffc93a4f-833d-4566-8...."
The caller is, surprise!, the sharethis script at http://w.sharethis.com/share4x/js/st.c58d43164c1cfdb83ea49cd898a81f54.js and after prettifying the relevant part is:
fragmentPump = new function () {
...
this.initialize = function () {
this.fragTimer = setInterval("fragmentPump.checkFragment()", 5)
};
this.checkFragment = function () {
var hash = document.location.hash.substring(1);
// Do various crud here with that hash
...
};
this.initialize()
}
So yes, every 5ms they get the hash (which in this case is huge for some reason) and then do stuff with it. This generates about 1200 bytes bytes of garbage (not counting the JS string overhead) per 5ms, which is 240KB of garbage per second. Or about 12MB/minute. Of course these are "external" strings so the JS GC doesn't realize it's holding on to that much data. Then eventually there's enough pure-JS garbage and there's a GC and all this stuff goes away.
For 1000s, by the way, I'd expect about 240MB of garbage per the numbers above. Matches the comment 1 graph pretty well, esp. if we assume that we can't quite maintain a 5ms timer while running under Massif.
On 3.6, the "leak" would be 2x slower at least because the timeout clamp is 10ms. But I think 3.6 also GCs more aggressively than 4 does.
On any browser with a generational GC that GCs often (e.g. Chrome; dunno about IE) there would be no problem, I'd think, because it would just GC all the strings often.
For what it's worth, I've reported this exact issue (the fact that they peg the CPU with checkFragment for no good reason) to ShareThis in the past; last time in March of 2010. Clearly no action yet!
I think on our side short of going to a GGC the useful thing would be to teach JSeng to treat external strings as part of its heap... or something (in some cases they really are shared with the browser core, so this could have undesirable effects too).
Assignee: nobody → general
Status: UNCONFIRMED → NEW
Component: General → JavaScript Engine
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → general
Summary: Page keeps leaking memory → Page has memory grow for a good long while before a GC ever happens
Would it be worth having nsLocation::GetHash cache the last URI/stringbuffer pair and return that stringbuffer when the URI hasn't changed?
Assignee | ||
Comment 7•14 years ago
|
||
Hmm. That would fix this one particular case, but the problem would remain for other cases where a new XPCOM stringbuffer is created and handed out to JS.
I guess we should spot-fix window.location for now to work around the ShareThis inanity, and then file a followup bug on an overall better solution....
Assignee | ||
Comment 8•14 years ago
|
||
For what it's worth, I tried updating the gc stats with the new memory and it helps some: we start GCing after our heap reahes 170MB instead of 300MB...
I guess I'll do that _and_ roc's suggestion from comment 6. :(
Assignee | ||
Comment 9•14 years ago
|
||
And if I do both, memory stabilizes at 70MB.
Assignee | ||
Comment 10•14 years ago
|
||
Attachment #531076 -
Flags: review?(jorendorff)
Assignee | ||
Comment 11•14 years ago
|
||
Attachment #531077 -
Flags: review?(roc)
Assignee | ||
Comment 12•14 years ago
|
||
This will still suck for other string-getting use cases, but we really do need GGC for that. Or seriously revamping how strings work, or both.
Assignee: general → nobody
Component: JavaScript Engine → DOM
Keywords: mlk
Priority: -- → P1
QA Contact: general → general
Whiteboard: [need review]
Comment 13•14 years ago
|
||
Jason said I should take a look.
We already update the mallocCounter for "length" within JSExternalString::new_.
Is this additional memory you want to account for?
Assignee | ||
Comment 14•14 years ago
|
||
Hrm. So we do. I'd missed that.
In this case the stringbuffer capacity happens to be a good bit bigger than the length (almost 2x as big), so that's undercounting in some cases and overcounting in others (the shared buffer case), but I guess that means we don't want the part 1 from this bug....
Comment on attachment 531077 [details] [diff] [review]
part 2. Try to make sure we always hand out the same exact string buffer each time location.hash is gotten.
Technically I'm not a peer in this code, but the patch looks good.
Attachment #531077 -
Flags: review?(roc) → review+
Assignee | ||
Updated•14 years ago
|
Attachment #531076 -
Flags: review?(jorendorff) → review-
Assignee | ||
Comment 16•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #531077 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #531076 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Whiteboard: [need review] → [need landing]
Updated•14 years ago
|
Whiteboard: [need landing] → [need landing][checkin-needed]
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → bzbarsky
Assignee | ||
Comment 17•14 years ago
|
||
Flags: in-testsuite?
Whiteboard: [need landing][checkin-needed] → fixed-in-cedar
Target Milestone: --- → mozilla6
Comment 18•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
Summary: Page has memory grow for a good long while before a GC ever happens → Prevent rapid memory growth on pages with ShareThis: cache the string buffer for location.hash
Comment 20•13 years ago
|
||
Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:13.0) Gecko/20100101 Firefox/13.0
I just experienced huge memory growth on this page with sharethis code:
http://pages.ebay.com/sellerinformation/news/insiderapr2012.html
In about:memory it was more than 1GB of memory related to sharethis, in several minutes!
Do this bug should be reopened or should I fill a new bug?
Assignee | ||
Comment 21•13 years ago
|
||
Please file a new bug with clear steps to reproduce and cc me?
Comment 22•13 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #21)
> Please file a new bug with clear steps to reproduce and cc me?
Hi Boris,
https://bugzilla.mozilla.org/show_bug.cgi?id=751656
just filled and you are cc.
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•