Patch: https://chromium-review.googlesource.com/c/chromium/src/+/876084

dyaroshev@, the CL itself (crrev.com/c/876084) is quite large, and we'd like to get some justification for its complexity.

Why are you adding this estimation at all? Did you see that autocomplete data is nontrivial in size? Can you share numbers?

I believe there are different issues here because of large history database.
The omnibox HQP indexing (url index private data) will be huge if there are lots of urls in history. We limit this by MaxNumHQPUrlsIndexedAtStartup().
The might be large number of favicons in history database. Just instrumenting the favicon service should give this information?
Why are you trying to instrument the history database. Isn't it enough to guess the memory usage using metrics like the number of urls in the history database?

Favicons aren't loaded into memory, so at a certain point I wouldn't expect the size of the favicon db (thumbnails) to impact memory size.

Thinking more, it might be a good idea to understand the memory usage of history service in detail. There are quite a few reports from users with large usage on history service. Maybe we could find ways to attribute memory in a simpler way which is approximate?

Understanding memory usage of history SGTM. I think that entails digging into sqlite. I'm not sure who could take that on.

Um what do you mean just sqlite?
From most of the reports I have seen, the sqlite memory usage is in order of KBs and the memory usage of history service tasks (Process db tasks?) are in order of 10MBs. I have observed it both in local debugging as well as reports from real users.
I was fairly confident that we are attributing memory usage of sqlite databases very completely. But, we might have to investigate that part.

I am surprised by that. I would not expect db tasks to be in the tens of MBs. Can you be more specific as to what tasks are that big?

So, this is a report from Android where history_service tasks allocate about 200MB. All of this is allocated by HistoryBackend::ProcessDBTask(). Unfortunately this is all the information I have currently from Android reports.
https://crash.corp.google.com/browse?stbtiq=b172a86a945bfdf7#6

I am still trying to find similar traces in desktop which might have stack traces, but couldn't find any. But, I suspect the usage is due to either Omnibox hqp indexing or favicon cache

200MB seems so excessive that I have to wonder if it's reporting the right thing?

Are you sure that's the right crash? I don't see the history thread in it. Maybe I'm reading it wrong?

Sorry let me explain in detail.
We collect memory heap dumps from users who have high memory usage. On other platforms it was feasible to obtain native stack traces, but on Android we cannot unwind stack traces. So, all we have in the heap dumps are trace events and tasks locations. So, we have memory allocated by tasks posted from history_service.cc.
For reading the heap dumps instructions are here:
https://chromium.googlesource.com/chromium/src/+/5379c83dad6d8f3cbd9752a1e878dc00710edad8/docs/memory-infra/heap_profiler.md

In short, clicking on malloc section of browser process would show up the stack frames and Object types. If you look at the stack frames, you'll see HistoryThread allocating 235MB. and on type names it shows the file which posted the task, which is history_service.cc. With some extra debug information I found that the function that posted tasks was HistoryService::ScheduleDBTask.

For example, the task here: https://cs.chromium.org/chromium/src/components/omnibox/browser/url_index_private_data.cc?type=cs&l=332
would update the whole url index from history database. The url indexing consumes a lot of memory since it stores the url in various map structures. This runs on history thread too.

Hi!

More on what I've seen:
- UrlIndexPrivateData is big - a few mbs for a small profile.
- HistoryService is negligible but I don't measure some pieces.
- HistoryBackend itself is accountable for a few dozens kilobytes, however I didn't get  into measuring tasks. I'll update patch to do this.
- Autocomplete History parts are big too, due to the fact that AutcompleteInput/Result/Match are significant.

The main goal of this patch was to get some initial history memory estimating infrastructure in place and improve memory tracing in general.

Best,
Denis.

ssid, if you look at what https://cs.chromium.org/chromium/src/components/omnibox/browser/url_index_private_data.cc?type=cs&l=332 runs on the db thread, it's a pretty simple select statement: https://cs.chromium.org/chromium/src/components/history/core/browser/visit_database.cc?type=cs&l=416 (max_results is 10). So, I wouldn't expect that to allocate 200mb. Seems like something is very wrong if that statement in fact triggers allocated 200mb.

drive-by, only possibly relevant: We have seen cases where circular redirect chains end up spamming the history database.  See bug 750845.

