New issue
Advanced search Search tips

Issue 693225 link

Starred by 2 users
Status: Archived
Owner: ----
Closed: Jun 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Switching to flat sets in HQP.

Reported by dyaros...@yandex-team.ru, Feb 16 2017

Comment 1 Deleted

Comment 2 Deleted

Comment 3 Deleted

Comment 12 by dyaros...@yandex-team.ru, Mar 1 2017 

Switching to flat_sets: https://codereview.chromium.org/2719403002/
Project Member

Comment 13 by bugdroid1@chromium.org, Mar 3 2017 

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

commit 246ebe8f2a857270c6a256493e2244babf257c00
Author: dyaroshev <dyaroshev@yandex-team.ru>
Date: Fri Mar 03 11:18:26 2017

Switching to flat_sets in HQP.
Also implemented a few cleanups suggested in the previous cl:
https://codereview.chromium.org/2690303012/

BUG= 693225 

Review-Url: https://codereview.chromium.org/2719403002
Cr-Commit-Position: refs/heads/master@{#454566}

[modify] https://crrev.com/246ebe8f2a857270c6a256493e2244babf257c00/components/omnibox/browser/in_memory_url_index_types.h
[modify] https://crrev.com/246ebe8f2a857270c6a256493e2244babf257c00/components/omnibox/browser/url_index_private_data.cc

Comment 14 by a-...@yandex-team.ru, Mar 12 2017 

@pkasting, mpearson
Please write about impact of this change on omnibox histograms values.

Comment 15 by mpear...@chromium.org, May 4 2017

Components: UI>Browser>Omnibox
Labels: Needs-Feedback
Replied via e-mail last month.

Is there anything left to do here?  I recall you saying you didn't think there was much to gain from flat_maps in terms of performance in this area.  Are there any other spots that we haven't yet converted to flat_sets in HQP that should be done?

Comment 16 by pkasting@chromium.org, May 4 2017 

FWIW, I got an email recently noting that the existing maps were a major consumer of memory in the browser process.  If we can convert any to flat maps and save memory, even at no performance gain, it would be worth doing (if there's not a significant performance loss).

Comment 17 by a-...@yandex-team.ru, May 4 2017 

Mark, sorry, did not get your email. You wrote to Denis maybe, dyaroshev@yandex-team.ru? 
If its not confident info, can you please forward to a-v-y@yandex-team.ru?
Project Member

Comment 18 by sheriffbot@chromium.org, Jun 5 2017

Status: Archived (was: Unconfirmed)
No feedback was received in the last 30 days from reporter "dyaroshev@yandex-team.ru", so archiving this. Please re-open or file a new bug if this is still an issue.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 19 by mpear...@chromium.org, Aug 14 2017 

a-v-y@yandex-team.ru:
> Please write about impact of this change on omnibox histograms values.

Sorry for the delayed answer.  Here's the notes I wrote at the time:

From the canary channel, however, it's easy to see when the change landed.
- the 95th percentile of Omnibox.HistoryQuickHistoryIDSetFromWords drops by about 70%.  (Equivalent in magnitude to the whole performance regression bug 696038 introduced in February!)  Wowza!
- the 95th percentile of Omnibox.ProviderTime2.HistoryQuick drops by 25%.
- the 95th percentile of Omnibox.QueryTime2.1 drops by 25% as well.
- the 95th percentile of Omnibox.CharTypedToRepaintLatency drops by about 15%.
These last three things are not quite recovering the whole performance regression, but recover at least the majority of it.

(By the way, the performance regression alluded to here did not make it to stable, and by now everyone has fully recovered from it I believe.)

Comment 20 by a-...@yandex-team.ru, Aug 15 2017 

Thanks for info. Bug 696038 is closed from non-googlers. 
What was the main reason for regression?

Comment 21 by mpear...@chromium.org, Aug 15 2017 

History started recording transient visits into history (e.g., subframe navigations), and those visits were getting into the HQP database, making it much larger and slower.  This issue happened in Chrome 58, making it all the way to Beta before getting squashed.  Stable users were unaffected, and those erroneously recorded visits should've long ago disappeared from all dev and beta users' histories.
►

Sign in to add a comment