Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2355273002:
Redirect handling in the resource_prefetch_predictor. (Closed)
DescriptionRedirect handling in the resource_prefetch_predictor.
Collect and save information about enpoints in redirect chains that
prefetch predictor can start to prefetch subresources before it reaches
last redirect hop.
Information about redirects is stored in the database keyed by url (or host)
of first hop in redirect chain. One row (RedirectData proto message)
contains collection of all previously seen urls (or hosts) appearing
as endpoints of redirect chain (RedirectStat proto message).
This CL depends on https://codereview.chromium.org/2357593002
BUG=631966
Committed: https://crrev.com/43b2f02a9eb2bc50e849d6120caa1de84abcbe46
Cr-Commit-Position: refs/heads/master@{#421520}
Patch Set 1 #Patch Set 2 : Fix tests + bug in database #Patch Set 3 : Minor changes #
Total comments: 50
Patch Set 4 : rebase and nits #
Total comments: 1
Patch Set 5 : Reusable DeleteDataHelper #
Total comments: 20
Patch Set 6 : Minor changes. #
Total comments: 21
Patch Set 7 : More nits. #
Total comments: 7
Patch Set 8 : Renaming refactoring. #Patch Set 9 : Revert table names, delete sensitive data, refactor. #
Total comments: 33
Patch Set 10 : Comments and enums (and rebase). #
Total comments: 4
Patch Set 11 : Very tiny nit. #Patch Set 12 : Rebase. #Patch Set 13 : Rename TableOperationType::DELETE because of win macro. #Patch Set 14 : Fix compilation complaints. #
Total comments: 1
Messages
Total messages: 50 (23 generated)
Description was changed from ========== Redirect handling in the resource_prefetch_predictor. Collect and save information about enpoints in redirect chains that prefetch predictor can start to prefetch subresources before he reach last redirect hop. BUG=631966 ========== to ========== Redirect handling in the resource_prefetch_predictor. Collect and save information about enpoints in redirect chains that prefetch predictor can start to prefetch subresources before it reaches last redirect hop. Information about redirects is stored in the database keyed by url (or host) of first hop in redirect chain. One row (RedirectData proto message) contains collection of all previously seen urls (or hosts) appearing as endpoints of redirect chain (RedirectStat proto message). This CL depends on https://codereview.chromium.org/2357593002 BUG=631966 ==========
alexilin@chromium.org changed reviewers: + lizeb@chromium.org, pasko@chromium.org
Hi guys, Please take a look at this CL. It is more or less similar to https://codereview.chromium.org/2321343002 (Benoit has already seen it).
Overall looks quite reasonable! Suggestions about headers, proto, histograms. I haven't looked at the implementation, would prefer to do that after this round of fixes. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:491: navigation.reset(new Navigation(response.navigation_id.main_frame_url)); why not MakeUnique here as well? https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:103: struct Navigation { The name 'Navigation' looks a bit generic, so it slightly complicates reading resource_prefetch_predictor.cc. RedirectChain would be more readable, at least to me. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.proto (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.proto:76: repeated RedirectStat redirects = 3; This proto would benefit from comments and probably renaming of this field. Is |redirects| a redirect chain or a collection of endpoints in all redirect chains for primary_key? https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.h:32: using RedirectStat = chrome_browser_predictors::RedirectData::RedirectStat; why not using chrome_browser_predictors::RedirectData::RedirectStat? https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.h:51: static void SortRedirects(std::vector<RedirectStat>* redirects); typedefs and struct declarations should go before methods, see: https://google.github.io/styleguide/cppguide.html#Declaration_Order https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.h:141: void DeleteRedirectDataHelper(PrefetchKeyType key_type, these two Delete* methods do the same thing with a minor variation of deleting from the metadata table. To avoid code duplication it feels better to joing them into one function: Something like: DeleteDataHelper(key_type, value_type, keys) And: * either provide bool delete_from_metadata parameter * or introduce DeleteMetadataHelper(key_type, keys) up to you on the latter https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.h:149: // Computes score of |data| I guess some sort of merging/rebasing decided to revert your previous change here. So now I finally provided a useful comment. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_test_util.h (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_test_util.h:38: // Google Mock can't find these declarations and unit tests are not compiled. the last part of the sentence is not necessary (starting from "and") https://codereview.chromium.org/2355273002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2355273002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:49341: + <owner>alexilin@chromium.org</owner> please put lizeb@ as another owner here and in another new histogram
https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:491: navigation.reset(new Navigation(response.navigation_id.main_frame_url)); On 2016/09/22 14:27:09, pasko wrote: > why not MakeUnique here as well? How to connect unique_ptr::reset() with MakeUnique? https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:103: struct Navigation { On 2016/09/22 14:27:09, pasko wrote: > The name 'Navigation' looks a bit generic, so it slightly complicates reading > resource_prefetch_predictor.cc. RedirectChain would be more readable, at least > to me. I don't think that 'RedirectChain' is also a good name. Because this structure holds the all information that 'ResourcePrefetchPredictor' collects within single navigation. 'subresource_requests' contains all requests to subresources as well, so 'RedirectChain' name isn't relevant. Actually, this struct could be moved in private section, because it isn't used outside 'ResourcePrefetchPredictor'. So then we could keep 'NavigationInfo' or 'NavigationData' name. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.proto (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.proto:76: repeated RedirectStat redirects = 3; On 2016/09/22 14:27:09, pasko wrote: > This proto would benefit from comments and probably renaming of this field. Is > |redirects| a redirect chain or a collection of endpoints in all redirect chains > for primary_key? Collection of endpoints. Ok, let's agree on the names before I'll make painful renaming. redirects -> endpoints ? Also, it could be reasonable to rename RedirectStat::url field because it could be a host as well (when we use host-based prefetching). Rename it to 'key'? https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.h:32: using RedirectStat = chrome_browser_predictors::RedirectData::RedirectStat; On 2016/09/22 14:27:09, pasko wrote: > why not using chrome_browser_predictors::RedirectData::RedirectStat? Because compiler doesn't like this: "error: using declaration cannot refer to class member" https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.h:51: static void SortRedirects(std::vector<RedirectStat>* redirects); On 2016/09/22 14:27:09, pasko wrote: > typedefs and struct declarations should go before methods, see: > > https://google.github.io/styleguide/cppguide.html#Declaration_Order Ok! Btw, I don't see nested struct/class in this list. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.h:141: void DeleteRedirectDataHelper(PrefetchKeyType key_type, On 2016/09/22 14:27:09, pasko wrote: > these two Delete* methods do the same thing with a minor variation of deleting > from the metadata table. To avoid code duplication it feels better to joing them > into one function: > > Something like: > DeleteDataHelper(key_type, value_type, keys) > > And: > * either provide bool delete_from_metadata parameter > * or introduce DeleteMetadataHelper(key_type, keys) > > up to you on the latter About all of these helpers... Honestly, I hope in the future to get rid of PrefetchData structure as well (and use another proto instead of it). Then we could delete 'resource_url' column from resource table and get unified table scheme (TEXT, BLOB). After that we could introduce unified helpers that will take base proto MessageLite class. But for now I'll change it as you proposed. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.h:149: // Computes score of |data| On 2016/09/22 14:27:09, pasko wrote: > I guess some sort of merging/rebasing decided to revert your previous change > here. So now I finally provided a useful comment. Oh nooo! My period is GONE!
https://codereview.chromium.org/2355273002/diff/60001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): https://codereview.chromium.org/2355273002/diff/60001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.h:32: using chrome_browser_predictors::RedirectData::RedirectStat; Woops, it won't be compiled
Hi, sorry about the delay. Here is a first batch of (old) comments, based on a previous patchset. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:489: // If we lost information about first hop for some reason I'm not entirely sure about what should be done here. Do you know why that may happen? https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:637: std::unique_ptr<PrefetchDataMap> url_data_map(new PrefetchDataMap()); nit: Can you use MakeUnique, here and below? auto my_ptr = base::MakeUnique<Type>(args); https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:644: RedirectDataMap* url_redirect_data_ptr = url_redirect_data_map.get(); nit: you can directly call .get() below. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.proto (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.proto:7: // resource_prefetch_predictor.h. Oops, can you correct my mistake here as well? This is resource_prefetch_predictor_tables.h... https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.proto:66: // Represents single redirect chain endpoint nit: Represents a single redirect chain endpoint. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:57: "main_page_url TEXT, " Would this be "initial_url" instead?
Hi Benoit, Here is answers on your previous comments. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:489: // If we lost information about first hop for some reason On 2016/09/26 11:18:36, Benoit L wrote: > I'm not entirely sure about what should be done here. Do you know why that may > happen? It could happen in some rare cases, for example, the request could be deleted in CleanupAbandonedNavigations function (timeout expired) but appears in OnMainFrameRedirect again. I don't know about possible problems on observer side because I don't have a picture of whole navigation flow. I mean, could observer gives us redirect without previous request? It seems like if ShouldRecordRequest passes than ShouldRecordRedirect also should pass and vice versa. Is it possible to have a redirect from non HTTP(S) scheme to HTTP(S)? https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:637: std::unique_ptr<PrefetchDataMap> url_data_map(new PrefetchDataMap()); On 2016/09/26 11:18:36, Benoit L wrote: > nit: Can you use MakeUnique, here and below? > > auto my_ptr = base::MakeUnique<Type>(args); Done. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:644: RedirectDataMap* url_redirect_data_ptr = url_redirect_data_map.get(); On 2016/09/26 11:18:36, Benoit L wrote: > nit: you can directly call .get() below. Done. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.proto (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.proto:7: // resource_prefetch_predictor.h. On 2016/09/26 11:18:36, Benoit L wrote: > Oops, can you correct my mistake here as well? > > This is resource_prefetch_predictor_tables.h... Done. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:57: "main_page_url TEXT, " On 2016/09/26 11:18:36, Benoit L wrote: > Would this be "initial_url" instead? Yeah, but then we will have to have two different delete statements. Maybe rename all primary keys just as "key"?
just respoding to responses to decide on naming earlier, I may send a few more comments later today https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:491: navigation.reset(new Navigation(response.navigation_id.main_frame_url)); On 2016/09/22 16:48:19, alexilin wrote: > On 2016/09/22 14:27:09, pasko wrote: > > why not MakeUnique here as well? > > How to connect unique_ptr::reset() with MakeUnique? navigation = MakeUnique<Navigation>(...)? https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:103: struct Navigation { On 2016/09/22 16:48:19, alexilin wrote: > On 2016/09/22 14:27:09, pasko wrote: > > The name 'Navigation' looks a bit generic, so it slightly complicates reading > > resource_prefetch_predictor.cc. RedirectChain would be more readable, at least > > to me. > > I don't think that 'RedirectChain' is also a good name. > Because this structure holds the all information that > 'ResourcePrefetchPredictor' collects within single navigation. > 'subresource_requests' contains all requests to subresources as well, so > 'RedirectChain' name isn't relevant. > > Actually, this struct could be moved in private section, because it isn't used > outside 'ResourcePrefetchPredictor'. So then we could keep 'NavigationInfo' or > 'NavigationData' name. I would also prefer moving it into the implementation if it is possible. How about 'PageRequestSummary' or 'PageLoadRequestInfo' or some variation of these? My understanding of 'Navigation' does not include subresources, only remotely relevant. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.proto (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.proto:66: // Represents single redirect chain endpoint On 2016/09/26 11:18:36, Benoit L wrote: > nit: Represents a single redirect chain endpoint. .. except that it is a mapping of origin to all possible endpoints, right? https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.proto:76: repeated RedirectStat redirects = 3; On 2016/09/22 16:48:19, alexilin wrote: > On 2016/09/22 14:27:09, pasko wrote: > > This proto would benefit from comments and probably renaming of this field. Is > > |redirects| a redirect chain or a collection of endpoints in all redirect > chains > > for primary_key? > > Collection of endpoints. > Ok, let's agree on the names before I'll make painful renaming. > > redirects -> endpoints ? I would suggest redirect_endpoints > Also, it could be reasonable to rename RedirectStat::url field because it could > be a host as well (when we use host-based prefetching). Rename it to 'key'? I'd prefer |primary_key| for consistency, but I agree it is weird to have primary keys in a proto repeated field. We can also invent a new name for url-or-host, ideas: * url_or_host * hrl * target .. 'host' is probably also a bad name because we care about scheme+host+port, which is effectively an 'origin', but this renaming is certainly *not* for this change. WDYT? https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.h:32: using RedirectStat = chrome_browser_predictors::RedirectData::RedirectStat; On 2016/09/22 16:48:19, alexilin wrote: > On 2016/09/22 14:27:09, pasko wrote: > > why not using chrome_browser_predictors::RedirectData::RedirectStat? > > Because compiler doesn't like this: "error: using declaration cannot refer to > class member" Oh, I did not know it, thanks! https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.h:51: static void SortRedirects(std::vector<RedirectStat>* redirects); On 2016/09/22 16:48:19, alexilin wrote: > On 2016/09/22 14:27:09, pasko wrote: > > typedefs and struct declarations should go before methods, see: > > > > https://google.github.io/styleguide/cppguide.html#Declaration_Order > > Ok! > Btw, I don't see nested struct/class in this list. This was confusing to me as well, I would put them in the same category as 'Typedefs and Enums'. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_test_util.h (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_test_util.h:38: // Google Mock can't find these declarations and unit tests are not compiled. On 2016/09/22 14:27:09, pasko wrote: > the last part of the sentence is not necessary (starting from "and") plz do not forget to respond with 'Done' in cases like this, see my other comment for explanation https://codereview.chromium.org/2355273002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2355273002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:49341: + <owner>alexilin@chromium.org</owner> On 2016/09/22 14:27:09, pasko wrote: > please put lizeb@ as another owner here and in another new histogram replying to myself :) please respond to _all_ comments that are done, with 'Done', so that the amount of responses matches the number of initial comments. That way it is easier to make sure no comment gets lost by mistake.
A few more comments, I haven't looked at the tests yet. Looks fine overall, even though I share your sentiment that there is quite a lot of code duplication here... https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:459: // A redirect will not lead to another OnMainFrameRequest call, so record the Please update this comment. https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:797: LearnNavigation(host, PREFETCH_KEY_TYPE_HOST, nit: Wouldn't the code be a bit simpler by passing the Navigation object to LearnNavigation()? https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:17: #include "base/memory/linked_ptr.h" Is it still needed? https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:107: GURL initial_url; nit: Can you add a comment to specify whether this is before or after redirects? https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:440: ? GetHostRedirectUpdateStatement() tiny nit: is this the result of git cl format? If not, please re-run it on the updated CL. https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:443: if (!inserter->Run()) nit: What about: return inserter->Run(); ? (here and above) https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:652: statement.Assign(DB()->GetUniqueStatement( I'm not sure these histograms are really useful now. Let's discuss, but I would lean towards not introducing them now.
https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:440: ? GetHostRedirectUpdateStatement() On 2016/09/26 12:51:14, Benoit L wrote: > tiny nit: is this the result of git cl format? > > If not, please re-run it on the updated CL. lol: I was actually checking it with git cl format, while there is certainly reasonable logic behind it, the end result indeed looks weird to humans. I am wondering if this could be one function: GetTableUpdateStatement(URL/HOST, RESOURCE/REDIRECT, UPDATE/DELETE). https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:652: statement.Assign(DB()->GetUniqueStatement( On 2016/09/26 12:51:14, Benoit L wrote: > I'm not sure these histograms are really useful now. > > Let's discuss, but I would lean towards not introducing them now. I probably lean towards the same opinion because: * more histograms now make reviews slower * we will rethink the histograms after the implementation is done, and these ones in the new light could appear to be better as part of something other https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:443: // TODO(shishir): There are significant gains to be had here if we can use the it is time to remove this TODO https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:710: base::Bind(&ResourcePrefetchPredictorTables::DeleteResourceData, DeletaAllUrls() is hooked into privacy-related cleanup on user request, in which case it is important to also clean up the redirect data https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:772: // TODO(alexilin) make only one request to DB thread nit: punctuation: TODO(alexilin): make only one request to DB thread. https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:774: // URL level data - merge only if we are already saving the data, or we it while we are here, let's fix it to be proper English. https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:805: const std::string& redirect_origin_key, this has nothing to do with web origin, so the name is confusing, how about: key_before_redirects ? https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:1013: if (redirects.size() > kMaxRedirectsPerEntry) this looks like dead code because trimming from consecutive_misses guarantees that redirects.size() <= max_consecutive_misses, right? https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.h:30: using chrome_browser_predictors::ResourceData; Please put a comment above these declarations: // From resource_prefetch_predictor.proto https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right): https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:779: // in resource table will be deleted s/deleted/deleted./ Same below: add full stops in all comments where it is not present. https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:807: // Tests that redirect is recorded correctly for URL already present in If the comment applies to the test as a whole, it should go above the test. https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:851: // Exising redirect to https://facebook.com/login will be deleted because of s/Exising/Existing/
Hi guys, I've fixed some nits and non-nits, but that's not all. Anyway, I'd like to share with you some comments. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:491: navigation.reset(new Navigation(response.navigation_id.main_frame_url)); On 2016/09/26 12:28:15, pasko wrote: > On 2016/09/22 16:48:19, alexilin wrote: > > On 2016/09/22 14:27:09, pasko wrote: > > > why not MakeUnique here as well? > > > > How to connect unique_ptr::reset() with MakeUnique? > > navigation = MakeUnique<Navigation>(...)? There will be one extra construction of unique_ptr, but well.. Done. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.proto (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.proto:66: // Represents single redirect chain endpoint On 2016/09/26 12:28:15, pasko wrote: > On 2016/09/26 11:18:36, Benoit L wrote: > > nit: Represents a single redirect chain endpoint. > > .. except that it is a mapping of origin to all possible endpoints, right? This comment refers to RedirectStat message, not RedirectData. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.h:32: using RedirectStat = chrome_browser_predictors::RedirectData::RedirectStat; On 2016/09/26 12:28:15, pasko wrote: > On 2016/09/22 16:48:19, alexilin wrote: > > On 2016/09/22 14:27:09, pasko wrote: > > > why not using chrome_browser_predictors::RedirectData::RedirectStat? > > > > Because compiler doesn't like this: "error: using declaration cannot refer to > > class member" > > Oh, I did not know it, thanks! Done. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.h:51: static void SortRedirects(std::vector<RedirectStat>* redirects); On 2016/09/26 12:28:15, pasko wrote: > On 2016/09/22 16:48:19, alexilin wrote: > > On 2016/09/22 14:27:09, pasko wrote: > > > typedefs and struct declarations should go before methods, see: > > > > > > https://google.github.io/styleguide/cppguide.html#Declaration_Order > > > > Ok! > > Btw, I don't see nested struct/class in this list. > > This was confusing to me as well, I would put them in the same category as > 'Typedefs and Enums'. Done. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.h:149: // Computes score of |data| On 2016/09/22 16:48:19, alexilin wrote: > On 2016/09/22 14:27:09, pasko wrote: > > I guess some sort of merging/rebasing decided to revert your previous change > > here. So now I finally provided a useful comment. > > Oh nooo! My period is GONE! Done. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_test_util.h (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_test_util.h:38: // Google Mock can't find these declarations and unit tests are not compiled. On 2016/09/26 12:28:15, pasko wrote: > On 2016/09/22 14:27:09, pasko wrote: > > the last part of the sentence is not necessary (starting from "and") > > plz do not forget to respond with 'Done' in cases like this, see my other > comment for explanation Done. https://codereview.chromium.org/2355273002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2355273002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:49341: + <owner>alexilin@chromium.org</owner> On 2016/09/26 12:28:15, pasko wrote: > On 2016/09/22 14:27:09, pasko wrote: > > please put lizeb@ as another owner here and in another new histogram > > replying to myself :) > > please respond to _all_ comments that are done, with 'Done', so that the amount > of responses matches the number of initial comments. That way it is easier to > make sure no comment gets lost by mistake. Done. https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:459: // A redirect will not lead to another OnMainFrameRequest call, so record the On 2016/09/26 12:51:14, Benoit L wrote: > Please update this comment. Done. https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:797: LearnNavigation(host, PREFETCH_KEY_TYPE_HOST, On 2016/09/26 12:51:14, Benoit L wrote: > nit: Wouldn't the code be a bit simpler by passing the Navigation object to > LearnNavigation()? It looks like no. We should still pass keys (|key| and |redirect_origin_key|) as strings because of spec()/host() difference. We can figure out right conversion in LearnNavigation by |key_type| but it doesn't look consistent with providing pointers to the right Prefetch/RedirectDataMap. Suppose we are okay with it. We obtain |key| from NavigationID and |redirect_origin_key| from Navigation, so we should pass (NavigationID, Navigation) instead of (string, string, vector) + additional conversions in LearnNavigation code. Not so much simpler. https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:17: #include "base/memory/linked_ptr.h" On 2016/09/26 12:51:14, Benoit L wrote: > Is it still needed? Done. https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:107: GURL initial_url; On 2016/09/26 12:51:14, Benoit L wrote: > nit: Can you add a comment to specify whether this is before or after redirects? Done. https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:440: ? GetHostRedirectUpdateStatement() On 2016/09/26 14:18:23, pasko wrote: > On 2016/09/26 12:51:14, Benoit L wrote: > > tiny nit: is this the result of git cl format? > > > > If not, please re-run it on the updated CL. > > lol: I was actually checking it with git cl format, while there is certainly > reasonable logic behind it, the end result indeed looks weird to humans. > > I am wondering if this could be one function: > GetTableUpdateStatement(URL/HOST, RESOURCE/REDIRECT, UPDATE/DELETE). There is the problem with SQL cached statements and SQL_FROM_HERE macro. I could write this common get statement function but we should still save different DB()->GetCachedStatement calls for all tables and operations. https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:443: if (!inserter->Run()) On 2016/09/26 12:51:14, Benoit L wrote: > nit: What about: > > return inserter->Run(); > > ? > > (here and above) Done. https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:443: // TODO(shishir): There are significant gains to be had here if we can use the On 2016/09/26 14:18:23, pasko wrote: > it is time to remove this TODO Done. https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:772: // TODO(alexilin) make only one request to DB thread On 2016/09/26 14:18:23, pasko wrote: > nit: punctuation: TODO(alexilin): make only one request to DB thread. Done. https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:774: // URL level data - merge only if we are already saving the data, or we it On 2016/09/26 14:18:23, pasko wrote: > while we are here, let's fix it to be proper English. You are asking the wrong person :) But I've tried. https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:1013: if (redirects.size() > kMaxRedirectsPerEntry) On 2016/09/26 14:18:23, pasko wrote: > this looks like dead code because trimming from consecutive_misses guarantees > that redirects.size() <= max_consecutive_misses, right? It depends on values of max_consecutive_misses and kMaxRedirectsPerEntry. Can we guarantee that max_consecutive_misses is always less than kMaxRedirectsPerEntry? https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.h:30: using chrome_browser_predictors::ResourceData; On 2016/09/26 14:18:23, pasko wrote: > Please put a comment above these declarations: > // From resource_prefetch_predictor.proto Done. https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right): https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:779: // in resource table will be deleted On 2016/09/26 14:18:23, pasko wrote: > s/deleted/deleted./ > > Same below: add full stops in all comments where it is not present. Done. https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:807: // Tests that redirect is recorded correctly for URL already present in On 2016/09/26 14:18:23, pasko wrote: > If the comment applies to the test as a whole, it should go above the test. Also fixed existing code. Done. https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:851: // Exising redirect to https://facebook.com/login will be deleted because of On 2016/09/26 14:18:23, pasko wrote: > s/Exising/Existing/ Done. https://codereview.chromium.org/2355273002/diff/120001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2355273002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:12: #include <set> git cl lint suggestion.
some responses to some of your answers :) https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:491: navigation.reset(new Navigation(response.navigation_id.main_frame_url)); On 2016/09/26 15:38:28, alexilin wrote: > On 2016/09/26 12:28:15, pasko wrote: > > On 2016/09/22 16:48:19, alexilin wrote: > > > On 2016/09/22 14:27:09, pasko wrote: > > > > why not MakeUnique here as well? > > > > > > How to connect unique_ptr::reset() with MakeUnique? > > > > navigation = MakeUnique<Navigation>(...)? > > There will be one extra construction of unique_ptr, but well.. I "think" on-stack unique_ptr constructors have all chances to be optimized by the compiler.. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.proto (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.proto:66: // Represents single redirect chain endpoint On 2016/09/26 15:38:28, alexilin wrote: > On 2016/09/26 12:28:15, pasko wrote: > > On 2016/09/26 11:18:36, Benoit L wrote: > > > nit: Represents a single redirect chain endpoint. > > > > .. except that it is a mapping of origin to all possible endpoints, right? > > This comment refers to RedirectStat message, not RedirectData. Ah oh, agreed. I was confused because of lack of comment for the RedirectData. Also, an empty line above this comment would help identifying where it attributes to. https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:1013: if (redirects.size() > kMaxRedirectsPerEntry) On 2016/09/26 15:38:29, alexilin wrote: > On 2016/09/26 14:18:23, pasko wrote: > > this looks like dead code because trimming from consecutive_misses guarantees > > that redirects.size() <= max_consecutive_misses, right? > > It depends on values of max_consecutive_misses and kMaxRedirectsPerEntry. > Can we guarantee that max_consecutive_misses is always less than > kMaxRedirectsPerEntry? Yeah, I think so. The max_consecutive_misses in the long run characterizes how much wasted work we are performing, and this one should be kept small. On the other hand, the memory footprint of a bunch of URLs is not really concerning, can reach dozens without anyone noticing. On-disk size is the least concerning. Let's hear what Benoit says.
https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:440: ? GetHostRedirectUpdateStatement() On 2016/09/26 15:38:28, alexilin wrote: > On 2016/09/26 14:18:23, pasko wrote: > > On 2016/09/26 12:51:14, Benoit L wrote: > > > tiny nit: is this the result of git cl format? > > > > > > If not, please re-run it on the updated CL. > > > > lol: I was actually checking it with git cl format, while there is certainly > > reasonable logic behind it, the end result indeed looks weird to humans. > > > > I am wondering if this could be one function: > > GetTableUpdateStatement(URL/HOST, RESOURCE/REDIRECT, UPDATE/DELETE). > > There is the problem with SQL cached statements and SQL_FROM_HERE macro. > I could write this common get statement function but we should still save > different DB()->GetCachedStatement calls for all tables and operations. hah, you are right, the way sql::StatementID is created by SQL_FROM_HERE would not work for us. How about: sql::StatementID(__FILE__, is_host | (is_redirect << 1) | (is_update << 2)) ?
Only small comments remaining, I'm happy with the code. Looking at the tests now. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:489: // If we lost information about first hop for some reason On 2016/09/26 12:14:54, alexilin wrote: > On 2016/09/26 11:18:36, Benoit L wrote: > > I'm not entirely sure about what should be done here. Do you know why that may > > happen? > > It could happen in some rare cases, for example, the request could be deleted in > CleanupAbandonedNavigations function (timeout expired) but appears in > OnMainFrameRedirect again. > > I don't know about possible problems on observer side because I don't have a > picture of whole navigation flow. I mean, could observer gives us redirect > without previous request? > It seems like if ShouldRecordRequest passes than ShouldRecordRedirect also > should pass and vice versa. > Is it possible to have a redirect from non HTTP(S) scheme to HTTP(S)? Ok, thanks. Only a nit then: // If we lost the information about the first hop for some reason. https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:797: LearnNavigation(host, PREFETCH_KEY_TYPE_HOST, On 2016/09/26 15:38:28, alexilin wrote: > On 2016/09/26 12:51:14, Benoit L wrote: > > nit: Wouldn't the code be a bit simpler by passing the Navigation object to > > LearnNavigation()? > > It looks like no. > We should still pass keys (|key| and |redirect_origin_key|) as strings because > of spec()/host() difference. We can figure out right conversion in > LearnNavigation by |key_type| but it doesn't look consistent with providing > pointers to the right Prefetch/RedirectDataMap. > > Suppose we are okay with it. We obtain |key| from NavigationID and > |redirect_origin_key| from Navigation, so we should pass (NavigationID, > Navigation) instead of (string, string, vector) + additional conversions in > LearnNavigation code. Not so much simpler. Ok, thanks for the explanation. https://codereview.chromium.org/2355273002/diff/120001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2355273002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:465: // redirect url as a new navigation id and save initial url in navigation. nit: and save the initial url. https://codereview.chromium.org/2355273002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:770: // URL level data - merge only either we've already saved the data, or it nit: "merge only if we already saved the data, or it meets the ..." https://codereview.chromium.org/2355273002/diff/120001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://codereview.chromium.org/2355273002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:30: const char kUrlResourceTableName[] = "resource_prefetch_predictor_url_resource"; Dont'change the name of the existing tables. It's unfortunate since it leads to less naming consistency, but the database is shared with other predictors. Which means that we cannot nuke the table when updating the schema. This means that your change will lead undead tables behind, since the outdated tables are removed in DropTablesIfOutdated() with the names you have modified here.
On 2016/09/27 09:55:06, Benoit L wrote: > Only small comments remaining, I'm happy with the code. > Looking at the tests now. > > https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... > File chrome/browser/predictors/resource_prefetch_predictor.cc (right): > > https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... > chrome/browser/predictors/resource_prefetch_predictor.cc:489: // If we lost > information about first hop for some reason > On 2016/09/26 12:14:54, alexilin wrote: > > On 2016/09/26 11:18:36, Benoit L wrote: > > > I'm not entirely sure about what should be done here. Do you know why that > may > > > happen? > > > > It could happen in some rare cases, for example, the request could be deleted > in > > CleanupAbandonedNavigations function (timeout expired) but appears in > > OnMainFrameRedirect again. > > > > I don't know about possible problems on observer side because I don't have a > > picture of whole navigation flow. I mean, could observer gives us redirect > > without previous request? > > It seems like if ShouldRecordRequest passes than ShouldRecordRedirect also > > should pass and vice versa. > > Is it possible to have a redirect from non HTTP(S) scheme to HTTP(S)? > > Ok, thanks. > > Only a nit then: > // If we lost the information about the first hop for some reason. > > https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... > File chrome/browser/predictors/resource_prefetch_predictor.cc (right): > > https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... > chrome/browser/predictors/resource_prefetch_predictor.cc:797: > LearnNavigation(host, PREFETCH_KEY_TYPE_HOST, > On 2016/09/26 15:38:28, alexilin wrote: > > On 2016/09/26 12:51:14, Benoit L wrote: > > > nit: Wouldn't the code be a bit simpler by passing the Navigation object to > > > LearnNavigation()? > > > > It looks like no. > > We should still pass keys (|key| and |redirect_origin_key|) as strings because > > of spec()/host() difference. We can figure out right conversion in > > LearnNavigation by |key_type| but it doesn't look consistent with providing > > pointers to the right Prefetch/RedirectDataMap. > > > > Suppose we are okay with it. We obtain |key| from NavigationID and > > |redirect_origin_key| from Navigation, so we should pass (NavigationID, > > Navigation) instead of (string, string, vector) + additional conversions in > > LearnNavigation code. Not so much simpler. > > Ok, thanks for the explanation. > > https://codereview.chromium.org/2355273002/diff/120001/chrome/browser/predict... > File chrome/browser/predictors/resource_prefetch_predictor.cc (right): > > https://codereview.chromium.org/2355273002/diff/120001/chrome/browser/predict... > chrome/browser/predictors/resource_prefetch_predictor.cc:465: // redirect url as > a new navigation id and save initial url in navigation. > nit: and save the initial url. > > https://codereview.chromium.org/2355273002/diff/120001/chrome/browser/predict... > chrome/browser/predictors/resource_prefetch_predictor.cc:770: // URL level data > - merge only either we've already saved the data, or it > nit: "merge only if we already saved the data, or it meets the ..." > > https://codereview.chromium.org/2355273002/diff/120001/chrome/browser/predict... > File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): > > https://codereview.chromium.org/2355273002/diff/120001/chrome/browser/predict... > chrome/browser/predictors/resource_prefetch_predictor_tables.cc:30: const char > kUrlResourceTableName[] = "resource_prefetch_predictor_url_resource"; > Dont'change the name of the existing tables. It's unfortunate since it leads to > less naming consistency, but the database is shared with other predictors. Which > means that we cannot nuke the table when updating the schema. > > This means that your change will lead undead tables behind, since the outdated > tables are removed in DropTablesIfOutdated() with the names you have modified > here. LGTM once the table names are changed, but please wait for pasko's approval as well before sending this to the CQ.
Hi Egor, Benoit, Following things were changed: - Made renaming: * Navigation -> PageRequestSummary * redirects -> redirect_endpoints * namespace chrome_browser_predictors -> predictors - Refactored ResourcePrefetchPredictorTables::Get*Statement function. Now it's one function with huge switch that accepts 3 params. (hack with custom sql::StatementID included). - Table names was reverted by Benoit request - ResourcePrefetchPredictor::DeleteUrls also deletes redirect data now. Benoit, there are some messages that wait for your response: https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... > "main_page_url TEXT, " > On 2016/09/26 11:18:36, Benoit L wrote: > > Would this be "initial_url" instead? > > Yeah, but then we will have to have two different delete > statements. > Maybe rename all primary keys just as "key"? https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... > if (redirects.size() > kMaxRedirectsPerEntry) > On 2016/09/26 15:38:29, alexilin wrote: > > On 2016/09/26 14:18:23, pasko wrote: > > > this looks like dead code because trimming from consecutive_misses guarantees > > > that redirects.size() <= max_consecutive_misses, right? > > > > It depends on values of max_consecutive_misses and kMaxRedirectsPerEntry. > > Can we guarantee that max_consecutive_misses is always less than > > kMaxRedirectsPerEntry? > > Yeah, I think so. The max_consecutive_misses in the long run characterizes how > much wasted work we are performing, and this one should be kept small. On the > other hand, the memory footprint of a bunch of URLs is not really concerning, > can reach dozens without anyone noticing. On-disk size is the least concerning. > > Let's hear what Benoit says. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:489: // If we lost information about first hop for some reason On 2016/09/27 09:55:06, Benoit L wrote: > On 2016/09/26 12:14:54, alexilin wrote: > > On 2016/09/26 11:18:36, Benoit L wrote: > > > I'm not entirely sure about what should be done here. Do you know why that > may > > > happen? > > > > It could happen in some rare cases, for example, the request could be deleted > in > > CleanupAbandonedNavigations function (timeout expired) but appears in > > OnMainFrameRedirect again. > > > > I don't know about possible problems on observer side because I don't have a > > picture of whole navigation flow. I mean, could observer gives us redirect > > without previous request? > > It seems like if ShouldRecordRequest passes than ShouldRecordRedirect also > > should pass and vice versa. > > Is it possible to have a redirect from non HTTP(S) scheme to HTTP(S)? > > Ok, thanks. > > Only a nit then: > // If we lost the information about the first hop for some reason. Thanks! Done. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:491: navigation.reset(new Navigation(response.navigation_id.main_frame_url)); On 2016/09/26 15:58:22, pasko wrote: > On 2016/09/26 15:38:28, alexilin wrote: > > On 2016/09/26 12:28:15, pasko wrote: > > > On 2016/09/22 16:48:19, alexilin wrote: > > > > On 2016/09/22 14:27:09, pasko wrote: > > > > > why not MakeUnique here as well? > > > > > > > > How to connect unique_ptr::reset() with MakeUnique? > > > > > > navigation = MakeUnique<Navigation>(...)? > > > > There will be one extra construction of unique_ptr, but well.. > > I "think" on-stack unique_ptr constructors have all chances to be optimized by > the compiler.. Done. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:103: struct Navigation { On 2016/09/26 12:28:15, pasko wrote: > On 2016/09/22 16:48:19, alexilin wrote: > > On 2016/09/22 14:27:09, pasko wrote: > > > The name 'Navigation' looks a bit generic, so it slightly complicates > reading > > > resource_prefetch_predictor.cc. RedirectChain would be more readable, at > least > > > to me. > > > > I don't think that 'RedirectChain' is also a good name. > > Because this structure holds the all information that > > 'ResourcePrefetchPredictor' collects within single navigation. > > 'subresource_requests' contains all requests to subresources as well, so > > 'RedirectChain' name isn't relevant. > > > > Actually, this struct could be moved in private section, because it isn't used > > outside 'ResourcePrefetchPredictor'. So then we could keep 'NavigationInfo' or > > 'NavigationData' name. > > I would also prefer moving it into the implementation if it is possible. > > How about 'PageRequestSummary' or 'PageLoadRequestInfo' or some variation of > these? My understanding of 'Navigation' does not include subresources, only > remotely relevant. Renamed to 'PageRequestSummary' and moved into private section of ResourcePrefetchPredictor class. Done. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.proto (right): https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.proto:66: // Represents single redirect chain endpoint On 2016/09/26 15:58:22, pasko wrote: > On 2016/09/26 15:38:28, alexilin wrote: > > On 2016/09/26 12:28:15, pasko wrote: > > > On 2016/09/26 11:18:36, Benoit L wrote: > > > > nit: Represents a single redirect chain endpoint. > > > > > > .. except that it is a mapping of origin to all possible endpoints, right? > > > > This comment refers to RedirectStat message, not RedirectData. > > Ah oh, agreed. I was confused because of lack of comment for the RedirectData. > Also, an empty line above this comment would help identifying where it > attributes to. Added comment for the RedirectData. 'git cl format' doesn't allow to add an empty line above this comment. Done. https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.proto:76: repeated RedirectStat redirects = 3; On 2016/09/26 12:28:15, pasko wrote: > On 2016/09/22 16:48:19, alexilin wrote: > > On 2016/09/22 14:27:09, pasko wrote: > > > This proto would benefit from comments and probably renaming of this field. > Is > > > |redirects| a redirect chain or a collection of endpoints in all redirect > > chains > > > for primary_key? > > > > Collection of endpoints. > > Ok, let's agree on the names before I'll make painful renaming. > > > > redirects -> endpoints ? > > I would suggest redirect_endpoints > > > Also, it could be reasonable to rename RedirectStat::url field because it > could > > be a host as well (when we use host-based prefetching). Rename it to 'key'? > > I'd prefer |primary_key| for consistency, but I agree it is weird to have > primary keys in a proto repeated field. We can also invent a new name for > url-or-host, ideas: > * url_or_host > * hrl > * target > > .. 'host' is probably also a bad name because we care about scheme+host+port, > which is effectively an 'origin', but this renaming is certainly *not* for this > change. > > WDYT? Renamed redirects -> redirect_endpoints. Leave further renaming for next reviews. Done. https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:797: LearnNavigation(host, PREFETCH_KEY_TYPE_HOST, On 2016/09/27 09:55:06, Benoit L wrote: > On 2016/09/26 15:38:28, alexilin wrote: > > On 2016/09/26 12:51:14, Benoit L wrote: > > > nit: Wouldn't the code be a bit simpler by passing the Navigation object to > > > LearnNavigation()? > > > > It looks like no. > > We should still pass keys (|key| and |redirect_origin_key|) as strings because > > of spec()/host() difference. We can figure out right conversion in > > LearnNavigation by |key_type| but it doesn't look consistent with providing > > pointers to the right Prefetch/RedirectDataMap. > > > > Suppose we are okay with it. We obtain |key| from NavigationID and > > |redirect_origin_key| from Navigation, so we should pass (NavigationID, > > Navigation) instead of (string, string, vector) + additional conversions in > > LearnNavigation code. Not so much simpler. > > Ok, thanks for the explanation. Done. https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:440: ? GetHostRedirectUpdateStatement() On 2016/09/26 16:44:06, pasko wrote: > On 2016/09/26 15:38:28, alexilin wrote: > > On 2016/09/26 14:18:23, pasko wrote: > > > On 2016/09/26 12:51:14, Benoit L wrote: > > > > tiny nit: is this the result of git cl format? > > > > > > > > If not, please re-run it on the updated CL. > > > > > > lol: I was actually checking it with git cl format, while there is certainly > > > reasonable logic behind it, the end result indeed looks weird to humans. > > > > > > I am wondering if this could be one function: > > > GetTableUpdateStatement(URL/HOST, RESOURCE/REDIRECT, UPDATE/DELETE). > > > > There is the problem with SQL cached statements and SQL_FROM_HERE macro. > > I could write this common get statement function but we should still save > > different DB()->GetCachedStatement calls for all tables and operations. > > hah, you are right, the way sql::StatementID is created by SQL_FROM_HERE would > not work for us. > > How about: > > sql::StatementID(__FILE__, is_host | (is_redirect << 1) | (is_update << 2)) ? Done. https://codereview.chromium.org/2355273002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:652: statement.Assign(DB()->GetUniqueStatement( On 2016/09/26 14:18:23, pasko wrote: > On 2016/09/26 12:51:14, Benoit L wrote: > > I'm not sure these histograms are really useful now. > > > > Let's discuss, but I would lean towards not introducing them now. > > I probably lean towards the same opinion because: > * more histograms now make reviews slower > * we will rethink the histograms after the implementation is done, and these > ones in the new light could appear to be better as part of something other Done. https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:710: base::Bind(&ResourcePrefetchPredictorTables::DeleteResourceData, On 2016/09/26 14:18:23, pasko wrote: > DeletaAllUrls() is hooked into privacy-related cleanup on user request, in which > case it is important to also clean up the redirect data Done. https://codereview.chromium.org/2355273002/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:805: const std::string& redirect_origin_key, On 2016/09/26 14:18:23, pasko wrote: > this has nothing to do with web origin, so the name is confusing, how about: > > key_before_redirects > > ? Done. https://codereview.chromium.org/2355273002/diff/120001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2355273002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:465: // redirect url as a new navigation id and save initial url in navigation. On 2016/09/27 09:55:06, Benoit L wrote: > nit: and save the initial url. Done. https://codereview.chromium.org/2355273002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:770: // URL level data - merge only either we've already saved the data, or it On 2016/09/27 09:55:06, Benoit L wrote: > nit: "merge only if we already saved the data, or it meets the ..." Done. https://codereview.chromium.org/2355273002/diff/120001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://codereview.chromium.org/2355273002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:30: const char kUrlResourceTableName[] = "resource_prefetch_predictor_url_resource"; On 2016/09/27 09:55:06, Benoit L wrote: > Dont'change the name of the existing tables. It's unfortunate since it leads to > less naming consistency, but the database is shared with other predictors. Which > means that we cannot nuke the table when updating the schema. > > This means that your change will lead undead tables behind, since the outdated > tables are removed in DropTablesIfOutdated() with the names you have modified > here. Yeah, I see. And we definitely don't want to have two versions of databases names. Done.
Thanks, a few comments below. https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:663: default: This is un-necessary. The compilers are smart enough to warn us about un-handled cases. See https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:667: default: ditto. https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:686: default: ditto. https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.h:177: sql::Statement* GetTableUpdateStatement(PrefetchKeyType key_type, Why not return a unique_ptr<> right away here? Then there is no need for a comment about ownership, and the call sites wrap these in unique_ptr anyway.
Almost there. Quite a few comments in this pass, but most of them require only small local changes. https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:439: // If we lost the information about the first hop for some reason. In what circumstances does this happen? https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:771: // TODO(alexilin) make only one request to DB thread. TODO(alexilin): https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:178: class GetUrlVisitCountTask : public history::HistoryDBTask { this class is an implementation detail behind resource_prefetch_predictor.h, can you explain why it is moving to the public interface? If you just wanted to move implementation of the methods outside of the class declaration, feel free to do it in the .cc file. Also, seems orthogonal to this change, so easier to review/land with separate CL. https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:312: // |redirect_map| and correspondingly updates the predictor database nit: . https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.proto (right): https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.proto:65: // Represents a mapping of origin to all possible endpoints. hm, this is again confusing because a web origin is a different thing. See: https://tools.ietf.org/html/rfc6454 I think I suggested to call it an origin because I thought we'd do it sooner, but we decided to postpone it. Sorry. Let's say something like: // Represents a mapping from URL or host to a list of redirect endpoints. note: removed 'all' because we actually clean the ones that have too many misses https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.proto:69: optional string url = 1; needs a comment like: // For RedirectData in a host-based table |url| represents the host. https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.proto:75: optional string primary_key = 1; needs a comment like: // Main frame URL or host. https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:648: const char* ResourcePrefetchPredictorTables::GetTableUpdateStatementTemplate( this function and the one below is only called once, and seems having no chance to be called more. It costs an extra lookup when reading this code (even though it is small), so I would prefer to have it inlined into GetTableUpdateStatement(). Not a strict requirement from me, the current state is probably good enough, up to you. https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.h:117: PREFETCH_DATA_TYPE_RESOURCE, there is a new shiny c++11 way to do it: enum class PrefetchDataType { RESOURCE, REDIRECT, METADATA }; Reference them like: PrefetchDataType::RESOURCE. https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.h:181: static const char* GetTableUpdateStatementTemplate( this does not need to be exposed in the interface https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc (right): https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:388: ADD_FAILURE() << r.url(); ADD_FAILURE() is non-fatal, replace with FAIL() << r.url()?
Hi folks, I answered all the comments, you could check this. Changes in code: - Scoped enum introduced instead of unscoped. - kMaxRedirectsPerEntry is thrown out. - A lot of fixed comments. - GetTableUpdateStatement function is more "smart" now. (From pointer point of view). - resource_prefetch_predictor_tables_unittest.cc was rebased. https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:439: // If we lost the information about the first hop for some reason. On 2016/09/27 16:21:27, pasko wrote: > In what circumstances does this happen? See https://codereview.chromium.org/2355273002/diff/40001/chrome/browser/predicto... https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:771: // TODO(alexilin) make only one request to DB thread. On 2016/09/27 16:21:27, pasko wrote: > TODO(alexilin): Done. https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:178: class GetUrlVisitCountTask : public history::HistoryDBTask { On 2016/09/27 16:21:27, pasko wrote: > this class is an implementation detail behind resource_prefetch_predictor.h, can > you explain why it is moving to the public interface? > > If you just wanted to move implementation of the methods outside of the class > declaration, feel free to do it in the .cc file. Also, seems orthogonal to this > change, so easier to review/land with separate CL. This declaration is in private section of ResourcePrefetchPredictor class therefore it isn't a part of the public interface. Or do you want to say that everything that's located in .h file is a part of public interface in terms of C++? Well, why do we need access modifiers then? I moved this implementation detail under the ResourcePrefetchPredictor class because I can no longer access PageRequestSummary inside GetUrlVisitCountTask since I moved PageRequestSummary (former Navigation) into private section. That's why it's in this CL. https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:312: // |redirect_map| and correspondingly updates the predictor database On 2016/09/27 16:21:27, pasko wrote: > nit: . Done. https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.proto (right): https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.proto:65: // Represents a mapping of origin to all possible endpoints. On 2016/09/27 16:21:27, pasko wrote: > hm, this is again confusing because a web origin is a different thing. > > See: https://tools.ietf.org/html/rfc6454 > > I think I suggested to call it an origin because I thought we'd do it sooner, > but we decided to postpone it. Sorry. Let's say something like: > // Represents a mapping from URL or host to a list of redirect endpoints. > > note: removed 'all' because we actually clean the ones that have too many misses Done. https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.proto:69: optional string url = 1; On 2016/09/27 16:21:27, pasko wrote: > needs a comment like: > // For RedirectData in a host-based table |url| represents the host. Done. https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.proto:75: optional string primary_key = 1; On 2016/09/27 16:21:27, pasko wrote: > needs a comment like: > // Main frame URL or host. Done. https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:648: const char* ResourcePrefetchPredictorTables::GetTableUpdateStatementTemplate( On 2016/09/27 16:21:27, pasko wrote: > this function and the one below is only called once, and seems having no chance > to be called more. It costs an extra lookup when reading this code (even though > it is small), so I would prefer to have it inlined into > GetTableUpdateStatement(). > > Not a strict requirement from me, the current state is probably good enough, up > to you. But it allows to use return statements in switch that looks nicer than if we would make assignments in each case clause. Moreover, it makes this method more readable IMHO. I tend to leave it as is. https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:663: default: On 2016/09/27 15:19:34, Benoit L wrote: > This is un-necessary. The compilers are smart enough to warn us about un-handled > cases. > > See > https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements Don't we care about casts from int to enum type? https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:667: default: On 2016/09/27 15:19:34, Benoit L wrote: > ditto. Done. https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:686: default: On 2016/09/27 15:19:34, Benoit L wrote: > ditto. Done. https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.h:117: PREFETCH_DATA_TYPE_RESOURCE, On 2016/09/27 16:21:27, pasko wrote: > there is a new shiny c++11 way to do it: > enum class PrefetchDataType { > RESOURCE, > REDIRECT, > METADATA > }; > > Reference them like: PrefetchDataType::RESOURCE. Done. https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.h:177: sql::Statement* GetTableUpdateStatement(PrefetchKeyType key_type, On 2016/09/27 15:19:34, Benoit L wrote: > Why not return a unique_ptr<> right away here? > Then there is no need for a comment about ownership, and the call sites wrap > these in unique_ptr anyway. Great! Besides, it meets object ownership convention. Done. https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.h:181: static const char* GetTableUpdateStatementTemplate( On 2016/09/27 16:21:27, pasko wrote: > this does not need to be exposed in the interface Do you suggest to move all private static functions out of class declaration? Because there is no difference between them. The same thing: I don't think function that's declared in class private section is "exposed". Convince me if I'm wrong. Of course, if we talk about dependence on the content of .h file, then we don't want to recompile all dependent units after changes in this function. But this is also true for all content of private section. https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc (right): https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:388: ADD_FAILURE() << r.url(); On 2016/09/27 16:21:27, pasko wrote: > ADD_FAILURE() is non-fatal, replace with FAIL() << r.url()? Why it should be fatal? This line means that we've found an element that is presented in rhs but not in lhs.
still lgtm, thank you. https://codereview.chromium.org/2355273002/diff/180001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://codereview.chromium.org/2355273002/diff/180001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:240: if (!hosts.empty()) { nit: no braces. https://codereview.chromium.org/2355273002/diff/180001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:267: DeleteDataHelper(PREFETCH_KEY_TYPE_HOST, PrefetchDataType::REDIRECT, hosts); nit: no braces.
https://codereview.chromium.org/2355273002/diff/180001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://codereview.chromium.org/2355273002/diff/180001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:240: if (!hosts.empty()) { On 2016/09/28 09:19:35, Benoit L wrote: > nit: no braces. Done. https://codereview.chromium.org/2355273002/diff/180001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:267: DeleteDataHelper(PREFETCH_KEY_TYPE_HOST, PrefetchDataType::REDIRECT, hosts); On 2016/09/28 09:19:35, Benoit L wrote: > nit: no braces. Oh, this statement become fit in one line after introducing of scoped enums and cl format did it without my attention. Thanks! Done.
The CQ bit was checked by lizeb@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lizeb@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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by alexilin@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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by alexilin@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: Exceeded global retry quota
The CQ bit was checked by alexilin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lizeb@chromium.org Link to the patchset: https://codereview.chromium.org/2355273002/#ps260001 (title: "Fix compilation complaints.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Redirect handling in the resource_prefetch_predictor. Collect and save information about enpoints in redirect chains that prefetch predictor can start to prefetch subresources before it reaches last redirect hop. Information about redirects is stored in the database keyed by url (or host) of first hop in redirect chain. One row (RedirectData proto message) contains collection of all previously seen urls (or hosts) appearing as endpoints of redirect chain (RedirectStat proto message). This CL depends on https://codereview.chromium.org/2357593002 BUG=631966 ========== to ========== Redirect handling in the resource_prefetch_predictor. Collect and save information about enpoints in redirect chains that prefetch predictor can start to prefetch subresources before it reaches last redirect hop. Information about redirects is stored in the database keyed by url (or host) of first hop in redirect chain. One row (RedirectData proto message) contains collection of all previously seen urls (or hosts) appearing as endpoints of redirect chain (RedirectStat proto message). This CL depends on https://codereview.chromium.org/2357593002 BUG=631966 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Redirect handling in the resource_prefetch_predictor. Collect and save information about enpoints in redirect chains that prefetch predictor can start to prefetch subresources before it reaches last redirect hop. Information about redirects is stored in the database keyed by url (or host) of first hop in redirect chain. One row (RedirectData proto message) contains collection of all previously seen urls (or hosts) appearing as endpoints of redirect chain (RedirectStat proto message). This CL depends on https://codereview.chromium.org/2357593002 BUG=631966 ========== to ========== Redirect handling in the resource_prefetch_predictor. Collect and save information about enpoints in redirect chains that prefetch predictor can start to prefetch subresources before it reaches last redirect hop. Information about redirects is stored in the database keyed by url (or host) of first hop in redirect chain. One row (RedirectData proto message) contains collection of all previously seen urls (or hosts) appearing as endpoints of redirect chain (RedirectStat proto message). This CL depends on https://codereview.chromium.org/2357593002 BUG=631966 Committed: https://crrev.com/43b2f02a9eb2bc50e849d6120caa1de84abcbe46 Cr-Commit-Position: refs/heads/master@{#421520} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/43b2f02a9eb2bc50e849d6120caa1de84abcbe46 Cr-Commit-Position: refs/heads/master@{#421520}
Message was sent while issue was closed.
LGTM https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:178: class GetUrlVisitCountTask : public history::HistoryDBTask { On 2016/09/27 18:11:08, alexilin wrote: > On 2016/09/27 16:21:27, pasko wrote: > > this class is an implementation detail behind resource_prefetch_predictor.h, > can > > you explain why it is moving to the public interface? > > > > If you just wanted to move implementation of the methods outside of the class > > declaration, feel free to do it in the .cc file. Also, seems orthogonal to > this > > change, so easier to review/land with separate CL. > > This declaration is in private section of ResourcePrefetchPredictor class > therefore it isn't a part of the public interface. Or do you want to say that > everything that's located in .h file is a part of public interface in terms of > C++? Well, why do we need access modifiers then? > > I moved this implementation detail under the ResourcePrefetchPredictor class > because I can no longer access PageRequestSummary inside GetUrlVisitCountTask > since I moved PageRequestSummary (former Navigation) into private section. > That's why it's in this CL. Ah. Thank you for this detailed explanation! What I tried to say is that removing implementation details from this header seemed possible, and hence would have been beneficial for reading it and the related classes. Since we need to instantiate a std::map<NavigationID, std::unique_ptr<PageRequestSummary>> at the point of instantiation of ResourcePrefetchPredictor, we would need a complete type of PageRequestSummary at that point. This indeed means that we have to declare the complete type in the header because we have two points of instantiation (factory and test). We could use the factory in the test and move the factory into resource_prefetch_predictor. This latter would probably work, but also would be a large addition to this change, and perhaps will raise its own questions. OK, thanks, I learned a lot :) https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc (right): https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:388: ADD_FAILURE() << r.url(); On 2016/09/27 18:11:09, alexilin wrote: > On 2016/09/27 16:21:27, pasko wrote: > > ADD_FAILURE() is non-fatal, replace with FAIL() << r.url()? > > Why it should be fatal? > This line means that we've found an element that is presented in rhs but not in > lhs. I (shamefully) did not know what a non-fatal failure does, up to now used only fatal failures, which does not work deep in call hierarchy. Good to know, nice, thanks!
Message was sent while issue was closed.
https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2355273002/diff/160001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.h:178: class GetUrlVisitCountTask : public history::HistoryDBTask { On 2016/09/27 18:11:08, alexilin wrote: > On 2016/09/27 16:21:27, pasko wrote: > > this class is an implementation detail behind resource_prefetch_predictor.h, > can > > you explain why it is moving to the public interface? > > > > If you just wanted to move implementation of the methods outside of the class > > declaration, feel free to do it in the .cc file. Also, seems orthogonal to > this > > change, so easier to review/land with separate CL. > > This declaration is in private section of ResourcePrefetchPredictor class > therefore it isn't a part of the public interface. Or do you want to say that > everything that's located in .h file is a part of public interface in terms of > C++? Well, why do we need access modifiers then? > > I moved this implementation detail under the ResourcePrefetchPredictor class > because I can no longer access PageRequestSummary inside GetUrlVisitCountTask > since I moved PageRequestSummary (former Navigation) into private section. > That's why it's in this CL. On the other hand .. URLRequestSummary is public already, so it's not a big shift to make PageRequestSummary public. I think keeping this bloated header smaller has more benefits than making PageRequestSummary private to ResourcePrefetchPredictor. WDYT?
Message was sent while issue was closed.
rnk@chromium.org changed reviewers: + rnk@chromium.org
Message was sent while issue was closed.
Many tests in ResourcePrefetchPredictorTest have been failing on the Clang ToT bots since this change went in: https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%20tester/build... Does the win_clang trybot run tests, or is it compile-only?
Message was sent while issue was closed.
https://codereview.chromium.org/2355273002/diff/260001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor.cc (left): https://codereview.chromium.org/2355273002/diff/260001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor.cc:592: PrefetchDataMap* host_data_ptr = host_data_map.get(); Removing the raw pointer local variables here is what broke the tests. Function call argument evaluation order is not defined. In particular, it's right-to-left on Windows at least, and Clang does better copy elision than MSVC, so Clang evaluates the base::Passed calls below first, and then you end up passing null pointer in the first bind callback. CL incoming. |