Before adding a large change to estimate memory, I'd like some idea of what's wrong with the current memory estimates we have in UMA.  For example, there are the UMA metrics of size on disk: History.DatabaseFileMB, History.URLTableCount, History.VisitTableCount, History.FaviconDatabaseSizeMB, and History.NumFaviconsInDB.  I assume these are heavily correlated with maximum amount of memory they can possibly take up.

And for in-memory structures, there are History.InMemoryDBItemCount, History.InMemoryURLCacheSize, and History.InMemoryURLHistoryItems.  All of these should be directly correlated with the actual amount of memory these structures take up.

Why do these metrics not suffice?

Re #17:
Sorry I meant to give example of In-memory url index:
https://cs.chromium.org/chromium/src/components/omnibox/browser/in_memory_url_index.cc?rcl=4edc316d30c5e4c17ebec4206ade62dee37a7571&l=61
IIUC, it could rebuild the whole index for lot of history urls allocating a lot of memory. I was just giving example that some other component could allocate on history tasks. (I believe it doesn't use all urls on Android).

Re #19:
History.DatabaseFileMB, History.FaviconDatabaseSizeMB, and History.NumFaviconsInDB is a disk size and we don't really have an estimate of ram usage. It does correlate, but like the case with https://bugs.chromium.org/p/chromium/issues/detail?id=799651#c12 we are not sure which component calls favicon service too many times and why the favicons get loaded in memory.

I agree History.InMemoryDBItemCount, History.InMemoryURLCacheSize, and History.InMemoryURLHistoryItems give an estimate of memory usage. It is better to have the estimations at one place and overall idea of memory usage than in parts.

There are few advantages of having the estimation integrated in memory-infra like this data is directly fed into UKM, UMA. It can be coorelated to process overall memory usage, to answer questions like how many users have high history usage among the users with high browser memory.
I am not saying we should go ahead with a large intrusive change. Maybe we could start with adding these existing memory estimates to memory-infra and extend to make it better?

dyaroshev@, How about starting with just UrlIndexPrivateData and Autocomplete History and concentrate on the ones we know are big. I understand it wouldn't be complete, but it gives a good estimate already. We always have a trade off between accuracy and code complexity while estimating memory usage. Adding EstimateMemoryUsage in too many classes gets hard to maintain.

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

commit 2b611de9689186c38c832533c2c4070eb9e3104e
Author: Siddhartha <ssid@chromium.org>
Date: Thu Jan 25 03:55:50 2018

Get location from code that schedules history db tasks

The histroy db tasks are run using the location of history service. But
in reality they are started by various other components. Using the
location from the original location is easier for tracing.

BUG= 803822 

Change-Id: I97bc4aa03aa202903dd8346c39d3b284be078a6c
Reviewed-on: https://chromium-review.googlesource.com/884725
Commit-Queue: Siddhartha S <ssid@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531807}
[modify] https://crrev.com/2b611de9689186c38c832533c2c4070eb9e3104e/chrome/browser/android/history_report/data_provider.cc
[modify] https://crrev.com/2b611de9689186c38c832533c2c4070eb9e3104e/chrome/browser/history/android/android_history_provider_service.cc
[modify] https://crrev.com/2b611de9689186c38c832533c2c4070eb9e3104e/chrome/browser/history/history_test_utils.cc
[modify] https://crrev.com/2b611de9689186c38c832533c2c4070eb9e3104e/chrome/browser/predictors/resource_prefetch_predictor.cc
[modify] https://crrev.com/2b611de9689186c38c832533c2c4070eb9e3104e/chrome/browser/profiles/profile_window_browsertest.cc
[modify] https://crrev.com/2b611de9689186c38c832533c2c4070eb9e3104e/chrome/browser/sync/test/integration/bookmarks_helper.cc
[modify] https://crrev.com/2b611de9689186c38c832533c2c4070eb9e3104e/chrome/browser/sync/test/integration/typed_urls_helper.cc
[modify] https://crrev.com/2b611de9689186c38c832533c2c4070eb9e3104e/components/browser_sync/profile_sync_service_typed_url_unittest.cc
[modify] https://crrev.com/2b611de9689186c38c832533c2c4070eb9e3104e/components/browser_sync/signin_confirmation_helper.cc
[modify] https://crrev.com/2b611de9689186c38c832533c2c4070eb9e3104e/components/history/content/browser/content_visit_delegate.cc
[modify] https://crrev.com/2b611de9689186c38c832533c2c4070eb9e3104e/components/history/core/browser/delete_directive_handler.cc
[modify] https://crrev.com/2b611de9689186c38c832533c2c4070eb9e3104e/components/history/core/browser/history_model_worker.cc
[modify] https://crrev.com/2b611de9689186c38c832533c2c4070eb9e3104e/components/history/core/browser/history_model_worker_unittest.cc
[modify] https://crrev.com/2b611de9689186c38c832533c2c4070eb9e3104e/components/history/core/browser/history_service.cc
[modify] https://crrev.com/2b611de9689186c38c832533c2c4070eb9e3104e/components/history/core/browser/history_service.h
[modify] https://crrev.com/2b611de9689186c38c832533c2c4070eb9e3104e/components/history/core/browser/history_service_unittest.cc
[modify] https://crrev.com/2b611de9689186c38c832533c2c4070eb9e3104e/components/history/core/browser/typed_url_data_type_controller.cc
[modify] https://crrev.com/2b611de9689186c38c832533c2c4070eb9e3104e/components/history/core/browser/typed_url_model_type_controller.cc
[modify] https://crrev.com/2b611de9689186c38c832533c2c4070eb9e3104e/components/omnibox/browser/history_quick_provider_unittest.cc
[modify] https://crrev.com/2b611de9689186c38c832533c2c4070eb9e3104e/components/omnibox/browser/in_memory_url_index.cc
[modify] https://crrev.com/2b611de9689186c38c832533c2c4070eb9e3104e/components/omnibox/browser/url_index_private_data.cc
[modify] https://crrev.com/2b611de9689186c38c832533c2c4070eb9e3104e/components/sync_sessions/revisit/typed_url_page_revisit_observer.cc

