Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2388783002:
predictors: Refactor resource_prefetch_predictor_tables. (Closed)
Descriptionpredictors: Refactor resource_prefetch_predictor_tables.
Replace PrefetchData struct by proto message. It changes sqlite tables
structure from 'single resource - single row' to 'single url/host -
single row'. Also get rid of {Host,Url}MetadataTable because we don't need it
anymore.
It allows to get more uniform database structure and reuse the same functions
for manipulating data.
+ 2 little bug fixes:
- Sort redirect data while reading from the database.
- Check the length of the key in redirect table before adding an entry.
BUG=631966
Committed: https://crrev.com/e3854397b42e53cdc26e9b5237f4f061522a1c45
Cr-Commit-Position: refs/heads/master@{#423168}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Remove duplication. #
Total comments: 34
Patch Set 3 : Fix nits. #
Total comments: 2
Patch Set 4 : Rebase. #Messages
Total messages: 35 (19 generated)
alexilin@chromium.org changed reviewers: + lizeb@chromium.org
Hi Benoit, I've made the refactoring of resource tables as we discussed. I also want to make some renaming (see comment below) and refactor resource_prefetch_predictor_tables.cc to eliminate duplicate code (I wasn't sure that it's good idea to make all changes in one CL). https://codereview.chromium.org/2388783002/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor.proto (right): https://codereview.chromium.org/2388783002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor.proto:22: message PrefetchData { I also would like to rename these messages that resource and redirects message names look more consistent. My proposal: PrefetchData -> ResourceData ResourceData -> ResourceStat
Thanks! Code looks fine, a few high-level comments. I feel there are still too much duplicate code, especially since one aim of the refactor is to expose this duplication. https://codereview.chromium.org/2388783002/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor.proto (right): https://codereview.chromium.org/2388783002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor.proto:23: // Represents information about single subresource. You don't have to nest this message in this one. Can you keep it separate? https://codereview.chromium.org/2388783002/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://codereview.chromium.org/2388783002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:43: const char kCreateResourceTableStatementTemplate[] = Can you remove the duplication here? https://codereview.chromium.org/2388783002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:71: void BindRedirectDataToStatement(const RedirectData& data, This function is now identical to the one above. Can you remove the duplication? https://codereview.chromium.org/2388783002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:100: bool StepAndInitializeRedirectData(Statement* statement, Ditto, can you remove the code duplication? https://codereview.chromium.org/2388783002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:281: std::vector<ResourceData> resources(kv.second.resources().begin(), Is it possible to sort directly the .mutable_resources()? std::sort(kv.second.mutable_resources().begin(), kv.second.mutable_resources().end()) https://codereview.chromium.org/2388783002/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): https://codereview.chromium.org/2388783002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_tables.h:144: static constexpr int kDatabaseVersion = 3; This needs to change.
Hey Benoit, I didn't forgot about duplication in code. I was planning to remove it in next CL. But now I added all changes in this CL. Btw, what about renaming that I proposed? https://codereview.chromium.org/2388783002/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor.proto (right): https://codereview.chromium.org/2388783002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor.proto:23: // Represents information about single subresource. On 2016/10/04 08:22:42, Benoit L wrote: > You don't have to nest this message in this one. > Can you keep it separate? Done. https://codereview.chromium.org/2388783002/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://codereview.chromium.org/2388783002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:43: const char kCreateResourceTableStatementTemplate[] = On 2016/10/04 08:22:42, Benoit L wrote: > Can you remove the duplication here? Done. https://codereview.chromium.org/2388783002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:71: void BindRedirectDataToStatement(const RedirectData& data, On 2016/10/04 08:22:42, Benoit L wrote: > This function is now identical to the one above. Can you remove the duplication? Done. https://codereview.chromium.org/2388783002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:100: bool StepAndInitializeRedirectData(Statement* statement, On 2016/10/04 08:22:42, Benoit L wrote: > Ditto, can you remove the code duplication? Done. https://codereview.chromium.org/2388783002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:281: std::vector<ResourceData> resources(kv.second.resources().begin(), On 2016/10/04 08:22:42, Benoit L wrote: > Is it possible to sort directly the .mutable_resources()? > > std::sort(kv.second.mutable_resources().begin(), > kv.second.mutable_resources().end()) Done. https://codereview.chromium.org/2388783002/diff/1/chrome/browser/predictors/r... File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): https://codereview.chromium.org/2388783002/diff/1/chrome/browser/predictors/r... chrome/browser/predictors/resource_prefetch_predictor_tables.h:144: static constexpr int kDatabaseVersion = 3; On 2016/10/04 08:22:42, Benoit L wrote: > This needs to change. Done. https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:291: SortRedirects(&(kv.second)); I had forgotten to sort redirects while reading. So this is a bug-fix.
Description was changed from
==========
predictors: Refactor resource_prefetch_predictor_tables.
Replace PrefetchData struct by proto message. It changes sqlite tables
structure from 'single resource - single row' to 'single url/host -
single row'. Also get rid of {Host,Url}MetadataTable because we don't need it
anymore.
It allows to get more uniform database structure and reuse the same functions
for manipulating data later.
BUG=631966
==========
to
==========
predictors: Refactor resource_prefetch_predictor_tables.
Replace PrefetchData struct by proto message. It changes sqlite tables
structure from 'single resource - single row' to 'single url/host -
single row'. Also get rid of {Host,Url}MetadataTable because we don't need it
anymore.
It allows to get more uniform database structure and reuse the same functions
for manipulating data.
BUG=631966
==========
lizeb@chromium.org changed reviewers: + pasko@chromium.org
+pasko
A few more comments. Looking at the tests now. https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:817: cache_entry->second.set_primary_key(key); nit: Seems like the code would be a bit easier to read by introducing: PrefetchData& prefetch_data = cache_entry->second; wdyt? https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:841: cache_entry->second.set_last_visit_time( nit: Same here. https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:857: GURL resource_url(resource.resource_url()); tiny nit: the variable resource is used exactly once, and doesn't really improve readability to me. https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.proto (right): https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.proto:21: // Represents information about single subresource. nit: Represents a single subresource. https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.proto:66: // Represents collection of subresources associated with URL or host. nit: Represents a collection of subresources associated with a URL or host. https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.proto:68: // Represents main frame URL of host. nit: Main frame URL or host. https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (left): https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:373: UMA_HISTOGRAM_BOOLEAN("ResourcePrefetchPredictor.DbStringTooLong", true); You should also mark the histogram as deprecated in histograms.xml since you're removing the recording code. https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:377: // Delete the older data from both the tables. Nice! No code is best code. https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:30: const char kMetadataTableName[] = "resource_prefetch_predictor_metadata"; The metadata table names are only used in DropTablesOfOutdated right? If so, please move the constants to this function, and add a comment saying that these are deprecated tables that we still need to remove. https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:268: data_map->insert(std::make_pair(key, data)); Can you re-introduce the DCHECK() you've removed from StepAndInitializeProtoData() (for good reasons) here? https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:287: data_map->insert(std::make_pair(key, data)); ditto. https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:291: SortRedirects(&(kv.second)); On 2016/10/04 09:43:49, alexilin wrote: > I had forgotten to sort redirects while reading. > So this is a bug-fix. Acknowledged. https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.h:87: // Removes too stale resources from |data|. Stale is a bit overloaded. // Removes the resources with more than |max_consecutive_misses| consecutive misses from |data|. https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.h:93: // Removes too stale redirects from |data|. Ditto.
Tests look good :-)
I like the simplification this change introduces! I have not carefully reviewed details, relying on lizeb@ to do it, his comments are looking great! lgtm with nits (that are, in my own (unique?) opinion not mandatory) https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:718: uint64_t oldest_time = UINT64_MAX; nit: max_possible_time ("oldest" suggests epoch to me ..) https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:817: cache_entry->second.set_primary_key(key); On 2016/10/04 15:43:57, Benoit L wrote: > nit: Seems like the code would be a bit easier to read by introducing: > > PrefetchData& prefetch_data = cache_entry->second; > > wdyt? +1 https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.proto (right): https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.proto:16: message Metadata { is Metadata still used?
Description was changed from
==========
predictors: Refactor resource_prefetch_predictor_tables.
Replace PrefetchData struct by proto message. It changes sqlite tables
structure from 'single resource - single row' to 'single url/host -
single row'. Also get rid of {Host,Url}MetadataTable because we don't need it
anymore.
It allows to get more uniform database structure and reuse the same functions
for manipulating data.
BUG=631966
==========
to
==========
predictors: Refactor resource_prefetch_predictor_tables.
Replace PrefetchData struct by proto message. It changes sqlite tables
structure from 'single resource - single row' to 'single url/host -
single row'. Also get rid of {Host,Url}MetadataTable because we don't need it
anymore.
It allows to get more uniform database structure and reuse the same functions
for manipulating data.
+ 2 little bug fixes
- Sort redirect data while reading from the database.
- Check length of the key in the redirect table before adding an entry.
BUG=631966
==========
Description was changed from
==========
predictors: Refactor resource_prefetch_predictor_tables.
Replace PrefetchData struct by proto message. It changes sqlite tables
structure from 'single resource - single row' to 'single url/host -
single row'. Also get rid of {Host,Url}MetadataTable because we don't need it
anymore.
It allows to get more uniform database structure and reuse the same functions
for manipulating data.
+ 2 little bug fixes
- Sort redirect data while reading from the database.
- Check length of the key in the redirect table before adding an entry.
BUG=631966
==========
to
==========
predictors: Refactor resource_prefetch_predictor_tables.
Replace PrefetchData struct by proto message. It changes sqlite tables
structure from 'single resource - single row' to 'single url/host -
single row'. Also get rid of {Host,Url}MetadataTable because we don't need it
anymore.
It allows to get more uniform database structure and reuse the same functions
for manipulating data.
+ 2 little bug fixes:
- Sort redirect data while reading from the database.
- Check the length of the key in redirect table before adding an entry.
BUG=631966
==========
Hi everyone, Answer your comments, fix nits... Also I updated issue description (added information about bugfixes). Could you please suggest me reviewers for histograms and ui? https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:718: uint64_t oldest_time = UINT64_MAX; On 2016/10/04 16:12:16, pasko wrote: > nit: max_possible_time > > ("oldest" suggests epoch to me ..) It's not a constant. And the value UINT64_MAX actually won't ever used (look at the conditions below). But I was forced to initialize this variable to some value by one of the compilers. Actually, value 0 is also suitable but it doesn't look consistent int this context. We are trying to find the entry with the minimum last visit time. Minimum time -> oldest. I could rename it to 'minimum_time' if you wish. https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:817: cache_entry->second.set_primary_key(key); On 2016/10/04 15:43:57, Benoit L wrote: > nit: Seems like the code would be a bit easier to read by introducing: > > PrefetchData& prefetch_data = cache_entry->second; > > wdyt? Yeah, I was thinking about it but it seemed that we would have to introduce this in 3 different scopes. And I didn't like it then. But I tried your suggestion and now it looks better, thanks. https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:841: cache_entry->second.set_last_visit_time( On 2016/10/04 15:43:57, Benoit L wrote: > nit: Same here. Done. https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:857: GURL resource_url(resource.resource_url()); On 2016/10/04 15:43:57, Benoit L wrote: > tiny nit: the variable resource is used exactly once, and doesn't really improve > readability to me. Twice, actually. Second time as a key in old_index map. But it could be replaced by map::insert function and checking its return value. 1 lookup instead of 2 (in DEBUG mode). PROFIT! https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.proto (right): https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.proto:16: message Metadata { On 2016/10/04 16:12:16, pasko wrote: > is Metadata still used? Actually, it was never used. https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.proto:21: // Represents information about single subresource. On 2016/10/04 15:43:57, Benoit L wrote: > nit: Represents a single subresource. Done. https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.proto:66: // Represents collection of subresources associated with URL or host. On 2016/10/04 15:43:57, Benoit L wrote: > nit: Represents a collection of subresources associated with a URL or host. Done. https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.proto:68: // Represents main frame URL of host. On 2016/10/04 15:43:57, Benoit L wrote: > nit: Main frame URL or host. Done. https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (left): https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:373: UMA_HISTOGRAM_BOOLEAN("ResourcePrefetchPredictor.DbStringTooLong", true); On 2016/10/04 15:43:57, Benoit L wrote: > You should also mark the histogram as deprecated in histograms.xml since you're > removing the recording code. Ok. Actually, it looks like this statement became unreachable at some time, because we do all string length checks in ResourcePrefetchPredictor before put a data into the database. https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:377: // Delete the older data from both the tables. On 2016/10/04 15:43:57, Benoit L wrote: > Nice! No code is best code. Done. https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right): https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:30: const char kMetadataTableName[] = "resource_prefetch_predictor_metadata"; On 2016/10/04 15:43:57, Benoit L wrote: > The metadata table names are only used in DropTablesOfOutdated right? > > If so, please move the constants to this function, and add a comment saying that > these are deprecated tables that we still need to remove. Done. https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:268: data_map->insert(std::make_pair(key, data)); On 2016/10/04 15:43:57, Benoit L wrote: > Can you re-introduce the DCHECK() you've removed from > StepAndInitializeProtoData() (for good reasons) here? Done. https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.cc:287: data_map->insert(std::make_pair(key, data)); On 2016/10/04 15:43:57, Benoit L wrote: > ditto. Done. https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right): https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.h:87: // Removes too stale resources from |data|. On 2016/10/04 15:43:57, Benoit L wrote: > Stale is a bit overloaded. > > // Removes the resources with more than |max_consecutive_misses| consecutive > misses from |data|. Done. https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_tables.h:93: // Removes too stale redirects from |data|. On 2016/10/04 15:43:57, Benoit L wrote: > Ditto. Done. https://codereview.chromium.org/2388783002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2388783002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:966: // If the primary key is too long reject it. Another bugfix! (oops!)
LGTM, thanks. https://codereview.chromium.org/2388783002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2388783002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:966: // If the primary key is too long reject it. On 2016/10/04 17:25:46, alexilin wrote: > Another bugfix! (oops!) I'm not 100% sure that we need that, but it's probably wise to keep it.
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: 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 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...
alexilin@chromium.org changed reviewers: + asvitkine@chromium.org, bauerb@chromium.org
bauerb: please review chrome/browser/ui/webui/predictors/predictors_handler.cc. Thanks! asvitkine: please review tools/metrics/histograms/histograms.xml. I marked one histogram as deprecated. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
..forgot to publish one draft https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2388783002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:718: uint64_t oldest_time = UINT64_MAX; On 2016/10/04 17:25:46, alexilin wrote: > On 2016/10/04 16:12:16, pasko wrote: > > nit: max_possible_time > > > > ("oldest" suggests epoch to me ..) > > It's not a constant. And the value UINT64_MAX actually won't ever used (look at > the conditions below). But I was forced to initialize this variable to some > value by one of the compilers. Actually, value 0 is also suitable but it doesn't > look consistent int this context. > We are trying to find the entry with the minimum last visit time. Minimum time > -> oldest. I could rename it to 'minimum_time' if you wish. ah, pardon, my bad, I missed the part that this is only an initial value
WebUI LGTM
lgtm
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by alexilin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org, lizeb@chromium.org Link to the patchset: https://codereview.chromium.org/2388783002/#ps60001 (title: "Rebase.")
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
==========
predictors: Refactor resource_prefetch_predictor_tables.
Replace PrefetchData struct by proto message. It changes sqlite tables
structure from 'single resource - single row' to 'single url/host -
single row'. Also get rid of {Host,Url}MetadataTable because we don't need it
anymore.
It allows to get more uniform database structure and reuse the same functions
for manipulating data.
+ 2 little bug fixes:
- Sort redirect data while reading from the database.
- Check the length of the key in redirect table before adding an entry.
BUG=631966
==========
to
==========
predictors: Refactor resource_prefetch_predictor_tables.
Replace PrefetchData struct by proto message. It changes sqlite tables
structure from 'single resource - single row' to 'single url/host -
single row'. Also get rid of {Host,Url}MetadataTable because we don't need it
anymore.
It allows to get more uniform database structure and reuse the same functions
for manipulating data.
+ 2 little bug fixes:
- Sort redirect data while reading from the database.
- Check the length of the key in redirect table before adding an entry.
BUG=631966
==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from
==========
predictors: Refactor resource_prefetch_predictor_tables.
Replace PrefetchData struct by proto message. It changes sqlite tables
structure from 'single resource - single row' to 'single url/host -
single row'. Also get rid of {Host,Url}MetadataTable because we don't need it
anymore.
It allows to get more uniform database structure and reuse the same functions
for manipulating data.
+ 2 little bug fixes:
- Sort redirect data while reading from the database.
- Check the length of the key in redirect table before adding an entry.
BUG=631966
==========
to
==========
predictors: Refactor resource_prefetch_predictor_tables.
Replace PrefetchData struct by proto message. It changes sqlite tables
structure from 'single resource - single row' to 'single url/host -
single row'. Also get rid of {Host,Url}MetadataTable because we don't need it
anymore.
It allows to get more uniform database structure and reuse the same functions
for manipulating data.
+ 2 little bug fixes:
- Sort redirect data while reading from the database.
- Check the length of the key in redirect table before adding an entry.
BUG=631966
Committed: https://crrev.com/e3854397b42e53cdc26e9b5237f4f061522a1c45
Cr-Commit-Position: refs/heads/master@{#423168}
==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e3854397b42e53cdc26e9b5237f4f061522a1c45 Cr-Commit-Position: refs/heads/master@{#423168} |
