Closed
Bug 1162953
Opened 9 years ago
Closed 5 years ago
page thumbnails have large memory impact
Categories
(Firefox :: New Tab Page, defect)
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: oliver.henshaw, Unassigned)
References
Details
(Whiteboard: [MemShrink:P2] )
Attachments
(5 files)
|
43.82 KB,
text/plain
|
Details | |
|
65.74 KB,
text/plain
|
Details | |
|
86.73 KB,
text/plain
|
Details | |
|
3.16 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
|
8.63 KB,
patch
|
Details | Diff | Splinter Review |
Recently the memory usage of firefox has ballooned by 1GB or more. I can see that this comes from images, and uncached images at that. With some investigation, detailed below, I can see that the problem is with page thumbnails on the new tab page: there are multiple copies of each in memory and they persist even when I've navigated to an actual web page. Normally firefox takes around 2.6GB (mostly because of an excess of tabs and windows, and perhaps 600MB+ of that is due to the adblock plus problem). But It's been even worse recently - here's a snippet from about:memory to illustrate the problem ├──1,084.55 MB (31.15%) -- images │ ├──1,065.61 MB (30.61%) -- uncached │ │ ├──1,065.31 MB (30.60%) -- raster/used │ │ │ ├────230.20 MB (06.61%) ++ (18 tiny) │ │ │ ├────122.21 MB (03.51%) -- image(moz-page-thumb://thumbnail?url=https%3A%2F%2Fevernote.com%2F) │ │ │ │ ├──122.21 MB (03.51%) ── source [272] │ │ │ │ └────0.00 MB (00.00%) ── decoded-heap [14] (The evernote.com thumbnail s just the top offender, there are several others with similar counts.) When I open the moz-page-thumb:// url and save the image I can see that the file is just 459KB. I can reproduce the problem with no extensions enabled and reproduce the symptoms (albeit on a smaller scale) with a fresh profile. Steps to Reproduce: * Create a new profile * Browse around a little to create at least one or two frecent websites * Open the new tab * Remove the pre-populated pages until some of the frecent sites appear on the new tab pages * Open about:memory Now: 1. Open the new tab page several times and take a memory measurement - I see the frecent page thumbnail appearing several times, though the object count doesn't quite correspond to the number of new tab pages = Expect exactly one object per thumbnail, no matter how many tabs are open. 2. Close the new tab pages and take a memory measurement + The contribution of page thumbnails drops to zero/near-zero 3. Open the new tab pages several times; for each one, click on a thumbnail to navigate away to a frecent page; take a memory measurement - The uncached image object continue to exist in memory. = Would expect them to vanish to near-zero, as in test #2. Note that test #3 is a little fuzzy, it sometimes seems to take a few goes to trigger. Tested on firefox 37.0, 37.0.1, 37.0.2 ==> BAD Tested on firefox 36.0 ==> BAD? [1] Tested on firefox 35.0 ==> partly GOOD [2] Tested on firefox 40.0a1 (2015-05-07) non-e10s [3] ==> BAD [1] Maybe it's a little harder to trigger the problem here [2] It does indeed seem to release memory when navigating from new tab page. But it still shows multiple copies of each thumbnail when multiple copies of the new tab page are open. [3] Tested with e10s disabled as the e10s about:memory has different sections for main process and Web Content - which is confusing. I think you can still see the problem in the main process images report but I wanted to keep it simple.
|
||
Comment 2•9 years ago
|
||
Based on this about:memory report it looks like every time imagelib is asked to load a moz-page-thumb url it will generate a new image (instead of using one from the image cache). The fact that there appears to be 272 copies uncached means there are 272 references to this thumbnail for this page still alive. If we want to use the cached image do we want any kind of invalidation behaviour (when to generate the thumbnail anew vs when to use the cached copy)? Or just always use the cached copy?
|
||
Comment 3•9 years ago
|
||
I just so happen to be investigating an issue with an image cache change I'm trying to land where a new assertion is failing. The assertion is in imgRequest::Init, checking that the URI the imgRequest is stored under in the cache is the same as imgRequest::mURI (the one that imgRequest::GetURI() returns). I get this failure on try: imgRequest::Init: mURI[file:///var/folders/qv/qvLcf24LGIi5vc43adPBU++++-k/-Tmp-/tmpLrkspP.mozrunner/thumbnails/e6920ad69fa373c80a75ecb113a42427.png] mCacheKey.Spec[moz-page-thumb://thumbnail?url=http%3A%2F%2Fmochi.test%3A8888%2Fbrowser%2Ftoolkit%2Fcomponents%2Fthumbnails%2Ftest%2Fbackground_red_scroll.html] Perhaps this is just a bug in the patch I'm working on, but I thought this was suggestive enough that I should post it here. If a similar mixup is happening without the patch, then we may be failing to load moz-page-thumb:// URLs from the cache, and possibly even failing to remove them when they're supposed to expire.
|
||
Comment 4•9 years ago
|
||
Help us, Seth, you're our only hope.
Whiteboard: [MemShrink] → [MemShrink:P1]
|
Reporter | |
Comment 6•9 years ago
|
||
On a recent nightly I still see multiple uncached thumbnails with multiple about:newtabs open. But after I navigate each one to real page and wait, the number of uncached images falls to a small number (though not zero). Perhaps some expiry is working better now? (Or perhaps I just hadn't noticed the improvement after a wait before?)
|
|
Reporter | |
Comment 7•9 years ago
|
||
My daily browser is still on 37.x, but I've just noticed something on it: There are a huge number of each uncached thumbnail... ├────662.85 MB (24.41%) -- images │ ├──636.23 MB (23.43%) -- uncached │ │ ├──635.92 MB (23.42%) -- raster/used │ │ │ ├──234.91 MB (08.65%) ++ (21 tiny) │ │ │ ├───52.30 MB (01.93%) -- image(moz-page-thumb://thumbnail?url=http%3A%2F%2Fwww.theguardian.com%2F) │ │ │ │ ├──52.30 MB (01.93%) ── source [239] │ │ │ │ └───0.00 MB (00.00%) ── decoded-heap [11] ... but when I expand the window objects 2,715.63 MB (100.0%) -- explicit ├──1,121.65 MB (41.30%) -- window-objects │ ├────881.41 MB (32.46%) ++ (701 tiny) I only see 38 entries for about:newtab (I say "only", but zero would be a much better number considering the new tab page isn't open anywhere). So where are all these thumbnails coming from? Here's a sample about:newtab, for reference: │ │ ├────1.41 MB (00.05%) -- top(about:newtab, id=4640) │ │ │ ├──1.30 MB (00.05%) -- active │ │ │ │ ├──1.20 MB (00.04%) -- window(about:newtab) │ │ │ │ │ ├──0.67 MB (00.02%) ++ js-compartment([System Principal], about:newtab) │ │ │ │ │ ├──0.41 MB (00.02%) ++ layout │ │ │ │ │ ├──0.11 MB (00.00%) ++ dom │ │ │ │ │ └──0.00 MB (00.00%) ── style-sheets │ │ │ │ └──0.10 MB (00.00%) -- window(about:blank) │ │ │ │ ├──0.09 MB (00.00%) ++ js-compartment(about:blank) │ │ │ │ ├──0.00 MB (00.00%) ++ dom │ │ │ │ └──0.00 MB (00.00%) ── style-sheets │ │ │ └──0.12 MB (00.00%) -- js-zone(0x7f6c7d5c8800) │ │ │ ├──0.04 MB (00.00%) ── unused-gc-things │ │ │ ├──0.03 MB (00.00%) ── type-objects/gc-heap │ │ │ ├──0.02 MB (00.00%) ── type-pool │ │ │ ├──0.01 MB (00.00%) ++ strings/string(<non-notable strings>) │ │ │ ├──0.01 MB (00.00%) ── gc-heap-arena-admin │ │ │ ├──0.01 MB (00.00%) ── baseline/optimized-stubs │ │ │ └──0.00 MB (00.00%) ++ sundries
|
|
Reporter | |
Comment 8•9 years ago
|
||
On a freshly restored firefox, before I've opend any new tabs I have: 1,825 (100.0%) -- js-main-runtime-compartments ├──1,047 (57.37%) -- system │ ├────655 (35.89%) ── [System Principal], inProcessTabChildGlobal?ownedBy=chrome://browser/content/browser.xul [655] │ ├────269 (14.74%) ++ (269 tiny) │ ├─────86 (04.71%) ── [System Principal], about:blank [86] │ └─────37 (02.03%) ── [System Principal], about:newtab [37] So this baseline of about:newtab entries must be coming from the session restore. Indeed: $ grep -r "about:newtab" -l . ./sessionstore.js ./sessionstore-backups/upgrade.js-20150321191402 ./sessionstore-backups/recovery.bak ./sessionstore-backups/recovery.js ./sessionstore-backups/upgrade.js-20141014105155 ./sessionstore-backups/upgrade.js-20150401164157 ./sessionstore-backups/upgrade.js-20141201111517 ./sessionstore-backups/previous.js ./sessionstore-backups/upgrade.js-20150220131016 ./sessionstore-backups/upgrade.js-20141113112924 ./sessionstore-backups/upgrade.js-20150407092154 With 38.0.1 + bug #1163866 the extra about:newtabs that appear when I open a new tab do, in fact, disappear some minutes after navigating to one of the suggested websites. I'm not yet sure what the story is with the uncached thumbnail count.
|
|
Reporter | |
Comment 9•9 years ago
|
||
Opening about:newtab leaks an imgRequest for every website with a tile until the about:newtab is closed or left by navigation. Opening a newtab then a website (e.g. by opening one of the tiles in another tab) and browsing around leaks several imgRequest for every tile. These don't go away as easily - closing the two tabs and then opening a new newtab seems to be required (but I haven't checked whether they're actually deleted or just lost track of). This seems to be what is happening: Opening the newtab page results in at least two imgLoader::LoadImage for each moz-page-thumb. The first time the imgRequest put in the cache. The second (or third) time it creates a new uncached imgRequest for the imgCacheValidator. Then in the imgCacheValidator::OnStartRequest callback it removes the old request from the cache and puts the new one in. Problem is the old imgRequest still has a reference somewhere and stays alive, uncached. I'm not sure why the cached image request needs validating in the first place. Seems like another bg to me, or is this expected? ShouldRevalidateEntry() returns true because the expiration time is 0 so the moz-page-thumb will always be popping in and out of the cache. Then the onStartRequest decides not to re-use the original imgRequest because cacheChan = null: does this look correct? The case when a newtab page is open while I browse around in another tab is just an accumulation of 'leaked' imgRequest whenever the imgCacheValidator::imgCacheValidator -> imgCacheValidator::OnStartRequest sequence happens. And this might have been hitting me harder than usual because I have several about:newtab in some tab's histories from my sessionstore.js, as noted in the comment above. Note: I think I've also seen evidence in my logs that chrome://browser/skin/newtab/controls.svg 'leaks' in a similar way (though I think the full story might be slightly different, anyway I haven't really investigated this).
|
|
Reporter | |
Comment 10•9 years ago
|
||
Addendum: When browsing with about:newtab open in another tab, ValidateEntry() return true at least half the time. This is either because: the context key match the loadid , i.e. ((request->LoadId() != key) && key). The rest of the time the validation routine checks the expiration time and fails it; ShouldRevalidateEntry return false because (VALIDATE_NEVER | VALIDATE_ONCE_PER_SESSION) are in aFlags.
|
|
||
Comment 11•9 years ago
|
||
That is some great investigation Oliver! Tagging needinfo to figure out what the next step is here based on your information.
Flags: needinfo?(tnikkel)
Flags: needinfo?(seth)
|
|
Reporter | |
Comment 12•9 years ago
|
||
The two image loads come from two different about:newpages documents. One extra newtab document is created for every visible new tab page by browser/base/content/tabbrowser.xml:_createPreloadBrowser(). This is controlled by browser.newtab.preload -> toggling it to false 'fixes' the leaks of uncached images when browsing around with a single about:newtab open. There are no leaked or uncached thumbnail images. All loads from the second on hit the cache. But with two about:newtabs there is at least one uncached images leaked per newtab pages. The second image load in each page leaks an uncached image but subsequent loads do indeed hit the cache. The uncached image is freed when the about:newtab is closed. With several about:newtabs open then I'm not sure how bounded the uncached image 'leaks' are - I've seen twice as many uncached thumbnails as newtab pages and the number freed when closing the tab/leaving the page varies. But still there are no uncached thumbnails left when there are no newtab pages left. I guess with browser.newtab.preload true then opening a new tab causes the second about:newtab to be created and stored as the preloaded tab and load the thumbnails again. So the image in the visible tab will be uncached (and last until the tab is closed). I don't think this explains why multiple uncached imaged are leaked when browsing around though - each newtab page should only be holding on to one css image for each site with a thumbnail, right?
|
|
Reporter | |
Comment 13•9 years ago
|
||
First, let's update the preparation steps from comment #0 then I'll describe my current understanding of the problem. * Create a new profile * Make sure browser.newtab.preload is still the default true * Browse around a little to create at least two frecent websites[1]. * Open the new tab page * Remove the pre-populated pages until the first of the frecent sites appear on the new tab pages * Close the tab * Close the browser [1] I created two frecent websites - the first (the guardian homepage) appears in the grid but I don't visit it while doing the STR. I log and use gdb to understand the lifecycle of the thumbnail for this site. The second (a web search for "fish") doesn't have to appear on the grid, I visit it to trigger the thumbnail refresh code. Now, open the browser and: 1. Open the newtab page => The newtab page is created and shown and a preloaded newtab page is created. => The thumbnail is loaded by the visible page and then by the preloaded page, which evicts the first from the cache via the mechanism outlined in comment #9. => Thus the visible page has CSS Image #1 (holding imgRequest A, which is uncached) and the preloaded page has CSS Image #2 (holding imgRequest B, which is in the image cache). 2. Wait a few seconds. (I think this is necessary for events to fire in the right order. If you don't wait, there's more ping-ponging of which document has an imgRequest in the image cache, but everything else is the same I think). Probably waiting a few seconds between each step is wise anyway. 3. Open a sponsored tile in a new tab then switch to it. => The preloaded newtab has its thumbnails refreshed but the visible tab doesn't (see browser/base/content/newtab/page.js:update() AIUI) => The refreshed thumbnail is CSS Image #3 (which re-uses imgRequest B since the cached entry has been validated for this context - see imgLoader::ValidateEntry()) -> But the document still holds a reference to CSS image #2 for some reason. 4. Go to the the second frecent page, (i.e. the "fish" search) => Now both newtab pages have their thumbnails refreshed. => The preloaded newtab add CSS Image #4 (which holds another reference to imgRequest B) -> The visible newtab adds CSS Image #5 (it does the imgCacheValidator dance from comment #9 and evicts imgRequest B from the cache, and creates imgRequest C) => Neither release their references to the old CSS Image So now we have: The visible about:newtab, with: CSS Image #5 (imgRequest C, cached), CSS Image #1 (imgRequest A) the preloaded about:newtab, with CSS Image #2/#3/#4 (imgRequest B) 5. Close the about:newtab page => CSS Image #1 is deleted by mRuleTree->Destroy() in nsStyleSet::Shutdown() [2] => imgRequest A follows it, as the css image held the last reference => CSS Image #5 is deleted by nsCycleCollector::FreeSnowWhite [3] => imgRequest C follows soon after, after imgCacheExpirationTracker::NotifyExpired() But the preloaded newtab page is still around. To get rid of it and all its images, you must. 6a. Open an about:newtab page => The preloaded newtab page becomes visible and a new preloaded newtab page is created => The new preloaded newtab page loads new thumbnails as before, and would evict imgRequests from the cache if they weren't already evicted. It isn't really important though. 6b. Close the newtab page => CSS Image #2 and #3 are deleted by mRuleTree->Destroy() in nsStyleSet::Shutdown() => CSS Image #4 is deleted by nsCycleCollector::FreeSnowWhite() => Finally, imgRequest B is deleted Footnotes to follow in next comment.
|
|
Reporter | |
Comment 14•9 years ago
|
||
Here's the relevant parts of the nsRefPtr logging tree for the imgRequestProxy stored with a nullptr key in mozilla/css/ImageLoader.cpp:ImageLoader::LoadImage()
[2] The StyleSet destructor
-1 #00: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/ipc/glue/MessagePump.cpp:127)
| -1 #00: NS_ProcessNextEvent(nsIThread*, bool) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/xpcom/glue/nsThreadUtils.cpp:265)
| -1 #00: nsThread::ProcessNextEvent(bool, bool*) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/xpcom/threads/nsThread.cpp:846 (discriminator 1))
| -1 #00: nsRunnableMethodImpl<void (mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(mozilla::TimeStamp), true, mozilla::TimeStamp>::Run() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/layout/base/../../dist/include/nsThreadUtils.h:828)
| -1 #00: mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/layout/base/../../../layout/base/nsRefreshDriver.cpp:372)
| -1 #00: mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/layout/base/../../../layout/base/nsRefreshDriver.cpp:179)
| -1 #00: nsRefreshDriver::Tick(long, mozilla::TimeStamp) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/layout/base/../../../layout/base/nsRefreshDriver.cpp:1636)
| -1 #00: nsTransitionManager::FlushTransitions(mozilla::css::CommonAnimationManager::FlushFlags) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/layout/style/nsTransitionManager.cpp:930)
| -1 #00: mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/dom/events/EventDispatcher.cpp:638)
| -1 #00: mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/dom/events/EventDispatcher.cpp:320)
| -1 #00: mozilla::EventListenerManager::HandleEvent(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/dom/events/../../dist/include/mozilla/EventListenerManager.h:330)
| -1 #00: mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/dom/events/EventListenerManager.cpp:1128)
| -1 #00: mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/dom/events/EventListenerManager.cpp:979)
| -1 #00: nsXBLEventHandler::HandleEvent(nsIDOMEvent*) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/dom/xbl/nsXBLEventHandler.cpp:53)
| -1 #00: nsCOMPtr<mozilla::JSEventHandler>::operator->() const (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/dom/xbl/../../dist/include/nsCOMPtr.h:705)
| -1 #00: Call<nsISupports*> (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/dom/events/../../dist/include/mozilla/dom/EventHandlerBinding.h:346)
| -1 #00: mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/dom/bindings/EventHandlerBinding.cpp:259)
| -1 #00: JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/js/src/jsapi.cpp:4419)
| -1 #00: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/js/src/vm/Interpreter.cpp:784)
| -1 #00: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/js/src/vm/Interpreter.cpp:747)
| -1 #00: js::RunScript(JSContext*, js::RunState&) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/js/src/vm/Interpreter.cpp:677)
| -1 #00: Interpret (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/js/src/vm/Interpreter.cpp:2956)
| -1 #00: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/js/src/vm/Interpreter.cpp:727)
| -1 #00: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/js/src/../../../js/src/jscntxtinlines.h:236)
| -1 #00: mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/dom/bindings/BindingUtils.cpp:2611)
| -1 #00: removeChild (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/dom/bindings/NodeBinding.cpp:734)
| -1 #00: nsINode::RemoveChild(nsINode&, mozilla::ErrorResult&) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/dom/base/nsINode.cpp:544)
| -1 #00: nsXULElement::RemoveChildAt(unsigned int, bool) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/dom/xul/nsXULElement.cpp:992)
| -1 #00: mozilla::dom::FragmentOrElement::RemoveChildAt(unsigned int, bool) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/dom/base/FragmentOrElement.cpp:1154)
| -1 #00: nsINode::doRemoveChildAt(unsigned int, bool, nsIContent*, nsAttrAndChildArray&) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/dom/base/nsINode.cpp:1657)
| -1 #00: mozAutoDocUpdate::~mozAutoDocUpdate() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/layout/style/../../../dom/base/mozAutoDocUpdate.h:40)
| -1 #00: mozilla::dom::XULDocument::EndUpdate(unsigned int) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/dom/xul/XULDocument.cpp:3210)
| -1 #00: nsDocument::EndUpdate(unsigned int) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/dom/base/nsDocument.cpp:4951 (discriminator 3))
| -1 #00: nsCOMPtr<nsIRunnable>::assign_with_AddRef(nsISupports*) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/dom/base/../../dist/include/nsCOMPtr.h:1038)
| -1 #00: nsHideViewer::Run() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/layout/generic/nsSubDocumentFrame.cpp:950)
| -1 #00: nsFrameLoader::Hide() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/dom/base/nsFrameLoader.cpp:858 (discriminator 1))
| -1 #00: nsDocShell::SetVisibility(bool) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/docshell/base/nsDocShell.cpp:6389 (discriminator 1))
| -1 #00: nsDocumentViewer::Hide() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/layout/base/nsDocumentViewer.cpp:2123)
| -1 #00: nsDocumentViewer::DestroyPresShell() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/layout/base/nsDocumentViewer.cpp:4446)
| 0 #00: PresShell::Destroy() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/layout/base/nsPresShell.cpp:1231)
| | 0 #00: nsDocument::DeleteShell() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/dom/base/nsDocument.cpp:3935)
| | 0 #00: nsBaseHashtable<nsPtrHashKey<imgIRequest>, unsigned int, unsigned int>::EnumerateRead(PLDHashOperator (*)(imgIRequest*, unsigned int, void*), void*) const (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/dom/base/../../dist/include/nsBaseHashtable.h:177)
| | 0 #00: PLDHashTable::Enumerate(PLDHashOperator (*)(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*), void*) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/xpcom/glue/pldhash.cpp:845)
| | 0 #00: nsBaseHashtable<nsPtrHashKey<imgIRequest>, unsigned int, unsigned int>::s_EnumReadStub(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/dom/base/../../dist/include/nsBaseHashtable.h:394)
| | 0 #00: RequestDiscardEnumerator(imgIRequest*, unsigned int, void*) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/dom/base/nsDocument.cpp:3920)
| | 0 #00: imgRequestProxy::RequestDiscard() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/image/../../image/imgRequestProxy.cpp:452)
| | 0 #00: mozilla::image::RasterImage::RequestDiscard() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/image/../../image/RasterImage.cpp:1944)
| | 0 #00: mozilla::image::ProgressTracker::OnDiscard() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/image/../../image/ProgressTracker.cpp:473 (discriminator 2))
| -1 #00: PresShell::Destroy() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/layout/base/nsPresShell.cpp:1281)
| -1 #00: nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::Length() const (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/layout/style/../../dist/include/nsTArray.h:361)
| -1 #00: nsRuleNode::DestroyInternal(nsRuleNode***) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/layout/style/nsRuleNode.cpp:1449 (discriminator 1))
| -1 #00: nsRuleNode::DestroyInternal(nsRuleNode***) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/layout/style/nsRuleNode.cpp:1461)
| -1 #00: mozilla::css::StyleRule::Release() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/layout/style/StyleRule.cpp:1393)
| | -1 #00: mozilla::css::StyleRule::~StyleRule() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/layout/style/StyleRule.cpp:1378)
| | -1 #00: operator delete (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/layout/style/../../dist/include/mozilla/mozalloc.h:210 (discriminator 1))
| | -1 #00: ~nsTArray_Impl (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/layout/style/../../dist/include/nsTArray.h:827)
| | -1 #00: nsAutoPtr<nsCSSCompressedDataBlock>::~nsAutoPtr() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/layout/style/../../dist/include/nsAutoPtr.h:75 (discriminator 1))
| | -1 #00: ~nsCSSValue (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/layout/style/nsCSSValue.h:414)
| | -1 #00: nsCSSValue::DoReset() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/layout/style/nsCSSValue.cpp:364)
| | -1 #00: operator delete (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/layout/style/../../dist/include/mozilla/mozalloc.h:210 (discriminator 2))
| | -1 #00: nsCSSValue::DoReset() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/layout/style/nsCSSValue.cpp:352)
| | -1 #00: operator delete (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/layout/style/../../dist/include/mozilla/mozalloc.h:210)
| | -1 #00: ~nsTHashtable (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/layout/style/../../dist/include/nsTHashtable.h:417)
| | 0 #00: PLDHashTable::Enumerate(PLDHashOperator (*)(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*), void*) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/xpcom/glue/pldhash.cpp:845)
| | | 0 #00: ClearRequestHashtable (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/layout/style/nsCSSValue.cpp:2464)
| | | 0 #00: imgRequestProxy::CancelAndForgetObserver(nsresult) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/image/../../image/imgRequestProxy.cpp:370)
| | | 0 #00: imgRequest::RemoveProxy(imgRequestProxy*, nsresult) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/image/../../image/imgRequest.cpp:249)
| | | 0 #00: mozilla::image::ProgressTracker::RemoveObserver(mozilla::image::IProgressObserver*) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/image/../../image/ProgressTracker.cpp:431)
| | | 1 #00: mozilla::image::ProgressTracker::EmulateRequestFinished(mozilla::image::IProgressObserver*) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/image/../../image/ProgressTracker.cpp:396)
| | | -1 #00: mozilla::image::ProgressTracker::EmulateRequestFinished(mozilla::image::IProgressObserver*) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/image/../../image/ProgressTracker.cpp:403)
| | -1 #00: PLDHashTable::Enumerate(PLDHashOperator (*)(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*), void*) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/xpcom/glue/pldhash.cpp:848)
| | -1 #00: PLDHashTable::RawRemove(PLDHashEntryHdr*) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/xpcom/glue/pldhash.cpp:795)
[3] The CycleCollector
-1 #00: mozilla::TimeStamp::Now() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/js/xpconnect/src/../../../dist/include/mozilla/TimeStamp.h:431)
| -1 #00: nsCycleCollector::FreeSnowWhite(bool) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/xpcom/base/nsCycleCollector.cpp:2799)
| -1 #00: SnowWhiteKiller::~SnowWhiteKiller() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/xpcom/base/nsCycleCollector.cpp:2639)
| -1 #00: mozilla::dom::HTMLSpanElement::~HTMLSpanElement() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/dom/html/HTMLSpanElement.cpp:22)
| -1 #00: mozilla::dom::FragmentOrElement::~FragmentOrElement() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/dom/base/FragmentOrElement.cpp:644)
| -1 #00: nsAttrAndChildArray::~nsAttrAndChildArray() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/dom/base/nsAttrAndChildArray.cpp:106)
| -1 #00: ~nsAttrName (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/dom/base/../../../dom/base/nsAttrName.h:53)
| -1 #00: nsAttrValue::Reset() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/dom/base/nsAttrValue.cpp:224)
| -1 #00: nsAttrValue::ClearMiscContainer() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/dom/base/nsAttrValue.cpp:1789)
| -1 #00: mozilla::css::StyleRule::Release() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/layout/style/StyleRule.cpp:1393)
| -1 #00: mozilla::css::StyleRule::~StyleRule() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/layout/style/StyleRule.cpp:1378)
| -1 #00: operator delete (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/layout/style/../../dist/include/mozilla/mozalloc.h:210 (discriminator 1))
| -1 #00: ~nsTArray_Impl (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/layout/style/../../dist/include/nsTArray.h:827)
| -1 #00: nsAutoPtr<nsCSSCompressedDataBlock>::~nsAutoPtr() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/layout/style/../../dist/include/nsAutoPtr.h:75 (discriminator 1))
| -1 #00: ~nsCSSValue (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/layout/style/nsCSSValue.h:414)
| -1 #00: nsCSSValue::DoReset() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/layout/style/nsCSSValue.cpp:364)
| -1 #00: operator delete (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/layout/style/../../dist/include/mozilla/mozalloc.h:210 (discriminator 2))
| -1 #00: nsCSSValue::DoReset() (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/layout/style/nsCSSValue.cpp:352)
| -1 #00: operator delete (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/layout/style/../../dist/include/mozilla/mozalloc.h:210)
| -1 #00: ~nsTHashtable (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/layout/style/../../dist/include/nsTHashtable.h:417)
| 0 #00: PLDHashTable::Enumerate(PLDHashOperator (*)(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*), void*) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/xpcom/glue/pldhash.cpp:845)
| | 0 #00: ClearRequestHashtable (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/layout/style/nsCSSValue.cpp:2464)
| | 0 #00: imgRequestProxy::CancelAndForgetObserver(nsresult) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/image/../../image/imgRequestProxy.cpp:370)
| | 0 #00: imgRequest::RemoveProxy(imgRequestProxy*, nsresult) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/image/../../image/imgRequest.cpp:249)
| | 0 #00: mozilla::image::ProgressTracker::RemoveObserver(mozilla::image::IProgressObserver*) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/image/../../image/ProgressTracker.cpp:431)
| | 1 #00: mozilla::image::ProgressTracker::EmulateRequestFinished(mozilla::image::IProgressObserver*) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/image/../../image/ProgressTracker.cpp:396)
| | -1 #00: mozilla::image::ProgressTracker::EmulateRequestFinished(mozilla::image::IProgressObserver*) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/obj-x86_64-unknown-linux-gnu-tests/image/../../image/ProgressTracker.cpp:403)
| -1 #00: PLDHashTable::Enumerate(PLDHashOperator (*)(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*), void*) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/xpcom/glue/pldhash.cpp:848)
| -1 #00: PLDHashTable::RawRemove(PLDHashEntryHdr*) (/home/oliver/projects/mozilla/workdir/thumb_cache_investigation/xpcom/glue/pldhash.cpp:795)|
|
||
Comment 15•9 years ago
|
||
When we refresh the thumbnails I would expect a stack like the FreeSnowWhite one to release the old thumbnail image. Is the HTMLSpanElement staying alive in the document (and keeping alive the style rule and hence the image)? Or does it go away, but the style rule sticks around for some reason?
|
|
||
Comment 16•9 years ago
|
||
(In reply to Oliver Henshaw from comment #9) > I'm not sure why the cached image request needs validating in the first > place. Seems like another bg to me, or is this expected? > ShouldRevalidateEntry() returns true because the expiration time is 0 so the > moz-page-thumb will always be popping in and out of the cache. Then the > onStartRequest decides not to re-use the original imgRequest because > cacheChan = null: does this look correct? This much is more-or-less as designed, though we could have designed it better. =) IIRC moz-page-thumb:// is a layer on top of file://, and thus it's not a caching channel. That leads us to conclude that we have no information about whether the channel contents will be different or not, and to reload the image from scratch. We could potentially check the mtime of the file - that's not totally sound, but it's probably good enough in practice. That sounds like something Necko probably already supports; I'll ask around. At any rate, the thing that's least clear to me here is: why are these pages holding on to old versions of the thumbnails, even after the thumbnails get refreshed? The stacks above suggest that it may be a cycle collection issue. Do they get freed if you click "CC" on about:memory? How about "GC"?
|
|
Reporter | |
Comment 17•9 years ago
|
||
(In reply to Timothy Nikkel (:tn) from comment #15) > When we refresh the thumbnails I would expect a stack like the FreeSnowWhite > one to release the old thumbnail image. Is the HTMLSpanElement staying alive > in the document (and keeping alive the style rule and hence the image)? Or > does it go away, but the style rule sticks around for some reason? The HTMLSpanElement isn't the problem, I think. The style rule sticks around after it's deleted. I've tracked the nsRefPtr lifetime of the StyleRule ultimately holding CSS Image #1, see attached files. (In reply to Seth Fowler [:seth] from comment #16) > At any rate, the thing that's least clear to me here is: why are these pages > holding on to old versions of the thumbnails, even after the thumbnails get > refreshed? The stacks above suggest that it may be a cycle collection issue. > Do they get freed if you click "CC" on about:memory? How about "GC"? No, 'CC' and 'GC' don't make any difference. Looks like the cycle collector cleans up the HTMLSpanElement but there's still a reference to the StyleRule that isn't released until the tab closes.
|
|
Reporter | |
Comment 18•9 years ago
|
||
|
|
Reporter | |
Comment 19•9 years ago
|
||
|
|
||
Comment 20•9 years ago
|
||
What happens if in the about:newtab code that creates the new thumbnails you set the old thumbnails to display:none _before_ the new thumbnails are created?
|
|
Reporter | |
Comment 21•9 years ago
|
||
Doesn't seem to make a difference. I did:
--- a/browser/base/content/newtab/grid.js
+++ b/browser/base/content/newtab/grid.js
@@ -111,6 +111,11 @@ let gGrid = {
* Renders the grid, including cells and sites.
*/
refresh() {
+ let old_thumbnails = this._node.querySelectorAll(".newtab-thumbnail");
+ for (let old_thumbnail of old_thumbnails) {
+ old_thumbnail.setAttribute("display", "none");
+ }
+
let cell = document.createElementNS(HTML_NAMESPACE, "div");
cell.classList.add("newtab-cell");
but nothing changes.|
|
||
Comment 22•9 years ago
|
||
I looked into this. It looks like the problem is that we aren't GCing style rule tree nodes. There looks to be two triggers for rule node gc: we end a reconstruct, or we have 300 unused rule nodes. When rule nodes can hold on to images 300 of them can be responsible for a large amount of memory. To fix this I'm guessing we should have a GC trigger heuristic based on the size in memory of the images the rule nodes might be holding on to. We should also respond to memory pressure notifications to do rule node GC.
|
|
||
Comment 23•9 years ago
|
||
Cameron, does comment 22 sound reasonable to you? Do you have any other thoughts?
Flags: needinfo?(cam)
|
||
Comment 24•9 years ago
|
||
GCing rule trees in response to memory pressure is a good idea. If we can get the heuristic right, GCing rule trees sounds like the right idea. We could check in nsRuleNode::Release whether we're now at mRefCnt == 0 and if we have an nsStyleBackground (and maybe some other structs?) cached on the rule node with > X MB image data, kick off a timer to GC rule trees soon.
Flags: needinfo?(cam)
|
|
Reporter | |
Comment 25•9 years ago
|
||
With bug #1180715 fixed, firefox can still 'leak' an imgRequest whenever the thumbnail file is updated and the newtab page is refreshed - i.e. at worst every thumbnail tile[1] every PageThumbs.jsm::MAX_THUMBNAIL_AGE_SECS (2 days). So users can still have a verrrry slow 'leak' with certain patterns of long-lived tabs. These lost images will still be counted as uncached so at least the cause will be relatively easy to spot. However, with my current fix for bug #1192394 the imgRequest for every old thumbnail image will remain in the cache. I don't know if anything in the image cache lifetime/expiration code will help here. So is there still a case for the approach discussed in comments #22-24? [1] min(browser.newtabpage.rows * browser.newtabpage.columns, NewTabUtils::LINKS_GET_LINKS_LIMIT) i.e. min (3*5, 100) by default.
|
|
||
Comment 26•9 years ago
|
||
(In reply to Oliver Henshaw from comment #25) > So is there still a case for the approach discussed in comments #22-24? Yes, because the general problem has nothing to do with thumbnails.
|
|
||
Comment 27•9 years ago
|
||
Flags: needinfo?(tnikkel)
Attachment #8668665 -
Flags: review?(cam)
|
|
||
Comment 28•9 years ago
|
||
What we call from AddRef/Release is getting bigger and more complicated. And inlining them seems to be important because they are implemented in nsStyleSet.h The calculation of image memory is very rough, mostly on purpose. I could probably add a function to the imgIContainer interface to get the actual memory use, but it didn't seem worth it. The amount of memory used by the image could change (discarding decoded surfaces, creating more decoded surfaces), this is why making sure the image pixels count stays above zero is needed when we subtract. This only handles backgrounds. The next most important ones to cover would probably be border and content (for before and after content). I have no idea what we should choose for the length of the timer, or how many image pixels should be the threshold. I didn't tune this at all, just choose something that seemed good.
Attachment #8668669 -
Flags: review?(cam)
|
|
||
Updated•9 years ago
|
Attachment #8668665 -
Flags: review?(cam) → review+
|
|
||
Comment 29•9 years ago
|
||
Comment on attachment 8668669 [details] [diff] [review] Part 2. Keep track of image memory referenced by unused rule nodes and use as gc heuristic. Review of attachment 8668669 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsRuleNode.cpp @@ +9808,5 @@ > +{ > + // Currently only looks at backgrounds. > + nsStyleBackground* bg = > + static_cast<nsStyleBackground*>(mStyleData.GetStyleData(eStyleStruct_Background)); > + if (!bg) { Two things here. First, I think we should not count Background struct pixels if this rule node doesn't own the Background struct (and is instead inheriting it from an ancestor rule node). So, only count it if !(mDependentBits & nsCachedStyleData::GetBitForSid(eStyleStruct_Background)). We can get into this situation with content like: <style> div { background: url(whatever.png); } /* rule 1 */ div { color: blue; } /* rule 2 */ </style> <div>...</div> The div's style context will point to the rule node for rule 2, and when we go to compute a Background struct on it, we'll find that no Background properties were specified on it until rule 1 (when traversing up the tree in WalkRuleTree), so we'll decide to store the owned Background struct on the rule node for rule 1, and store a dependent copy of the Background struct pointer on the rule node for rule 2. When these two rule nodes become unused, we should only count the pixels for that struct once. Second, we should also count Background structs that have been stored with conditions. The single-argument nsConditionalResetStyleData::GetStyleData, which nsCachedStyleData::GetStyleData calls, will return null if we've stored a reset struct with conditions. (That behaviour is confusing; I should try to remove the single-argument version.) We could end up in that situation if we have say: div { background: url(whatever.png) / 10em 10em; } where we'll store the Background struct with a font-size dependency. Have a look at nsConditionalResetStyleData to see how we sometimes store just a struct and sometimes an Entry (which is the head of a linked list of cached structs with conditions). @@ +9827,5 @@ > + if (NS_FAILED(imgReq->GetImage(getter_AddRefs(img))) || !img) { > + continue; > + } > + int32_t width, height; > + if (NS_FAILED(img->GetWidth(&width)) || NS_FAILED(img->GetHeight(&height))) { I guess we could start an image load and cause a rule node to be no longer referenced before the load knows how big the image is, and so return 0 from AssociatedImagePixels and not have a chance to update it. But being a heuristic I don't suppose it matters much. ::: layout/style/nsRuleNode.h @@ +1053,5 @@ > nscolor& aResult); > > static bool ParentHasPseudoElementData(nsStyleContext* aContext); > + > + size_t AssociatedImagePixels(); Can you add a comment here to describe how this returns an approximation of the number of pixels from images this rule node owns.
Attachment #8668669 -
Flags: review?(cam)
|
|
||
Comment 30•9 years ago
|
||
(In reply to Timothy Nikkel (:tn) from comment #28) > The calculation of image memory is very rough, mostly on purpose. I could > probably add a function to the imgIContainer interface to get the actual > memory use, but it didn't seem worth it. The amount of memory used by the > image could change (discarding decoded surfaces, creating more decoded > surfaces), this is why making sure the image pixels count stays above zero > is needed when we subtract. I actually don't think we should consider decoded size at all; that memory gets freed automatically by the ImageLib SurfaceCache in any case. (Well, we don't have visibility tracking for CSS images right now, so they'll never be freed because they stay locked, but we're about to fix that in bug 1218990.) In a post-bug 1218990 world, what matters is the source size. I think pixel count is a completely reasonable heuristic, though.
Flags: needinfo?(seth)
|
|
||
Comment 31•9 years ago
|
||
Tim, I just wanted to bring this back to your attention. If it's not too hard to get this in shape for landing we should probably go ahead and do it. (Though it'll matter less post-bug 1218990, so getting that landed will reduce the urgency a bit.)
Flags: needinfo?(tnikkel)
|
|
||
Comment 32•9 years ago
|
||
Yeah, this is still on my radar, just other things took priority.
|
||
Updated•7 years ago
|
Flags: needinfo?(tnikkel)
|
|
||
Comment 33•7 years ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #32) > Yeah, this is still on my radar, just other things took priority. Timothy, is this still worth doing? We have it as a P1 but maybe we should drop the priority?
Flags: needinfo?(tnikkel)
|
|
||
Comment 34•7 years ago
|
||
(In reply to Eric Rahm [:erahm] from comment #33) > (In reply to Timothy Nikkel (:tnikkel) from comment #32) > > Yeah, this is still on my radar, just other things took priority. > > Timothy, is this still worth doing? We have it as a P1 but maybe we should > drop the priority? Probably still worth doing, and probably should drop the priority. I'd say bug 1213245 is the most important memory related image bug now.
Flags: needinfo?(tnikkel)
|
|
||
Updated•7 years ago
|
Whiteboard: [MemShrink:P1] → [MemShrink:P2]
|
||
Comment 35•5 years ago
|
||
Hello!
This bug has been closed due to inactivity and/or the potential for this bug to no longer be an issue with the new Discovery Stream-powered New Tab experience.
Please help us triage by reopening if this issue still persists and should be addressed.
Thanks!
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•