ssid@

Ok, If you say so.
There are just things like recent_redirects_ and queued_history_db_tasks_ that I am worried about in history_backend.

But I get your point about complexity. Will do.

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

commit 89dd2bf1c4794fb781b114811e866b443da380f8
Author: Denis Yaroshevskiy <dyaroshev@yandex-team.ru>
Date: Mon Jan 29 22:04:27 2018

Memory tracing for history. InMemoryUrlIndex.

Bug:  803822 
Change-Id: I65b3eea6ebd61fff719650d8af5364d429b73103
Reviewed-on: https://chromium-review.googlesource.com/888701
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Siddhartha S <ssid@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532615}
[modify] https://crrev.com/89dd2bf1c4794fb781b114811e866b443da380f8/chrome/browser/autocomplete/autocomplete_browsertest.cc
[modify] https://crrev.com/89dd2bf1c4794fb781b114811e866b443da380f8/components/history/core/browser/url_row.cc
[modify] https://crrev.com/89dd2bf1c4794fb781b114811e866b443da380f8/components/history/core/browser/url_row.h
[modify] https://crrev.com/89dd2bf1c4794fb781b114811e866b443da380f8/components/omnibox/browser/in_memory_url_index.cc
[modify] https://crrev.com/89dd2bf1c4794fb781b114811e866b443da380f8/components/omnibox/browser/in_memory_url_index.h
[modify] https://crrev.com/89dd2bf1c4794fb781b114811e866b443da380f8/components/omnibox/browser/in_memory_url_index_types.cc
[modify] https://crrev.com/89dd2bf1c4794fb781b114811e866b443da380f8/components/omnibox/browser/in_memory_url_index_types.h
[modify] https://crrev.com/89dd2bf1c4794fb781b114811e866b443da380f8/components/omnibox/browser/url_index_private_data.cc
[modify] https://crrev.com/89dd2bf1c4794fb781b114811e866b443da380f8/components/omnibox/browser/url_index_private_data.h

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

commit 22e397c6daa690c7384dd6be7b859bd9585040a4
Author: Siddhartha <ssid@chromium.org>
Date: Thu Feb 01 05:37:31 2018

Memory-infra: whitelist Omnibox urlindex MDP

BUG= 803822 

Change-Id: I6d635e220a5e7a1bf0001426ac4e6c643b90b8fd
Reviewed-on: https://chromium-review.googlesource.com/892466
Reviewed-by: Dmitry Skiba <dskiba@chromium.org>
Commit-Queue: Siddhartha S <ssid@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533573}
[modify] https://crrev.com/22e397c6daa690c7384dd6be7b859bd9585040a4/base/trace_event/memory_infra_background_whitelist.cc

