Hey Ken -- in your version, do you see favicons?

-- and if you do, does the problem go away if you force off chrome://flags/#omnibox-ui-show-suggestion-favicons ?

Yes I do. I'll try turning the flag off. Thanks!

Oh yes. 100% that was it. Omnibox feels significantly more responsive with the flag off.

Ken - excellent, - thanks for reporting this problem. I will investigate to see if I can fix the slowness and get another version out for you to try.

I think I must have slow reflexes because I have not noticed the jank -- but I do want to make it fast.

Hey Ken - Can you tell me a bit about what kind of machine you are experiencing jank on?

Are you using a spinning disk machine? Hard Disk, RAM, CPU, OS would be helpful.

Also if you could let me know what version of Chrome precisely that would help as well, thanks.

This was a Z620 running current dev release (65.0.3298.3) on gLinux, so 64 GB RAM, 32 2.9GHz xeon cores. Chrome profile storage is definitely sitting on a spinning disk.

I should point out that no matter how hard I try now, I cannot repro the jank. I wonder if maybe there's some initially high overhead needed to warm up a cache of favicon/title data or something?

Hey Ken -- thanks for the info.

Hmm... well - I can say that the Favicons are stored inline within a SQLite database file.

But -- that database file is so small that I would expect that to be cached into RAM upon first access. One possibility could be that there was a lot of disk activity / contention when you got first got jank?

If you cannot repro the jank that could be the case... but I'll leave the bug open and still see if I can find any perf optimizations to make.

I've been doing some investigations here and there are three pieces of low hanging fruit for improvement on the current implementation:

 1. We make repeated requests to FaviconService (uncached sqlite) for the same page URL. Instead, we should track pending requests and patiently wait for the original to return.

 2. We make repeated requests to FaviconService for pages without favicons. We get a null result, and we don't cache null results currently for fear that the icon might become populated after the user and we miss out on the new one. We should cache the null results with a reasonable expiration policy.

 3. Our cache size is 12 entries at a memory size of ~37KB. That's probably too conservative. Given the cost of hitting the disk, we should probably go to 24 entries for a ~74KB max memory cost.. or even higher.

QQ: Is there any possibility that the operations which hit disk will block
the main thread? Presumably the work is all done in background tasks and
the UI thread should not ever block on omnibox updates, right?

Hey Ken - the favicons run on the HistoryBackend background task runner. In theory this should not block the main thread -- but I'll point out two caveats:

 1. The Autocomplete for the Omnibox dropdown also queries history, so I would guess that favicons are in competition with that.

 2. Likewise there's only one physical disk so there could hypothetically also be contention there -- though like I said above I would imagine a sane OS to cache the thumbnail DB file into memory.

FWIW, There is commentary on ThumbnailDatabase about moving it onto its own thread, but it's just speculation:

https://cs.chromium.org/chromium/src/components/history/core/browser/thumbnail_database.h?sq=package:chromium&l=33

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

commit a240bde71779c335b9addce02df86de75e8c02cd
Author: Tommy C. Li <tommycli@chromium.org>
Date: Fri Jan 05 00:54:33 2018

Omnibox UI Experiments: Refactor FaviconCache into its own class.

This CL has no behavior change. It refactors the favicon cache
mechanism into its own class in preparation for adding performance
optimizations and tests as described in the below bug.

Bug:  798490 
Change-Id: Ie1b7086083b6ea193476297f4b2a954b0fa8de53
Reviewed-on: https://chromium-review.googlesource.com/849412
Reviewed-by: Kevin Bailey <krb@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527176}
[modify] https://crrev.com/a240bde71779c335b9addce02df86de75e8c02cd/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/a240bde71779c335b9addce02df86de75e8c02cd/chrome/browser/ui/omnibox/chrome_omnibox_client.cc
[modify] https://crrev.com/a240bde71779c335b9addce02df86de75e8c02cd/chrome/browser/ui/omnibox/chrome_omnibox_client.h
[add] https://crrev.com/a240bde71779c335b9addce02df86de75e8c02cd/chrome/browser/ui/omnibox/favicon_cache.cc
[add] https://crrev.com/a240bde71779c335b9addce02df86de75e8c02cd/chrome/browser/ui/omnibox/favicon_cache.h
[modify] https://crrev.com/a240bde71779c335b9addce02df86de75e8c02cd/components/omnibox/browser/omnibox_client.cc
[modify] https://crrev.com/a240bde71779c335b9addce02df86de75e8c02cd/components/omnibox/browser/omnibox_client.h
[modify] https://crrev.com/a240bde71779c335b9addce02df86de75e8c02cd/components/omnibox/browser/omnibox_popup_model.cc
[modify] https://crrev.com/a240bde71779c335b9addce02df86de75e8c02cd/components/omnibox/browser/omnibox_popup_model.h

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

commit fc7c852c03345b030676d54ce766284e1d2d4118
Author: Tommy C. Li <tommycli@chromium.org>
Date: Mon Jan 08 18:22:04 2018

Omnibox UI Experiments: Add a basic unittest for FaviconCache.

Adding a unit test for the existing behavior. Lays the groundwork for
adding performance optimizations in a followup.

