Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2864103003:
Limit the number of history urls indexed for omnibox suggestions. (Closed)
DescriptionLimit the number of history urls indexed for omnibox suggestions.
The limit is set by experiment args and by default there is no
limit.
BUG=715852
Review-Url: https://codereview.chromium.org/2864103003
Cr-Commit-Position: refs/heads/master@{#470742}
Committed: https://chromium.googlesource.com/chromium/src/+/887dee671bfb3f1862912eae6d92a127e67f6325
Patch Set 1 #
Total comments: 1
Patch Set 2 : Fixes. #
Total comments: 12
Patch Set 3 : address comments. #
Total comments: 8
Patch Set 4 : Address comments. #
Messages
Total messages: 32 (17 generated)
Description was changed from ========== Limit the number of histiry urls indexed for omnibox suggestions. BUG=715852 ========== to ========== Limit the number of histiry urls indexed for omnibox suggestions. The limit is set by command line flag and by default there is no limit. BUG=715852 ==========
+Mark, wdyt? I am not sure if I can enable experiments using command line flags. I will look into that soon.
ssid@chromium.org changed reviewers: + mpearson@chromium.org
+Mark, wdyt? I am not sure if I can enable experiments using command line flags. I will look into that soon.
Seems like a plausible approach. Some comments: 1. please add tests to url_database_unittest.cc 2. please don't use a command line switch, instead use a feature flag, which can put in components/omnibox/browser/omnibox_field_trial.h and have the feature flag associated with a parameter name. The key to this will be calling GetFieldTrialParamValueByFeature(). You may want a simple wrapper function, say something like, LimitHQPIndexSizeAtStartup() (I invite you to come up with a better name) that calls GetFieldTrialParamValueByFeature, much like the wrappe function here does for example: https://cs.chromium.org/chromium/src/components/omnibox/browser/omnibox_field... 3. This is only a limit that applies at startup (well, profile open). Is that intentional? It's not a big deal in Android because Chrome gets killed and restarted frequently, but it should be clearly indicated as such in the variable names and comments, as the code clearly applies cross-platform. --mark https://codereview.chromium.org/2864103003/diff/1/components/omnibox/browser/... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2864103003/diff/1/components/omnibox/browser/... components/omnibox/browser/url_index_private_data.cc:403: // Limit the nuber of URLs indexed on low end devices. This would degrade the nit: would degrade -> degrades
Description was changed from ========== Limit the number of histiry urls indexed for omnibox suggestions. The limit is set by command line flag and by default there is no limit. BUG=715852 ========== to ========== Limit the number of history urls indexed for omnibox suggestions. The limit is set by command line flag and by default there is no limit. BUG=715852 ==========
Thanks for your comments. Made changes as suggested. ptal thanks!
Generally fine. --mark https://codereview.chromium.org/2864103003/diff/20001/components/history/core... File components/history/core/browser/url_database_unittest.cc (right): https://codereview.chromium.org/2864103003/diff/20001/components/history/core... components/history/core/browser/url_database_unittest.cc:237: TEST_F(URLDatabaseTest, EnumeratorForSignificant) { Please restructure this test a bit. Right now, for example, if enumerator simply returned suggestions in a first-added-first-out order, it would pass the test. https://codereview.chromium.org/2864103003/diff/20001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.h (right): https://codereview.chromium.org/2864103003/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:412: // Returns the maximum number of history urls to index at the startup. Note: There are multiple history systems. You're only limiting one. This comment and function name should mention which one you're talking about here (HistoryQuick provider). You can use HQP in the function name, enough people who touch this file will recognize that. ditto const below https://codereview.chromium.org/2864103003/diff/20001/components/omnibox/brow... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2864103003/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:407: int max_urls_indexed = OmniboxFieldTrial::MaxNumHistoryUrlsIndexedAtStartup(); nit: const https://codereview.chromium.org/2864103003/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:407: int max_urls_indexed = OmniboxFieldTrial::MaxNumHistoryUrlsIndexedAtStartup(); I don't think our field trial config can make an experiment target low-end devices explicitly. I think you might want code here (or perhaps in the omnibox_field_trial.cc implementation of the function to return -1 on those devices?) that restricts the trial to just those devices.
Thanks, made changes as suggested. ptal. https://codereview.chromium.org/2864103003/diff/20001/components/history/core... File components/history/core/browser/url_database_unittest.cc (right): https://codereview.chromium.org/2864103003/diff/20001/components/history/core... components/history/core/browser/url_database_unittest.cc:237: TEST_F(URLDatabaseTest, EnumeratorForSignificant) { On 2017/05/08 22:39:17, Mark P wrote: > Please restructure this test a bit. Right now, for example, if enumerator > simply returned suggestions in a first-added-first-out order, it would pass the > test. good point. Done. https://codereview.chromium.org/2864103003/diff/20001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.h (right): https://codereview.chromium.org/2864103003/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:412: // Returns the maximum number of history urls to index at the startup. Note: On 2017/05/08 22:39:17, Mark P wrote: > There are multiple history systems. You're only limiting one. This comment and > function name should mention which one you're talking about here (HistoryQuick > provider). You can use HQP in the function name, enough people who touch this > file will recognize that. > > ditto const below Ah sorry. I coulnd't find what hqp means from the code. So, I just named it history after your last comment. Fixed thanks! https://codereview.chromium.org/2864103003/diff/20001/components/omnibox/brow... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2864103003/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:407: int max_urls_indexed = OmniboxFieldTrial::MaxNumHistoryUrlsIndexedAtStartup(); On 2017/05/08 22:39:17, Mark P wrote: > I don't think our field trial config can make an experiment target low-end > devices explicitly. I think you might want code here (or perhaps in the > omnibox_field_trial.cc implementation of the function to return -1 on those > devices?) that restricts the trial to just those devices. I was thinking Lets first experiment for all devices and see how the suggestions are affected. I looked at History.InMemoryTypedUrlVisitCount and it shows more than 100k urls in some cases. If we can find a sweet spot to limit this number for suggestions then we could leave those devices little more usable. Once we find the limit I think we could remove this experiment and set hard limits based on low-end / high end devices. wdyt? https://codereview.chromium.org/2864103003/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:407: int max_urls_indexed = OmniboxFieldTrial::MaxNumHistoryUrlsIndexedAtStartup(); On 2017/05/08 22:39:17, Mark P wrote: > nit: const Done.
ssid@chromium.org changed reviewers: + sdefresne@chromium.org
+sdefresne for history/ changes.
Description was changed from ========== Limit the number of history urls indexed for omnibox suggestions. The limit is set by command line flag and by default there is no limit. BUG=715852 ========== to ========== Limit the number of history urls indexed for omnibox suggestions. The limit is set by experiment args and by default there is no limit. BUG=715852 ==========
lgtm https://codereview.chromium.org/2864103003/diff/40001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.cc (right): https://codereview.chromium.org/2864103003/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.cc:639: if (!param_value.empty() && base::StringToInt(param_value, &num_urls)) nit: base::StringToInt returns false for empty strings, so the "!param_value.empty()" check can be removed https://codereview.chromium.org/2864103003/diff/40001/components/omnibox/brow... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2864103003/diff/40001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:411: // Do not use <= to account for case of -1 for unlimited urls. nit: Should this comment be "// Do not use >= to account for case of -1 for unlimited urls."?
https://codereview.chromium.org/2864103003/diff/20001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.h (right): https://codereview.chromium.org/2864103003/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:412: // Returns the maximum number of history urls to index at the startup. Note: On 2017/05/10 01:01:35, ssid wrote: > On 2017/05/08 22:39:17, Mark P wrote: > > There are multiple history systems. You're only limiting one. This comment > and > > function name should mention which one you're talking about here (HistoryQuick > > provider). You can use HQP in the function name, enough people who touch this > > file will recognize that. > > > > ditto const below > > Ah sorry. I coulnd't find what hqp means from the code. So, I just named it > history after your last comment. > Fixed thanks! Yeah, I agree the names are confusing. The basic structure is that the "URL Index Private Data" is an index used by "History Quick Provider", the omnibox system that's supposed to search most of the user's history intelligently and quickly. https://codereview.chromium.org/2864103003/diff/20001/components/omnibox/brow... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2864103003/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:407: int max_urls_indexed = OmniboxFieldTrial::MaxNumHistoryUrlsIndexedAtStartup(); On 2017/05/10 01:01:35, ssid wrote: > On 2017/05/08 22:39:17, Mark P wrote: > > I don't think our field trial config can make an experiment target low-end > > devices explicitly. I think you might want code here (or perhaps in the > > omnibox_field_trial.cc implementation of the function to return -1 on those > > devices?) that restricts the trial to just those devices. > > I was thinking Lets first experiment for all devices and see how the suggestions > are affected. > > I looked at History.InMemoryTypedUrlVisitCount and it shows more than 100k urls > in some cases. If we can find a sweet spot to limit this number for suggestions > then we could leave those devices little more usable. Once we find the limit I > think we could remove this experiment and set hard limits based on low-end / > high end devices. > > wdyt? I'm reluctant. We recently had discussions and complaints that led to filing bugs 692215 and 692386, which both involve allowing more items to go into the index. Doing this blanket on all devices or platforms feels like it's moving in the wrong direction. Also, experiment config cannot target low-end devices explicitly. Unless you put in code now, you cannot decide to do that later without another code change and rollout. If you feel strongly about wanting to consider targeting all devices, I suggest you expand the parameters your experiment looks at, e.g., have two parameters MaxNumHQPUrlsIndexedAtStartupOnLowEndDevices and MaxNumHQPUrlsIndexedAtStartupOnNonLowEndDevices and have the function MaxNumHQPUrlsIndexedAtStartup() examine the appropriate one depending on device settings. https://codereview.chromium.org/2864103003/diff/40001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.cc (right): https://codereview.chromium.org/2864103003/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.cc:635: int OmniboxFieldTrial::MaxNumHQPUrlsIndexedAtStartup() { nit: .cc function order should match .h order (I do appreciate you not moving this function originally so I could see the diff during the review.) https://codereview.chromium.org/2864103003/diff/40001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.h (right): https://codereview.chromium.org/2864103003/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:306: // For experitment to limit HQP url indexing that's part of the bundled nit: typo (experitment)
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks made changes https://codereview.chromium.org/2864103003/diff/20001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.h (right): https://codereview.chromium.org/2864103003/diff/20001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:412: // Returns the maximum number of history urls to index at the startup. Note: On 2017/05/10 17:23:09, Mark P wrote: > On 2017/05/10 01:01:35, ssid wrote: > > On 2017/05/08 22:39:17, Mark P wrote: > > > There are multiple history systems. You're only limiting one. This comment > > and > > > function name should mention which one you're talking about here > (HistoryQuick > > > provider). You can use HQP in the function name, enough people who touch > this > > > file will recognize that. > > > > > > ditto const below > > > > Ah sorry. I coulnd't find what hqp means from the code. So, I just named it > > history after your last comment. > > Fixed thanks! > > Yeah, I agree the names are confusing. > > The basic structure is that the "URL Index Private Data" is an index used by > "History Quick Provider", the omnibox system that's supposed to search most of > the user's history intelligently and quickly. Acknowledged. https://codereview.chromium.org/2864103003/diff/20001/components/omnibox/brow... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2864103003/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:407: int max_urls_indexed = OmniboxFieldTrial::MaxNumHistoryUrlsIndexedAtStartup(); On 2017/05/10 17:23:09, Mark P wrote: > On 2017/05/10 01:01:35, ssid wrote: > > On 2017/05/08 22:39:17, Mark P wrote: > > > I don't think our field trial config can make an experiment target low-end > > > devices explicitly. I think you might want code here (or perhaps in the > > > omnibox_field_trial.cc implementation of the function to return -1 on those > > > devices?) that restricts the trial to just those devices. > > > > I was thinking Lets first experiment for all devices and see how the > suggestions > > are affected. > > > > I looked at History.InMemoryTypedUrlVisitCount and it shows more than 100k > urls > > in some cases. If we can find a sweet spot to limit this number for > suggestions > > then we could leave those devices little more usable. Once we find the limit I > > think we could remove this experiment and set hard limits based on low-end / > > high end devices. > > > > wdyt? > > I'm reluctant. We recently had discussions and complaints that led to filing > bugs 692215 and 692386, which both involve allowing more items to go into the > index. Doing this blanket on all devices or platforms feels like it's moving in > the wrong direction. Okay I am only looking to fix the very high numbers in UMA. Maybe a limit of say 1000 or even 10000 would be good, which I would suspect not change the quality. > Also, experiment config cannot target low-end devices explicitly. Unless you > put in code now, you cannot decide to do that later without another code change > and rollout. > Yes, this was my initial plan. To change the code later. > If you feel strongly about wanting to consider targeting all devices, I suggest > you expand the parameters your experiment looks at, e.g., have two parameters > MaxNumHQPUrlsIndexedAtStartupOnLowEndDevices and > MaxNumHQPUrlsIndexedAtStartupOnNonLowEndDevices and have the function > MaxNumHQPUrlsIndexedAtStartup() examine the appropriate one depending on device > settings. Thanks! Made it 2 different params as you suggested. https://codereview.chromium.org/2864103003/diff/40001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.cc (right): https://codereview.chromium.org/2864103003/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.cc:635: int OmniboxFieldTrial::MaxNumHQPUrlsIndexedAtStartup() { On 2017/05/10 17:23:09, Mark P wrote: > nit: .cc function order should match .h order > > (I do appreciate you not moving this function originally so I could see the diff > during the review.) Sorry. I actually forgot to move this. https://codereview.chromium.org/2864103003/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.cc:639: if (!param_value.empty() && base::StringToInt(param_value, &num_urls)) On 2017/05/10 08:59:41, sdefresne wrote: > nit: base::StringToInt returns false for empty strings, so the > "!param_value.empty()" check can be removed Done. Also some other functions have similar checks. Not removing them in this cl. Eg: GetPhysicalWebAfterTypingBaseRelevance https://codereview.chromium.org/2864103003/diff/40001/components/omnibox/brow... File components/omnibox/browser/omnibox_field_trial.h (right): https://codereview.chromium.org/2864103003/diff/40001/components/omnibox/brow... components/omnibox/browser/omnibox_field_trial.h:306: // For experitment to limit HQP url indexing that's part of the bundled On 2017/05/10 17:23:09, Mark P wrote: > nit: typo (experitment) Done. https://codereview.chromium.org/2864103003/diff/40001/components/omnibox/brow... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2864103003/diff/40001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:411: // Do not use <= to account for case of -1 for unlimited urls. On 2017/05/10 08:59:41, sdefresne wrote: > nit: Should this comment be "// Do not use >= to account for case of -1 for > unlimited urls."? Done.
lgtm Please test this interactively before submitting using --force-fieldtrials= and --force-fieldtrial-params= and about:histograms to make sure it's doing what you want. --mark
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/10 19:56:46, Mark P wrote: > lgtm > > Please test this interactively before submitting using --force-fieldtrials= and > --force-fieldtrial-params= and about:histograms to make sure it's doing what you > want. > > --mark Verified it works. thanks
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2864103003/#ps60001 (title: "Address comments.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494457125825320,
"parent_rev": "12111562acf829a2beaf6ec01fe95e5c0bdca46e", "commit_rev":
"887dee671bfb3f1862912eae6d92a127e67f6325"}
Description was changed from ========== Limit the number of history urls indexed for omnibox suggestions. The limit is set by experiment args and by default there is no limit. BUG=715852 ========== to ========== Limit the number of history urls indexed for omnibox suggestions. The limit is set by experiment args and by default there is no limit. BUG=715852 Review-Url: https://codereview.chromium.org/2864103003 Cr-Commit-Position: refs/heads/master@{#470742} Committed: https://chromium.googlesource.com/chromium/src/+/887dee671bfb3f1862912eae6d92... ==========
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/887dee671bfb3f1862912eae6d92...
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/887dee671bfb3f1862912eae6d92... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
