Assigning to mastiz@ since I found recent CLs in components/favicon.
Not sure I understand fully, but it feels like favicon service triggers lot of memory allocations which decode / encode favicon bitmaps.

Thanks for bringing this up!

It's not obvious to me whether a 99th percentile of 8MB for low-end devices FaviconDatabaseSizeMB, being disk usage, is catastrophic.

On the other hand, metric NumFaviconBitmapsInDB (see https://goo.gl/qcghe2) shows a 99th percentile of ~533 bitmaps, which leads to an approximation of ~15 KB per favicon (assuming a common userbase). This is more than I though, but then we have popular pages like youtube which seems to serve a favicon of 96 KB.

There are at least two ways to improve the situation:

1. (Trivial to implement, with IMO undesired side effects) Adjust kLargestIconSize to a smaller value for low-end devices.

2. Add more smartness to choose which favicons get cached, similarly to how is done for desktop thumbnails,see  ThumbnailServiceImpl::ShouldAcquirePageThumbnail().

Both ideas require some work to avoid regressing on favicon quality, and unfortunately I cannot currently commit to that effort, but I'd be happy to review patches.

The entire browser process on low end devices allocate less than 5MB in native heap at median. Usually around 3MB. Considering this case, we would have 3MB from all other allocations and 8MB from favicon database (Ignoring the fact that history database will also be big in case where favicons are huge). So, 8MB is a huge problem on low end devices. I do not want to make it priority 1 due to resource constraints.

What would be the user experience if we disable the favicon cache on low end devices?

"allocate less than" already suggests memory usage. As I said earlier, FaviconDatabaseSizeMB is disk usage.

Wrt to comment 5: this would lead to multiple UIs, most notably the NTP or ChromeHome, showing gray fallback icons (referred to as scrabble tiles). These are broadly disliked, so disabling the favicon cache is IMO too drastic.

Sorry that I was being unclear. I was talking about RAM usage all this while. I was unaware that History.FaviconDatabaseSizeMB is disk usage. Is there anyway to get an estimate of RAM usage from UMA?
The local testing I am talking about was RAM usage of 20MB. Can you please give more context on when these are loaded into memory?
Thanks for the patience.

AFAIK, HistoryService/FaviconService/LargeIconService don't keep favicons in RAM so I cannot think of obvious places to profile. One suspect could be the caching layer for java, in https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/favicon/LargeIconBridge.java.

In your initial report, you claimed "it shows that the favicons alone account for 20MB". Could you please share details on how you concluded this?

The allocations I found were allocated by stack trace:
↳ƒ[Thread: oid.apps.chrome]	
↳ƒ <base.odex> + 0x3c537L	
↳ƒ Java_org_chromium_base_SystemMessageHandler_nativeDoRunLoopOnce	
↳ƒ base::MessageLoop::DoWork()	
↳ƒ base::MessageLoop::RunTask(base::PendingTask*)	
↳ƒ base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*)	
↳ƒ base::(anonymous namespace)::PostTaskAndReplyRelay::RunReplyAndSelfDestruct()	
↳ƒ base::internal::Invoker<base::internal::BindState<void (*)(base::CancellationFlag const*, base::OnceCallback<void ()>, base::OnceCallback<void ()>), base::internal::OwnedWrapper<base::CancellationFlag>, ↳ƒbase::(anonymous namespace)::RunIfNotCanceledThenUntrack(base::CancellationFlag const*, base::OnceCallback<void ()>, base::OnceCallback<void ()>)	
↳ƒ history::(anonymous namespace)::RunWithFaviconResults(base::RepeatingCallback<void (std::__ndk1::vector<favicon_base::FaviconRawBitmapResult, std::__ndk1::allocator<favicon_base::FaviconRawBitmapResult> 
↳ƒ void base::internal::Invoker<base::internal::BindState<void (favicon::FaviconServiceImpl::*)(base::RepeatingCallback<void (favicon_base::FaviconImageResult const&)> const&, int, 
↳ƒ void base::internal::InvokeHelper<false, void>::MakeItSo<void (offline_pages::GetOperationRequest::* const&)(std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> ↳ƒfavicon::FaviconServiceImpl::RunFaviconImageCallbackWithBitmapResults(base::RepeatingCallback<void (favicon_base::FaviconImageResult const&)> const&, int, 
↳ƒ favicon_base::SelectFaviconFramesFromPNGs(std::__ndk1::vector<favicon_base::FaviconRawBitmapResult, std::__ndk1::allocator<favicon_base::FaviconRawBitmapResult> > const&, std::__ndk1::vector<float, std::__ndk1::allocator<float> > const&, int)	
↳ƒ gfx::EncodeSkBitmap(SkBitmap const&, bool, std::__ndk1::vector<unsigned char, std::__ndk1::allocator<unsigned char> >*, int)	
↳ƒ gfx::EncodeSkPixmap(SkPixmap const&, std::__ndk1::vector<gfx::PNGCodec::Comment, std::__ndk1::allocator<gfx::PNGCodec::Comment> > const&, std::__ndk1::vector<unsigned char, 
↳ƒ SkPngEncoder::Encode(SkWStream*, SkPixmap const&, SkPngEncoder::Options const&)	
↳ƒ SkEncoder::encodeRows(int)	
↳ƒ SkPngEncoder::onEncodeRows(int)	
↳ƒ cr_png_write_rows	
↳ƒ cr_png_write_row

There's FAVICON_MAX_CACHE_SIZE_BYTES set 10 MB (of RAM), used by both BookmarkManager and HistoryManager. They can theoretically add up to the 20 MB you observe. However, skimming through LargeIconBridge, there it seems to decode the PNG data before caching, so it wouldn't explain the allocations reported in c#9.

There are other caches like the omnibox's FaviconCache but they are small.

Qq: is this a client with many bookmarks?

I ask this because there are not many clients on Android that use the favicon APIs with gfx::Image (FaviconService::GetFaviconImageForPageURL()), suggested by the allocations above, and BookmarksModel is one.

yeah the profile had a large number of bookmarks as well. The other stack trace that had large allocations was:

↳ƒ base::(anonymous namespace)::RunIfNotCanceledThenUntrack(base::CancellationFlag const*, base::OnceCallback<void ()>, base::OnceCallback<void ()>)
↳ƒ history::(anonymous namespace)::RunWithFaviconResults(base::RepeatingCallback<void (std::__ndk1::vector<favicon_base::FaviconRawBitmapResult, std::__ndk1::allocator<favicon_base::FaviconRawBitmapResult>
↳ƒ void base::internal::InvokeHelper<true, void>::MakeItSo<void (sync_sessions::FaviconCache::* const&)(GURL const&, std::__ndk1::vector<favicon_base::FaviconRawBitmapResult, 
↳ƒ sync_sessions::FaviconCache::OnFaviconDataAvailable(GURL const&, std::__ndk1::vector<favicon_base::FaviconRawBitmapResult, std::__ndk1::allocator<favicon_base::FaviconRawBitmapResult> > const&)
↳ƒ std::__ndk1::map<GURL, GURL, std::__ndk1::less<GURL>, std::__ndk1::allocator<std::__ndk1::pair<GURL const, GURL> > >::operator[](GURL const&)

I have posted a CL which would remove some unnecessary image resizing done in FaviconServiceImpl::RunFaviconImageCallbackWithBitmapResults() https://chromium-review.googlesource.com/c/chromium/src/+/865667

The stack traces that you have posted are not very helpful. To determine what can be improved we need to know who calls FaviconService a lot of times.

In general, I would expect calls to FaviconService on each page navigation
 Are we calling FaviconService too many times per page navigation?
Are we calling FaviconService for frivolous things like fragment navigations? (probably yes)
Is something else like bookmark sync calling FaviconService a lot? (It used to be the case that sync would load the favicons for all bookmarks into memory at Chrome launch. I don't think that this is the case anymore but I can no longer test anything sync related due to  crbug.com/677887 )

favicon_cache.cc is the cache for "Recently Closed" for tab sync. (This is a different cache than the one mentioned by mastiz@) The amount of favicons which are kept in memory is controlled by kMaxSyncFavicons in sessions_sync_manager.cc

I can probably give you more insight once we know who is calling FaviconService a lot. As much as I would like to I am not allowed to spend a week or month to investigate things because anything favicon related for me falls into 20% work.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f5d5e2e779a0cc6d6e128b5915b5facbbace201c

commit f5d5e2e779a0cc6d6e128b5915b5facbbace201c
Author: Siddhartha <ssid@chromium.org>
Date: Wed Jan 24 04:15:00 2018

Do not store favicon for all tabs in MediaSessionTabHelper

When the user opens lot of tabs in a session, too many favicons are
stored for media notifications. Do not store favicon unless media is
playing. When media status changes, obtain the largest favicon from
large icon bridge.

BUG= 799651 

Change-Id: Ib9ba8d0827dbe3d6df23e207648ba52b6d811c0e
Reviewed-on: https://chromium-review.googlesource.com/879093
Commit-Queue: Siddhartha S <ssid@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531430}
[modify] https://crrev.com/f5d5e2e779a0cc6d6e128b5915b5facbbace201c/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a0c2a568819d862f5706a0bb07d96f9b153100f3

commit a0c2a568819d862f5706a0bb07d96f9b153100f3
Author: Siddhartha <ssid@chromium.org>
Date: Wed Jan 31 22:57:48 2018

Add unittests for MediaSesionTabHelper with fake LargeIconBridge

Add tests and address nits for changes in crrev.com/879093

BUG= 799651 

Change-Id: Iefc7c6590a5fd74534e195583aa0a20e0080a983
Reviewed-on: https://chromium-review.googlesource.com/884507
Commit-Queue: Siddhartha S <ssid@chromium.org>
Reviewed-by: Mounir Lamouri (slow) <mlamouri@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533459}
[modify] https://crrev.com/a0c2a568819d862f5706a0bb07d96f9b153100f3/chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaSessionTabHelper.java
[modify] https://crrev.com/a0c2a568819d862f5706a0bb07d96f9b153100f3/chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationFaviconTest.java
[modify] https://crrev.com/a0c2a568819d862f5706a0bb07d96f9b153100f3/chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationTestTabHolder.java

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/02b92af9daf72ae77321f99bc69d7f3f8bbf02b0

commit 02b92af9daf72ae77321f99bc69d7f3f8bbf02b0
Author: Peter Kotwicz <pkotwicz@chromium.org>
Date: Wed Feb 21 19:03:01 2018

[Android] Don't generate unused blurry favicon bitmaps

This CL changes ui::GetSupportedScaleFactors() to return just 1x on Android. This affects
FaviconService::GetFaviconForPageURL(). (This affects other FaviconService methods but I believe
that the methods are unused on Android). Previously, GetFaviconForPageURL() would return a
gfx::ImageSkia with multiple densities. The CL changes the return value of GetFaviconForPageURL()
to return a gfx::ImageSkia with just the 1x density.

The SkBitmap for the larger density in the gfx::ImageSkia returned by GetFaviconForPageURL() was a
crappily resized version of the SkBitmap for the smaller density (resized via
SelectFaviconFramesFromPNGs()) because on Android we only store a single SkBitmap per page URL
(per icon type).

On Android, we store two favicons in the database per page URL:
- Largest icon of type kFavicon
- Largest icon of type kTouchIcon/kTouchPrecomposedIcon/kWebManifestIcon.
We don't store favicons in the database for multiple scale factors the way we do on
Mac/Windows/CrOS.

GetFaviconForPageURL() has two callers on Android:
BookmarkModel::LoadFavicon()
FaviconCache::OnPageFaviconUpdated()

The SkBitmap for the larger image density was unused on Android:
bookmark_model.cc -> Android uses LargeIconService to request bookmark icons. I
  believe that BookmarkModel::GetFavicon() is only used by sync on Android.
favicon_cache.cc -> Note GetIconSizeBinFromBitmapResult()

This CL should make Chrome slightly faster by removing unnecessary image resizing.

Support for non-1x scale factors on Android was originally added in
https://chromiumcodereview.appspot.com/11886074

BUG= 799651 

Change-Id: Ie99fac8b4850976ef92101383c475d15a775f35a
Reviewed-on: https://chromium-review.googlesource.com/865667
Commit-Queue: Peter Kotwicz <pkotwicz@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538174}
[modify] https://crrev.com/02b92af9daf72ae77321f99bc69d7f3f8bbf02b0/ui/android/java/src/org/chromium/ui/base/ResourceBundle.java
[modify] https://crrev.com/02b92af9daf72ae77321f99bc69d7f3f8bbf02b0/ui/base/resource/resource_bundle.cc
[modify] https://crrev.com/02b92af9daf72ae77321f99bc69d7f3f8bbf02b0/ui/base/resource/resource_bundle_android.cc

Is there anything more to be done here?

This bug is not on my radar right now

pkotwicz@
> This bug is not on my radar right now
Apologies if I was unclear.  I was asking if this bug is fixed / can be closed.

There seems to be no other places where favicons are stored in memory. I am closing this for now. Will file another bug if anything was seen.