Bug:  798490 
Change-Id: Ic1734c9ca062d432143413e83c37676719a47394
Reviewed-on: https://chromium-review.googlesource.com/852824
Reviewed-by: Kevin Bailey <krb@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527675}
[modify] https://crrev.com/fc7c852c03345b030676d54ce766284e1d2d4118/chrome/browser/ui/omnibox/chrome_omnibox_client.cc
[modify] https://crrev.com/fc7c852c03345b030676d54ce766284e1d2d4118/chrome/browser/ui/omnibox/favicon_cache.cc
[modify] https://crrev.com/fc7c852c03345b030676d54ce766284e1d2d4118/chrome/browser/ui/omnibox/favicon_cache.h
[add] https://crrev.com/fc7c852c03345b030676d54ce766284e1d2d4118/chrome/browser/ui/omnibox/favicon_cache_unittest.cc
[modify] https://crrev.com/fc7c852c03345b030676d54ce766284e1d2d4118/chrome/test/BUILD.gn

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

commit 7bddca5ed13672ceac37c9ac315d8976fe2d364e
Author: Tommy C. Li <tommycli@chromium.org>
Date: Tue Jan 09 00:38:51 2018

Omnibox UI Experiments: For FaviconCache, coalesce multiple requests.

This CL prevents FaviconCache from hitting the underlying (uncached)
FaviconService multiple times for the same page URL. Instead it keep
track of its pending requests and add a callback to be called when
the original request (for the same page URL) returns.

Bug:  798490 
Change-Id: Ic8104c7ef5c48facec30270b3f006e70a0030901
Reviewed-on: https://chromium-review.googlesource.com/853213
Reviewed-by: Kevin Bailey <krb@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527831}
[modify] https://crrev.com/7bddca5ed13672ceac37c9ac315d8976fe2d364e/chrome/browser/ui/omnibox/favicon_cache.cc
[modify] https://crrev.com/7bddca5ed13672ceac37c9ac315d8976fe2d364e/chrome/browser/ui/omnibox/favicon_cache.h
[modify] https://crrev.com/7bddca5ed13672ceac37c9ac315d8976fe2d364e/chrome/browser/ui/omnibox/favicon_cache_unittest.cc

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

commit 52333cd288e457c8e0ecd0bd450d9bc28e64be01
Author: Tommy C. Li <tommycli@chromium.org>
Date: Thu Jan 11 02:12:18 2018

Omnibox UI Experiments: Add comment about multiple repaints.

Bug:  798490 
Change-Id: I47458f1d8830daa70947fa000bc24e8fbc96451f
Reviewed-on: https://chromium-review.googlesource.com/861048
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528534}
[modify] https://crrev.com/52333cd288e457c8e0ecd0bd450d9bc28e64be01/components/omnibox/browser/omnibox_popup_model.cc

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

commit 97ea2bbe8aee0877b49e069fda338b1a6d1a1a16
Author: Tommy C. Li <tommycli@chromium.org>
Date: Thu Jan 11 20:44:09 2018

Omnibox UI Experiments: Expire FaviconCache when history deleted.

Currently we keep cached favicons even when the user clears their
history. After this CL, we observe the HistoryService and appropriately
clear cached favicons.

It's not strictly speaking necessary -- but meets intuitive user
expectations for history clearing.

Bug: 799469,  798490 
Change-Id: I6705005e067f6c51864234b01682acb2cdc24777
Reviewed-on: https://chromium-review.googlesource.com/860534
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: Kevin Bailey <krb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528744}
[modify] https://crrev.com/97ea2bbe8aee0877b49e069fda338b1a6d1a1a16/chrome/browser/ui/omnibox/chrome_omnibox_client.cc
[modify] https://crrev.com/97ea2bbe8aee0877b49e069fda338b1a6d1a1a16/chrome/browser/ui/omnibox/favicon_cache.cc
[modify] https://crrev.com/97ea2bbe8aee0877b49e069fda338b1a6d1a1a16/chrome/browser/ui/omnibox/favicon_cache.h
[modify] https://crrev.com/97ea2bbe8aee0877b49e069fda338b1a6d1a1a16/chrome/browser/ui/omnibox/favicon_cache_unittest.cc

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

commit 44a90357e59104878d5637c7b399fd221c47849b
Author: Tommy C. Li <tommycli@chromium.org>
Date: Fri Jan 12 23:58:27 2018

Omnibox UI Experiments: Cache null favicon results

Previously null favicon results from FaviconService were not cached,
and therefore on each keystroke, we would run another SQL query.

This CL caches the null result and expires null results using a hint
from HistoryServiceObserver.

Bug:  798490 
Change-Id: I7a7f48329d6753f2699bc0c60e0f80c32fa186fa
Reviewed-on: https://chromium-review.googlesource.com/861087
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: Kevin Bailey <krb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529109}
[modify] https://crrev.com/44a90357e59104878d5637c7b399fd221c47849b/chrome/browser/ui/omnibox/favicon_cache.cc
[modify] https://crrev.com/44a90357e59104878d5637c7b399fd221c47849b/chrome/browser/ui/omnibox/favicon_cache.h
[modify] https://crrev.com/44a90357e59104878d5637c7b399fd221c47849b/chrome/browser/ui/omnibox/favicon_cache_unittest.cc

Okay we've put forth the due effort on performance here --

Please re-open if anyone sees any jank caused by switching that flag on.