Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1877833002:
Optimize shortcuts provider (Closed)
|
Created:
4 years, 8 months ago by Alexander Yashkin Modified:
4 years, 8 months ago CC:
chromium-reviews, Mark P Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOptimize shortcuts provider
After tracing code in ShortcutsProvider I have come to conclusion that
main source of slow work is usage and full initialization of
AutocompleteMatch structure for every possible match candidate.
Especially, when most of the matches are thrown away after
deduplication and sort by relevance.
I tried to implement an alternative solution by creating and using
ShortcutMatch structure that contains only fields necessary to
deduplication and relevance sort process.
BUG=365932
R=pkasting@chromium.org, mpearson@chromium.org
Committed: https://crrev.com/31940121e40746f5ca59496b857fc5de9cf403af
Cr-Commit-Position: refs/heads/master@{#387820}
Patch Set 1 #Patch Set 2 : Added new files #
Total comments: 22
Patch Set 3 : Fixes after review, round 1 #
Total comments: 20
Patch Set 4 : Fixes after review, round 2 #
Total comments: 40
Patch Set 5 : Fixes after review, round 3 #Patch Set 6 : Reverted rename of ShortcutMatchToACMatch to ShortcutToACMatch #
Total comments: 4
Patch Set 7 : Apply git cl format to CL #Patch Set 8 : Rebase on master and minor format fix #Messages
Total messages: 25 (6 generated)
Do you happen to know what was most slow about the AutocompleteMatch route? I ask because there are other parts of the autocomplete system that may also be slow if constructing these is slow, and perhaps a solution that is focused only on Shortcuts is insufficient.
https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... File components/omnibox/browser/match_compare.h (right): https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... components/omnibox/browser/match_compare.h:24: int GetDemotedRelevance(const Match& match) { Nit: Can be const https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... File components/omnibox/browser/shortcut_match.cc (right): https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... components/omnibox/browser/shortcut_match.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. Nit: Wrong copyright header (no (c)) https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... File components/omnibox/browser/shortcut_match.h (right): https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... components/omnibox/browser/shortcut_match.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. Nit: Wrong copyright header (no (c)) https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... components/omnibox/browser/shortcut_match.h:17: // processing. Nit: How about: ShortcutMatch holds sufficient information about a single match from the shortcut database to allow for destination deduping and relevance sorting. After those stages the top matches are converted to the more heavyweight AutocompleteMatch struct. Avoiding constructing the larger struct for every such match can save significant time when there are many shortcut matches to process. https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... components/omnibox/browser/shortcut_match.h:22: static bool DestinationsEqual(const ShortcutMatch& elem1, Why are these next two methods static on this class instead of just being, say, local to shortcuts_provider.cc? https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... File components/omnibox/browser/shortcut_match_unittest.cc (right): https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... components/omnibox/browser/shortcut_match_unittest.cc:30: // field only. Store it in db_shortcuts vector. Why do we need a vector to hold one object? And why does ShortcutMatchTest need to own the object at all -- why can't this just construct a ShortcutMatch and return it by value? https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... components/omnibox/browser/shortcut_match_unittest.cc:60: // Matches with higher relevance are left from duplicates. Nit: How about: ...The remaining match out of each set of duplicates should be the one with the highest relevance. https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... File components/omnibox/browser/shortcuts_provider.cc (right): https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:161: shortcut_matches.emplace_back(relevance, stripped_destination_url, See the recommendations on https://chromium-cpp.appspot.com/ regarding emplace_back() for performance reasons. https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:162: &(it->second)); Nit: All lines of args should be indented even https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:180: CompareWithDemoteByType<ShortcutMatch> Why did you templatize CompareWithDemoteByType and use that here instead of just using a lambda in the partial_sort() call that's equivalent to AutocompleteMatch::MoreRelevant()? https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:206: term_string, terms_map)); Nit: All lines of args should be indented even
On 2016/04/12 00:32:40, Peter Kasting wrote:
> Do you happen to know what was most slow about the AutocompleteMatch route? I
> ask because there are other parts of the autocomplete system that may also be
> slow if constructing these is slow, and perhaps a solution that is focused
only
> on Shortcuts is insufficient.
I added traces inside ShortcutsProvider::ShortcutToACMatch and the result is
that more than half of function time
is used by calls to RecordAdditionalInfo. Most of the time is spend inside
RecordAdditionalInfo that takes base::Time value as argument.
void AutocompleteMatch::RecordAdditionalInfo(const std::string& property,
const base::Time& value) {
TRACE_EVENT0("omnibox", "RecordAdditionalInfo time");
RecordAdditionalInfo(property,
base::UTF16ToUTF8(
base::TimeFormatShortDateAndTime(value)));
}
Inside of this function most of the time is spend in TimeFormatShortDateAndTime.
Also about 30% of ShortcutToACMatch is spent in calls to
ClassifyAllMatchesInString while calculating match.contents_class and
match.description_class.
I will add screenshot of trace to bug description, cause a could not find how to
add it here.
https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... File components/omnibox/browser/match_compare.h (right): https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... components/omnibox/browser/match_compare.h:24: int GetDemotedRelevance(const Match& match) { On 2016/04/12 00:55:08, Peter Kasting wrote: > Nit: Can be const Done. https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... File components/omnibox/browser/shortcut_match.cc (right): https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... components/omnibox/browser/shortcut_match.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/04/12 00:55:08, Peter Kasting wrote: > Nit: Wrong copyright header (no (c)) Done. https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... File components/omnibox/browser/shortcut_match.h (right): https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... components/omnibox/browser/shortcut_match.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/04/12 00:55:08, Peter Kasting wrote: > Nit: Wrong copyright header (no (c)) Done. https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... components/omnibox/browser/shortcut_match.h:17: // processing. On 2016/04/12 00:55:08, Peter Kasting wrote: > Nit: How about: > > ShortcutMatch holds sufficient information about a single match from the > shortcut database to allow for destination deduping and relevance sorting. > After those stages the top matches are converted to the more heavyweight > AutocompleteMatch struct. Avoiding constructing the larger struct for every > such match can save significant time when there are many shortcut matches to > process. Done, thanks. https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... components/omnibox/browser/shortcut_match.h:22: static bool DestinationsEqual(const ShortcutMatch& elem1, On 2016/04/12 00:55:08, Peter Kasting wrote: > Why are these next two methods static on this class instead of just being, say, > local to shortcuts_provider.cc? I agree that DedupShortcutMatchesByDestination is somewhat foreign to ShortcutMatch interface. Yet DestinationsEqual seems to me like in proper place. I'll move DedupShortcutMatchesByDestination to ShortcutsProvider. I can not make it local there because i want to use it in tests. https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... File components/omnibox/browser/shortcut_match_unittest.cc (right): https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... components/omnibox/browser/shortcut_match_unittest.cc:30: // field only. Store it in db_shortcuts vector. On 2016/04/12 00:55:08, Peter Kasting wrote: > Why do we need a vector to hold one object? And why does ShortcutMatchTest need > to own the object at all -- why can't this just construct a ShortcutMatch and > return it by value? Redone, removed vector of ShortcutMatches. It remained from my previous attempt, where i used Shortcut pointer to access contents string. Now i store contents string in ShortcutMatch object so where is no need to hold ShortcutsDatabase::Shortcut at all. Also moved this test to shortcuts_provider_unittest.cc https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... components/omnibox/browser/shortcut_match_unittest.cc:60: // Matches with higher relevance are left from duplicates. On 2016/04/12 00:55:08, Peter Kasting wrote: > Nit: How about: > > ...The remaining match out of each set of duplicates should be the one with the > highest relevance. Done. https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... File components/omnibox/browser/shortcuts_provider.cc (right): https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:161: shortcut_matches.emplace_back(relevance, stripped_destination_url, On 2016/04/12 00:55:09, Peter Kasting wrote: > See the recommendations on https://chromium-cpp.appspot.com/ regarding > emplace_back() for performance reasons. replaced with push_back(). https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:162: &(it->second)); On 2016/04/12 00:55:08, Peter Kasting wrote: > Nit: All lines of args should be indented even Done. https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:180: CompareWithDemoteByType<ShortcutMatch> On 2016/04/12 00:55:09, Peter Kasting wrote: > Why did you templatize CompareWithDemoteByType and use that here instead of just > using a lambda in the partial_sort() call that's equivalent to > AutocompleteMatch::MoreRelevant()? Fixed. I missed that CompareWithDemoteByType is used only in deduplication process, https://codereview.chromium.org/1877833002/diff/20001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:206: term_string, terms_map)); On 2016/04/12 00:55:08, Peter Kasting wrote: > Nit: All lines of args should be indented even Done.
On 2016/04/12 07:21:35, a-v-y wrote:
> On 2016/04/12 00:32:40, Peter Kasting wrote:
> > Do you happen to know what was most slow about the AutocompleteMatch route?
I
> > ask because there are other parts of the autocomplete system that may also
be
> > slow if constructing these is slow, and perhaps a solution that is focused
> only
> > on Shortcuts is insufficient.
>
> I added traces inside ShortcutsProvider::ShortcutToACMatch and the result is
> that more than half of function time
> is used by calls to RecordAdditionalInfo. Most of the time is spend inside
> RecordAdditionalInfo that takes base::Time value as argument.
>
> void AutocompleteMatch::RecordAdditionalInfo(const std::string& property,
> const base::Time& value) {
> TRACE_EVENT0("omnibox", "RecordAdditionalInfo time");
> RecordAdditionalInfo(property,
> base::UTF16ToUTF8(
> base::TimeFormatShortDateAndTime(value)));
> }
>
> Inside of this function most of the time is spend in
TimeFormatShortDateAndTime.
OK. I checked the other providers and we shouldn't have other cases of this
being called many times.
> Also about 30% of ShortcutToACMatch is spent in calls to
> ClassifyAllMatchesInString while calculating match.contents_class and
> match.description_class.
That one's shortcut-specific so I won't worry about it either.
Thanks for this. I'm comfortable proceeding with this change now.
https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... File components/omnibox/browser/shortcut_match.cc (right): https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... components/omnibox/browser/shortcut_match.cc:24: return elem1.stripped_destination_url == elem2.stripped_destination_url; Nit: Simpler: return (elem1.stripped_destination_url == elem2.stripped_destination_url) && !elem1.stripped_destination_url.is_empty(); (You can also fix autocomplete_match.cc to do this) https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... File components/omnibox/browser/shortcut_match.h (right): https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... components/omnibox/browser/shortcut_match.h:24: static bool DestinationsEqual(const ShortcutMatch& elem1, Nit: Since these two methods are each used just once -- to pass to an STL algorithm -- and none of the data members they refer to are private, it seems like they should both just be lambdas at the callsites. https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... File components/omnibox/browser/shortcuts_provider.cc (right): https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:163: &(it->second))); Nit: Shorter: shortcut_matches.push_back( ShortcutMatch(relevance, stripped_destination_url, &(it->second))); https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:178: &shortcut_matches); After removing the call to AutocompleteResult::DedupMatchesByDestination(), that function's lone non-test caller passes |true| for the second argument, so that function should be simplified to eliminate the second arg (assuming it's always true) and the unittest location that calls it updated to not rely on the |false| behavior. https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:449: DestinationSort<ShortcutMatch> destination_sort(page_classification); Nit: Seems like we could just inline this into the sort() call. https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... File components/omnibox/browser/shortcuts_provider.h (right): https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.h:110: // Matches with higher relevance are left from duplicates. Nit: Let's use a similar comment to what's in AutocompleteResult: // Sorts |matches| by destination, taking into account demotions based on // |page_classification| when resolving ties about which of several // duplicates to keep. The matches are also deduplicated. I also suggest renaming this function, and the one in AutocompleteResult, SortAndDedupMatches(). Also, if we're going to be adding another private static method to this class, can we collect all the static methods together in one group (in the .h and .cc files)? I would put them above the private virtual methods. https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... File components/omnibox/browser/shortcuts_provider_unittest.cc (right): https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider_unittest.cc:233: // Object used only for ShortcutMatch initialization. Nit: Wrong indenting https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider_unittest.cc:234: ShortcutsDatabase::Shortcut fake_shortcut("", base::string16(), Nit: "" -> std::string https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider_unittest.cc:236: return ShortcutMatch(relevance, stripped_destination_url, &fake_shortcut); This leaves a pointer-to-garbage in the ShortcutMatch when this function returns. That's asking for someone later to add code that reads that field. The easiest way to avoid this is to eliminate this helper, move the fake_XXX objects to the caller side, and convert the CreateShortcutMatch() calls to simply calling the ShortcutMatch() constructor directly. https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider_unittest.cc:789: EXPECT_EQ(matches.size(), 3U); Nit: (expected, actual) (all lines)
https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... File components/omnibox/browser/shortcut_match.cc (right): https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... components/omnibox/browser/shortcut_match.cc:24: return elem1.stripped_destination_url == elem2.stripped_destination_url; On 2016/04/12 23:29:50, Peter Kasting wrote: > Nit: Simpler: > > return (elem1.stripped_destination_url == elem2.stripped_destination_url) && > !elem1.stripped_destination_url.is_empty(); > > (You can also fix autocomplete_match.cc to do this) Replaced here and in autocomplete_match.cc with return !elem1.stripped_destination_url.is_empty() && (elem1.stripped_destination_url == elem2.stripped_destination_url); I suspect that its more optimal to check that first url is non empty before comparing both urls content. https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... File components/omnibox/browser/shortcut_match.h (right): https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... components/omnibox/browser/shortcut_match.h:24: static bool DestinationsEqual(const ShortcutMatch& elem1, On 2016/04/12 23:29:50, Peter Kasting wrote: > Nit: Since these two methods are each used just once -- to pass to an STL > algorithm -- and none of the data members they refer to are private, it seems > like they should both just be lambdas at the callsites. Done, yet i had to inline this call to match_compare.h also. After this changes I dont see much sense in keeping this structure in separate file. I'll move it inside ShortcutsProvider class. https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... File components/omnibox/browser/shortcuts_provider.cc (right): https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:163: &(it->second))); On 2016/04/12 23:29:50, Peter Kasting wrote: > Nit: Shorter: > > shortcut_matches.push_back( > ShortcutMatch(relevance, stripped_destination_url, &(it->second))); Done. https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:178: &shortcut_matches); On 2016/04/12 23:29:50, Peter Kasting wrote: > After removing the call to AutocompleteResult::DedupMatchesByDestination(), that > function's lone non-test caller passes |true| for the second argument, so that > function should be simplified to eliminate the second arg (assuming it's always > true) and the unittest location that calls it updated to not rely on the |false| > behavior. Done. https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:449: DestinationSort<ShortcutMatch> destination_sort(page_classification); On 2016/04/12 23:29:50, Peter Kasting wrote: > Nit: Seems like we could just inline this into the sort() call. Done. https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... File components/omnibox/browser/shortcuts_provider.h (right): https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.h:110: // Matches with higher relevance are left from duplicates. On 2016/04/12 23:29:50, Peter Kasting wrote: > Nit: Let's use a similar comment to what's in AutocompleteResult: > > // Sorts |matches| by destination, taking into account demotions based on > // |page_classification| when resolving ties about which of several > // duplicates to keep. The matches are also deduplicated. > > I also suggest renaming this function, and the one in AutocompleteResult, > SortAndDedupMatches(). > > Also, if we're going to be adding another private static method to this class, > can we collect all the static methods together in one group (in the .h and .cc > files)? I would put them above the private virtual methods. Done, thanks. https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... File components/omnibox/browser/shortcuts_provider_unittest.cc (right): https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider_unittest.cc:233: // Object used only for ShortcutMatch initialization. On 2016/04/12 23:29:50, Peter Kasting wrote: > Nit: Wrong indenting Done. https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider_unittest.cc:234: ShortcutsDatabase::Shortcut fake_shortcut("", base::string16(), On 2016/04/12 23:29:50, Peter Kasting wrote: > Nit: "" -> std::string Done. https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider_unittest.cc:236: return ShortcutMatch(relevance, stripped_destination_url, &fake_shortcut); On 2016/04/12 23:29:51, Peter Kasting wrote: > This leaves a pointer-to-garbage in the ShortcutMatch when this function > returns. That's asking for someone later to add code that reads that field. > > The easiest way to avoid this is to eliminate this helper, move the fake_XXX > objects to the caller side, and convert the CreateShortcutMatch() calls to > simply calling the ShortcutMatch() constructor directly. Done. https://codereview.chromium.org/1877833002/diff/40001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider_unittest.cc:789: EXPECT_EQ(matches.size(), 3U); On 2016/04/12 23:29:50, Peter Kasting wrote: > Nit: (expected, actual) (all lines) Done.
https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... File components/omnibox/browser/autocomplete_result.cc (right): https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/autocomplete_result.cc:24: using metrics::OmniboxEventProto; Nit: You can remove this now and explicitly qualify the usages below. https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... File components/omnibox/browser/match_compare.h (right): https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/match_compare.h:10: using metrics::OmniboxEventProto; Nit: Avoid this using directive, just explicitly qualify below. https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/match_compare.h:25: OmniboxFieldTrial::DemotionMultipliers::const_iterator demotion_it = Nit: Wrong indentation (this line is in too far, next isn't far enough) https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/match_compare.h:51: OmniboxEventProto::PageClassification current_page_classification) : Nit: ':' goes at the beginning of the next line, not the end of this one https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/match_compare.h:53: bool operator()(const Match& elem1, Nit: Unnecessary linebreak https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/match_compare.h:64: if (matches_destinations_equal || matches_destinations_empty) { This is all more complicated than need be. The logic just boils down to: return (elem1.stripped_destination_url == elem2.stripped_destination_url) ? demote_by_type_(elem1, elem2) : (elem1.stripped_destination_url < elem2.stripped_destination_url); https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... File components/omnibox/browser/shortcuts_provider.cc (right): https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:57: const ShortcutsDatabase::Shortcut* shortcut) : Nit: ':' goes at the beginning of the next line, not the end of this one https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:58: relevance(relevance), Nit: These lines are not sufficiently indented https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:131: // static Nit: Might want to move these functions up in a separate CL (feel free to TBR me) just so it's clear what about them you're actually changing in this CL. https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:298: ShortcutMatch(relevance, stripped_destination_url, &(it->second))); Nit: Parens around -> operator not necessary https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:323: // "stable" sort across multiple updates. Nit: This comment is a bit odd since this code is not being run for multiple providers. I'd change to: // Ensure a stable sort by sorting equal-relevance matches alphabetically. https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:333: // Create and initialize autocomplete matches from shortcut matches. Nit: Wrap all comment lines as close to 80 columns as possible https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:340: match.relevance = max_relevance; Rather than update match.relevance, it would be better to just pass the desired relevance to ShortcutToACMatch(), as was being done before. This would allow this loop to mark |match| as a const ref instead of a non-const ref. And if you also implement my comment below, there's no longer a need to pass the whole ShortcutMatch at all -- just passing the contained Shortcut will be sufficient. https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:372: match.stripped_destination_url = shortcut_match.stripped_destination_url; Does this matter? It seems likely that AutocompleteResult::SortAndCull() will be called after this and recompute this anyway, in which case we might as well not bother to do this. https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:412: languages_, Nit: Put on previous line https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... File components/omnibox/browser/shortcuts_provider.h (right): https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.h:40: struct ShortcutMatch { This struct doesn't need to access any members of ShortcutsProvider, so I would not make it a member struct. I would keep it as a toplevel struct, probably with its own (very short) header and .cc file since you want to be able to sue it in the unittest. (The other option would be to change the unittests to test a larger piece of functionality than just SortAndDedupMatches(), e.g. GetMatches(), which would not take ShortcutMatches directly as input, and then make this struct local to the .cc file here.) https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.h:56: FRIEND_TEST_ALL_PREFIXES(ShortcutsProviderTest, SortAndDedupMatches); Nit: If this is only needed because of the call to SortAndDedupMatches(), I'd rather make that function public and eliminate this. https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... File components/omnibox/browser/shortcuts_provider_unittest.cc (right): https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider_unittest.cc:766: db_shortcut.match_core.contents = base::ASCIIToUTF16("123"); Nit: Set this value in the constructor above, push all the matches using it onto the vector first, and eliminate all the explicit sets of ("123"). https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider_unittest.cc:767: matches.push_back( Nit: While normally push_back() is more readable, I'm inclined to suggest using emplace_back() on these lines just so they can all be fit onto one line each. (That would also eliminate the need for the using directive at the top.) WDYT? https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider_unittest.cc:791: EXPECT_EQ(matches[0].stripped_destination_url, GURL("http://www.abc.com")); Nit: (expected, actual) (many places)
https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... File components/omnibox/browser/autocomplete_result.cc (right): https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/autocomplete_result.cc:24: using metrics::OmniboxEventProto; On 2016/04/14 23:52:33, Peter Kasting wrote: > Nit: You can remove this now and explicitly qualify the usages below. Done. https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... File components/omnibox/browser/match_compare.h (right): https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/match_compare.h:10: using metrics::OmniboxEventProto; On 2016/04/14 23:52:33, Peter Kasting wrote: > Nit: Avoid this using directive, just explicitly qualify below. Done. Yet, IMHO, metrics::OmniboxEventProto::PageClassification looks too long in arguments list. https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/match_compare.h:25: OmniboxFieldTrial::DemotionMultipliers::const_iterator demotion_it = On 2016/04/14 23:52:33, Peter Kasting wrote: > Nit: Wrong indentation (this line is in too far, next isn't far enough) Done. https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/match_compare.h:51: OmniboxEventProto::PageClassification current_page_classification) : On 2016/04/14 23:52:33, Peter Kasting wrote: > Nit: ':' goes at the beginning of the next line, not the end of this one Done. https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/match_compare.h:53: bool operator()(const Match& elem1, On 2016/04/14 23:52:33, Peter Kasting wrote: > Nit: Unnecessary linebreak Done. https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/match_compare.h:64: if (matches_destinations_equal || matches_destinations_empty) { On 2016/04/14 23:52:33, Peter Kasting wrote: > This is all more complicated than need be. The logic just boils down to: > > return (elem1.stripped_destination_url == elem2.stripped_destination_url) ? > demote_by_type_(elem1, elem2) : > (elem1.stripped_destination_url < elem2.stripped_destination_url); Good point. Done. https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... File components/omnibox/browser/shortcuts_provider.cc (right): https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:57: const ShortcutsDatabase::Shortcut* shortcut) : On 2016/04/14 23:52:33, Peter Kasting wrote: > Nit: ':' goes at the beginning of the next line, not the end of this one Done. https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:58: relevance(relevance), On 2016/04/14 23:52:33, Peter Kasting wrote: > Nit: These lines are not sufficiently indented Done. https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:131: // static On 2016/04/14 23:52:33, Peter Kasting wrote: > Nit: Might want to move these functions up in a separate CL (feel free to TBR > me) just so it's clear what about them you're actually changing in this CL. Reverted move. https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:298: ShortcutMatch(relevance, stripped_destination_url, &(it->second))); On 2016/04/14 23:52:33, Peter Kasting wrote: > Nit: Parens around -> operator not necessary Done. https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:323: // "stable" sort across multiple updates. On 2016/04/14 23:52:33, Peter Kasting wrote: > Nit: This comment is a bit odd since this code is not being run for multiple > providers. I'd change to: > > // Ensure a stable sort by sorting equal-relevance matches alphabetically. Done. https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:333: // Create and initialize autocomplete matches from shortcut matches. On 2016/04/14 23:52:33, Peter Kasting wrote: > Nit: Wrap all comment lines as close to 80 columns as possible Done. https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:340: match.relevance = max_relevance; On 2016/04/14 23:52:33, Peter Kasting wrote: > Rather than update match.relevance, it would be better to just pass the desired > relevance to ShortcutToACMatch(), as was being done before. This would allow > this loop to mark |match| as a const ref instead of a non-const ref. And if you > also implement my comment below, there's no longer a need to pass the whole > ShortcutMatch at all -- just passing the contained Shortcut will be sufficient. Done. https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:372: match.stripped_destination_url = shortcut_match.stripped_destination_url; On 2016/04/14 23:52:33, Peter Kasting wrote: > Does this matter? It seems likely that AutocompleteResult::SortAndCull() will > be called after this and recompute this anyway, in which case we might as well > not bother to do this. Done. https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.cc:412: languages_, On 2016/04/14 23:52:33, Peter Kasting wrote: > Nit: Put on previous line Done. https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... File components/omnibox/browser/shortcuts_provider.h (right): https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.h:40: struct ShortcutMatch { On 2016/04/14 23:52:33, Peter Kasting wrote: > This struct doesn't need to access any members of ShortcutsProvider, so I would > not make it a member struct. I would keep it as a toplevel struct, probably > with its own (very short) header and .cc file since you want to be able to sue > it in the unittest. (The other option would be to change the unittests to test > a larger piece of functionality than just SortAndDedupMatches(), e.g. > GetMatches(), which would not take ShortcutMatches directly as input, and then > make this struct local to the .cc file here.) I moved ShortcutMatch structure and SortAndDedupMatches function to shortcuts_provider.cc anonimous namespace. I removed unittest for SortAndDedupMatches entirely because I believe its functionality is already tested by existing ShortcutsProvider tests. https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider.h:56: FRIEND_TEST_ALL_PREFIXES(ShortcutsProviderTest, SortAndDedupMatches); On 2016/04/14 23:52:34, Peter Kasting wrote: > Nit: If this is only needed because of the call to SortAndDedupMatches(), I'd > rather make that function public and eliminate this. Removed. https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... File components/omnibox/browser/shortcuts_provider_unittest.cc (right): https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider_unittest.cc:766: db_shortcut.match_core.contents = base::ASCIIToUTF16("123"); On 2016/04/14 23:52:34, Peter Kasting wrote: > Nit: Set this value in the constructor above, push all the matches using it onto > the vector first, and eliminate all the explicit sets of ("123"). I removed unittest for SortAndDedupMatches entirely because I believe its functionality is already tested by existing ShortcutsProvider tests. https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider_unittest.cc:767: matches.push_back( On 2016/04/14 23:52:34, Peter Kasting wrote: > Nit: While normally push_back() is more readable, I'm inclined to suggest using > emplace_back() on these lines just so they can all be fit onto one line each. > (That would also eliminate the need for the using directive at the top.) WDYT? Removed test entirely. https://codereview.chromium.org/1877833002/diff/60001/components/omnibox/brow... components/omnibox/browser/shortcuts_provider_unittest.cc:791: EXPECT_EQ(matches[0].stripped_destination_url, GURL("http://www.abc.com")); On 2016/04/14 23:52:34, Peter Kasting wrote: > Nit: (expected, actual) (many places) Removed test entirely.
I'm moving myself to the CC list for this changelist. I'm sure pkasting@'s detailed review will be more than sufficient. Either of you should feel free to let me know if there's anything subtle about ranking or other behavioral changes going on here that you want me to look at. I'm assuming this change shouldn't change the order and relevance or any results at all ever (not even edge cases); the only visible change is improved performance. Let me know if I'm wrong. --mark
LGTM https://codereview.chromium.org/1877833002/diff/100001/components/omnibox/bro... File components/omnibox/browser/match_compare.h (right): https://codereview.chromium.org/1877833002/diff/100001/components/omnibox/bro... components/omnibox/browser/match_compare.h:38: (demoted_relevance1 > demoted_relevance2); Not sure this indentation is correct for the ?: operator. (2 places) I'd probably just run git cl format and have done with it. https://codereview.chromium.org/1877833002/diff/100001/components/omnibox/bro... File components/omnibox/browser/shortcuts_provider.cc (right): https://codereview.chromium.org/1877833002/diff/100001/components/omnibox/bro... components/omnibox/browser/shortcuts_provider.cc:62: stripped_destination_url(stripped_destination_url), These lines are still not correctly indented. The member name needs to line up with the name in the line above.
https://codereview.chromium.org/1877833002/diff/100001/components/omnibox/bro... File components/omnibox/browser/match_compare.h (right): https://codereview.chromium.org/1877833002/diff/100001/components/omnibox/bro... components/omnibox/browser/match_compare.h:38: (demoted_relevance1 > demoted_relevance2); On 2016/04/15 20:23:44, Peter Kasting wrote: > Not sure this indentation is correct for the ?: operator. (2 places) > > I'd probably just run git cl format and have done with it. Sorry, I completely forgot about git cl format command. Applied it. https://codereview.chromium.org/1877833002/diff/100001/components/omnibox/bro... File components/omnibox/browser/shortcuts_provider.cc (right): https://codereview.chromium.org/1877833002/diff/100001/components/omnibox/bro... components/omnibox/browser/shortcuts_provider.cc:62: stripped_destination_url(stripped_destination_url), On 2016/04/15 20:23:44, Peter Kasting wrote: > These lines are still not correctly indented. The member name needs to line up > with the name in the line above. Apllied git cl format and it fixed this problem..
On 2016/04/15 20:23:45, Peter Kasting wrote: > LGTM > > https://codereview.chromium.org/1877833002/diff/100001/components/omnibox/bro... > File components/omnibox/browser/match_compare.h (right): > > https://codereview.chromium.org/1877833002/diff/100001/components/omnibox/bro... > components/omnibox/browser/match_compare.h:38: (demoted_relevance1 > > demoted_relevance2); > Not sure this indentation is correct for the ?: operator. (2 places) > > I'd probably just run git cl format and have done with it. > > https://codereview.chromium.org/1877833002/diff/100001/components/omnibox/bro... > File components/omnibox/browser/shortcuts_provider.cc (right): > > https://codereview.chromium.org/1877833002/diff/100001/components/omnibox/bro... > components/omnibox/browser/shortcuts_provider.cc:62: > stripped_destination_url(stripped_destination_url), > These lines are still not correctly indented. The member name needs to line up > with the name in the line above. Thanks for thorough and thoughtful and patient review :)
The CQ bit was checked by a-v-y@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1877833002/#ps120001 (title: "Apply git cl format to CL")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1877833002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1877833002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by a-v-y@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1877833002/#ps140001 (title: "Rebase on master and minor format fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1877833002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1877833002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Optimize shortcuts provider After tracing code in ShortcutsProvider I have come to conclusion that main source of slow work is usage and full initialization of AutocompleteMatch structure for every possible match candidate. Especially, when most of the matches are thrown away after deduplication and sort by relevance. I tried to implement an alternative solution by creating and using ShortcutMatch structure that contains only fields necessary to deduplication and relevance sort process. BUG=365932 R=pkasting@chromium.org, mpearson@chromium.org ========== to ========== Optimize shortcuts provider After tracing code in ShortcutsProvider I have come to conclusion that main source of slow work is usage and full initialization of AutocompleteMatch structure for every possible match candidate. Especially, when most of the matches are thrown away after deduplication and sort by relevance. I tried to implement an alternative solution by creating and using ShortcutMatch structure that contains only fields necessary to deduplication and relevance sort process. BUG=365932 R=pkasting@chromium.org, mpearson@chromium.org Committed: https://crrev.com/31940121e40746f5ca59496b857fc5de9cf403af Cr-Commit-Position: refs/heads/master@{#387820} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/31940121e40746f5ca59496b857fc5de9cf403af Cr-Commit-Position: refs/heads/master@{#387820} |