dyaroshev is currently making at least one more CL here: https://chromium-review.googlesource.com/893361

I can't make them Owner without a chromium.org address, apparently. Since I'm reviewing that change, I'll put myself as Owner for now.

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

commit 361a59325a9772a272b313ebf591ec679cb3feda
Author: Denis Yaroshevskiy <dyaroshev@yandex-team.ru>
Date: Mon Feb 12 20:16:12 2018

Memory tracing in parts of autocomplete controller related to history.

Bug:  803822 
Change-Id: I5410a2204e2c613468c45c03f75d2a17e7804a3d
Reviewed-on: https://chromium-review.googlesource.com/893361
Commit-Queue: Denis Yaroshevskiy <dyaroshev@yandex-team.ru>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536170}
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/chrome/browser/autocomplete/autocomplete_browsertest.cc
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/chrome/browser/search_engines/ui_thread_search_terms_data.cc
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/chrome/browser/search_engines/ui_thread_search_terms_data.h
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/omnibox/browser/autocomplete_controller.cc
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/omnibox/browser/autocomplete_controller.h
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/omnibox/browser/autocomplete_input.cc
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/omnibox/browser/autocomplete_input.h
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/omnibox/browser/autocomplete_match.cc
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/omnibox/browser/autocomplete_match.h
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/omnibox/browser/autocomplete_provider.cc
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/omnibox/browser/autocomplete_provider.h
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/omnibox/browser/autocomplete_result.cc
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/omnibox/browser/autocomplete_result.h
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/omnibox/browser/history_match.cc
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/omnibox/browser/history_match.h
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/omnibox/browser/history_quick_provider.cc
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/omnibox/browser/history_quick_provider.h
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/omnibox/browser/history_url_provider.cc
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/omnibox/browser/history_url_provider.h
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/omnibox/browser/omnibox_edit_model_unittest.cc
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/omnibox/browser/omnibox_field_trial.cc
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/omnibox/browser/omnibox_field_trial.h
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/omnibox/browser/omnibox_popup_model_unittest.cc
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/omnibox/browser/suggestion_answer.cc
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/omnibox/browser/suggestion_answer.h
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/search_engines/search_terms_data.cc
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/search_engines/search_terms_data.h
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/search_engines/template_url.cc
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/search_engines/template_url.h
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/search_engines/template_url_data.cc
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/search_engines/template_url_data.h
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/search_engines/testing_search_terms_data.cc
[modify] https://crrev.com/361a59325a9772a272b313ebf591ec679cb3feda/components/search_engines/testing_search_terms_data.h

dyaroshev: Did you have any more instrumentation in mind or can I mark this fixed?

Or did you want to keep it open as a place to comment on the analysis once your changes roll out widely? If so, can you update the title/description of this bug to indicate that the objective is to analyze memory usage, not just implement the estimation?

@jdonnelly

I don't see any more useful instrumentation in history at this point. I suggest we close the bug.

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

commit 25ed841563c61ad9fbeafc9d74763e80e2ed70af
Author: Siddhartha <ssid@chromium.org>
Date: Wed Mar 14 06:08:54 2018

MemoryInfra: Fix AutoComplete provider and enable on background mode

There could be multiple providers. So, each must create allocator dumps
with distinct names. Refer notes on GetOrCreateAllocatorDump.
Group autocomplete and in_memory_url_index dumps under different child
dumps for easier collection of metrics.

BUG= 803822 
TBR=mpearson@chromium.org

Change-Id: I872491bb1496572600fc6b6f87d9198a76eea8c7
Reviewed-on: https://chromium-review.googlesource.com/961263
Commit-Queue: Siddhartha S <ssid@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543017}
[modify] https://crrev.com/25ed841563c61ad9fbeafc9d74763e80e2ed70af/base/trace_event/memory_infra_background_whitelist.cc
[modify] https://crrev.com/25ed841563c61ad9fbeafc9d74763e80e2ed70af/chrome/browser/autocomplete/autocomplete_browsertest.cc
[modify] https://crrev.com/25ed841563c61ad9fbeafc9d74763e80e2ed70af/components/omnibox/browser/autocomplete_controller.cc
[modify] https://crrev.com/25ed841563c61ad9fbeafc9d74763e80e2ed70af/components/omnibox/browser/in_memory_url_index.cc