Refactor the resource_prefetch_predictor.

Refactor the resource_prefetch_predictor.

This removes ResourceRow object and uses proto message ResourceData instead of
it. It allows to get rid of the boilerplate conversion from the DB to an
internal structure.

Plus adds lost ResourceData::RESOURCE_TYPE_FONT_RESOURCE case in
chrome/browser/ui/webui/predictors/predictors_handler.cc

BUG=631966
Committed: https://crrev.com/68ebcad9d6ab7491ee1f2f5c5a28e7eac1c4987f
Cr-Commit-Position: refs/heads/master@{#420935}

[alexilin, 2016-09-20 11:11:53]

alexilin@chromium.org changed reviewers:
+ lizeb@chromium.org

[alexilin, 2016-09-20 11:11:53]

Hi Benoit,

I've finished with refactoring of ResourcePrefetchPredictor. 

I'm not sure about the way I solved problem with score storage. 

At first, I thought about to add score field in proto declaration and always
clean it before saving in database. As it turned out, we pass ResourceData
objects by constant reference (because
ResourcePrefetchPredictorTables::UpdateData takes constant reference and this is
right). So I would have to copy of ResourceData every time, when I'm trying to
keep it in database only to clean score field. Of course, it is possible to use
const_cast but it doesn't look right.

So afterwards I noticed, we use score only when we need to sort resources. So I
don't even store scores together with ResourceData and calculate them only in
sort function. But I'm not sure that nobody else will want to use it in the
future.

[Benoit L, 2016-09-20 11:34:16]

On 2016/09/20 11:11:53, alexilin wrote:
> Hi Benoit,
> 
> I've finished with refactoring of ResourcePrefetchPredictor. 
> 
> I'm not sure about the way I solved problem with score storage. 
> 
> At first, I thought about to add score field in proto declaration and always
> clean it before saving in database. As it turned out, we pass ResourceData
> objects by constant reference (because
> ResourcePrefetchPredictorTables::UpdateData takes constant reference and this
is
> right). So I would have to copy of ResourceData every time, when I'm trying to
> keep it in database only to clean score field. Of course, it is possible to
use
> const_cast but it doesn't look right.
> 
> So afterwards I noticed, we use score only when we need to sort resources. So
I
> don't even store scores together with ResourceData and calculate them only in
> sort function. But I'm not sure that nobody else will want to use it in the
> future.

Thanks!

Just one question before I start reviewing this in more details: is this mostly
a rebase of https://codereview.chromium.org/2268283005/ ?
If not, can you quickly sum up the differences?

[alexilin, 2016-09-20 11:40:09]

On 2016/09/20 11:34:16, Benoit L wrote:
> On 2016/09/20 11:11:53, alexilin wrote:
> > Hi Benoit,
> > 
> > I've finished with refactoring of ResourcePrefetchPredictor. 
> > 
> > I'm not sure about the way I solved problem with score storage. 
> > 
> > At first, I thought about to add score field in proto declaration and always
> > clean it before saving in database. As it turned out, we pass ResourceData
> > objects by constant reference (because
> > ResourcePrefetchPredictorTables::UpdateData takes constant reference and
this
> is
> > right). So I would have to copy of ResourceData every time, when I'm trying
to
> > keep it in database only to clean score field. Of course, it is possible to
> use
> > const_cast but it doesn't look right.
> > 
> > So afterwards I noticed, we use score only when we need to sort resources.
So
> I
> > don't even store scores together with ResourceData and calculate them only
in
> > sort function. But I'm not sure that nobody else will want to use it in the
> > future.
> 
> Thanks!
> 
> Just one question before I start reviewing this in more details: is this
mostly
> a rebase of https://codereview.chromium.org/2268283005/ ?
> If not, can you quickly sum up the differences?

Almost yes. The main difference is the way of handling score of ResourceData.
There are also some minor differences (e.g. namespacing) but generally this is 
the only change.

[Benoit L, 2016-09-20 12:19:00]

The CQ bit was checked by lizeb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 12:19:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Benoit L, 2016-09-20 12:33:27]

lgtm, thanks.

Since I'm not an owner of this directory, and that this is mostly a self
high-five, please add pasko@ in the reviewer list.

https://codereview.chromium.org/2357593002/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right):

https://codereview.chromium.org/2357593002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_tables.h:58: std::string
primary_key;  // is_host() ? main frame url : host.
Looking at this, I believe that this comment is incorrect. (the clauses should
be flipped). Can you change this as well?

[alexilin, 2016-09-20 13:04:01]

alexilin@chromium.org changed reviewers:
+ pasko@chromium.org

[alexilin, 2016-09-20 13:04:01]

alexilin@chromium.org changed required reviewers:
+ pasko@chromium.org

[alexilin, 2016-09-20 13:04:02]

Hi Egor,

I resurrected refactoring was proposed in
https://codereview.chromium.org/2268283005.
We need it to manage databases in a uniform manner later, when we'll add
redirects table.
Look at this CL (https://codereview.chromium.org/2321343002/) to familiarize
with the problem. ResourcePrefetchPredictorTables class too swelled.

[Benoit L, 2016-09-21 08:10:22]

The CQ bit was checked by lizeb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 08:10:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 09:13:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 09:13:19]

Dry run: This issue passed the CQ dry run.

[pasko, 2016-09-21 17:38:37]

overall looks good, thank you, asking only for a few small changes

https://codereview.chromium.org/2357593002/diff/40001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right):

https://codereview.chromium.org/2357593002/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:106: // Sort
indices instead of ResourceData objects and then apply resulting
Why not just this:

std::sort(resources->begin(), resources->end(), [](const ResourceData& x, const
ResourceData& y) { return ComputeScore(x) > ComputeScore(y); });

?

Performance wins of this are not obvious:
1. vector<ResourceData> is going to be <100 entries
2. probably mostly sorted already, so there is not many swaps
3. creating new vectors will create heap allocations

I think your implementation will be faster, and heap fragmentation is hard to
evaluate, so I am OK with it. I would still slightly prefer shorter code until
we prove that extra copying is actually a performance bottleneck.

https://codereview.chromium.org/2357593002/diff/40001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right):

https://codereview.chromium.org/2357593002/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.h:116: // Computes
score of |data|
Except for end-of-line case, all comments should be written as complete
sentences, including a period at the end.

See:
https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G...

Please do it in all comments you added/modified in this change.

https://codereview.chromium.org/2357593002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/predictors/predictors_handler.cc (right):

https://codereview.chromium.org/2357593002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/predictors/predictors_handler.cc:38: case
ResourceData::RESOURCE_TYPE_FONT_RESOURCE:
this seems like a bigfix (yay!)

But then the description of the change is not fully correct by saying that it is
refactoring. Feel free to mention it in the description or sending this as a
separate change.

[alexilin, 2016-09-22 11:17:19]

Description was changed from

==========
Refactor the resource_prefetch_predictor.

This removes ResourceRow object and uses proto message ResourceData instead of
it. It allows to get rid of the boilerplate conversion from the DB to an
internal structure.

BUG=631966
==========

to

==========
Refactor the resource_prefetch_predictor.

This removes ResourceRow object and uses proto message ResourceData instead of
it. It allows to get rid of the boilerplate conversion from the DB to an
internal structure.

Plus adds lost ResourceData::RESOURCE_TYPE_FONT_RESOURCE case in
chrome/browser/ui/webui/predictors/predictors_handler.cc

BUG=631966
==========

[alexilin, 2016-09-22 11:17:33]

Description was changed from

==========
Refactor the resource_prefetch_predictor.

This removes ResourceRow object and uses proto message ResourceData instead of
it. It allows to get rid of the boilerplate conversion from the DB to an
internal structure.

Plus adds lost ResourceData::RESOURCE_TYPE_FONT_RESOURCE case in
chrome/browser/ui/webui/predictors/predictors_handler.cc

BUG=631966
==========

to

==========
Refactor the resource_prefetch_predictor.

This removes ResourceRow object and uses proto message ResourceData instead of
it. It allows to get rid of the boilerplate conversion from the DB to an
internal structure.

Plus adds lost ResourceData::RESOURCE_TYPE_FONT_RESOURCE case in
chrome/browser/ui/webui/predictors/predictors_handler.cc

BUG=631966
==========

[alexilin, 2016-09-22 11:21:16]

Hello Egor,

I worked on yours comments.
Waiting for your approval.

https://codereview.chromium.org/2357593002/diff/40001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_tables.cc (right):

https://codereview.chromium.org/2357593002/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:106: // Sort
indices instead of ResourceData objects and then apply resulting
On 2016/09/21 17:38:36, pasko wrote:
> Why not just this:
> 
> std::sort(resources->begin(), resources->end(), [](const ResourceData& x,
const
> ResourceData& y) { return ComputeScore(x) > ComputeScore(y); });
> 
> ?
> 
> Performance wins of this are not obvious:
> 1. vector<ResourceData> is going to be <100 entries
> 2. probably mostly sorted already, so there is not many swaps
> 3. creating new vectors will create heap allocations
> 
> I think your implementation will be faster, and heap fragmentation is hard to
> evaluate, so I am OK with it. I would still slightly prefer shorter code until
> we prove that extra copying is actually a performance bottleneck.

Yeah, I agree. I simply overlooked this more convenient way.

https://codereview.chromium.org/2357593002/diff/40001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_tables.h (right):

https://codereview.chromium.org/2357593002/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.h:116: // Computes
score of |data|
On 2016/09/21 17:38:36, pasko wrote:
> Except for end-of-line case, all comments should be written as complete
> sentences, including a period at the end.
> 
> See:
>
https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G...
> 
> Please do it in all comments you added/modified in this change.

Feel free to leave more grammar-nazi comments!

[alexilin, 2016-09-22 11:22:53]

The CQ bit was checked by alexilin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 11:23:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pasko, 2016-09-22 12:01:02]

lgtm, thank you, Alexandr

[alexilin, 2016-09-22 12:01:39]

The CQ bit was unchecked by alexilin@chromium.org

[alexilin, 2016-09-22 12:01:40]

The CQ bit was checked by alexilin@chromium.org

[alexilin, 2016-09-22 12:01:40]

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/2357593002/#ps60001
(title: "Simplify sorting")

[commit-bot: I haz the power, 2016-09-22 12:01:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 12:10:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-22 12:10:40]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[alexilin, 2016-09-22 12:22:32]

alexilin@chromium.org changed reviewers:
+ pkasting@chromium.org

[alexilin, 2016-09-22 12:22:32]

alexilin@chromium.org changed required reviewers:
+ pkasting@chromium.org
- pasko@chromium.org

[alexilin, 2016-09-22 12:22:33]

pkasting@chromium.org: Please review changes in
chrome/browser/ui/webui/predictors/predictors_handler.cc

[Peter Kasting, 2016-09-22 23:42:09]

LGTM

https://codereview.chromium.org/2357593002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/predictors/predictors_handler.cc (right):

https://codereview.chromium.org/2357593002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/predictors/predictors_handler.cc:28: using ResourceType
= ResourceData::ResourceType;
Nit: Avoid these.  They don't significantly improve readability and they cost
LOC.

https://codereview.chromium.org/2357593002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/predictors/predictors_handler.cc:118: for (const auto& p
: data_map) {
Good small cleanups!  Range-based for FTW.

[alexilin, 2016-09-23 08:45:57]

The CQ bit was checked by alexilin@chromium.org

[alexilin, 2016-09-23 08:45:58]

The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org,
lizeb@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2357593002/#ps80001
(title: "Nit fix")

[commit-bot: I haz the power, 2016-09-23 08:46:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 08:48:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 08:48:47]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alexilin, 2016-09-23 09:25:33]

The CQ bit was checked by alexilin@chromium.org

[alexilin, 2016-09-23 09:25:34]

The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org,
lizeb@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2357593002/#ps100001
(title: ".")

[commit-bot: I haz the power, 2016-09-23 09:25:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 09:36:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 09:36:57]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[alexilin, 2016-09-23 13:30:29]

The CQ bit was checked by alexilin@chromium.org

[commit-bot: I haz the power, 2016-09-23 13:30:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 13:46:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 13:46:58]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[alexilin, 2016-09-23 15:17:49]

The CQ bit was checked by alexilin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 15:18:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 15:32:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-23 15:32:56]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[alexilin, 2016-09-26 08:15:00]

The CQ bit was checked by alexilin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 08:15:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 09:32:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 09:32:15]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[alexilin, 2016-09-26 11:16:26]

The CQ bit was checked by alexilin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 11:16:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 12:30:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 12:30:07]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[alexilin, 2016-09-26 16:12:18]

The CQ bit was checked by alexilin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 16:12:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 17:34:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 17:34:12]

Dry run: This issue passed the CQ dry run.

[alexilin, 2016-09-26 18:07:39]

The CQ bit was checked by alexilin@chromium.org

[commit-bot: I haz the power, 2016-09-26 18:08:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 18:16:34]

Description was changed from

==========
Refactor the resource_prefetch_predictor.

This removes ResourceRow object and uses proto message ResourceData instead of
it. It allows to get rid of the boilerplate conversion from the DB to an
internal structure.

Plus adds lost ResourceData::RESOURCE_TYPE_FONT_RESOURCE case in
chrome/browser/ui/webui/predictors/predictors_handler.cc

BUG=631966
==========

to

==========
Refactor the resource_prefetch_predictor.

This removes ResourceRow object and uses proto message ResourceData instead of
it. It allows to get rid of the boilerplate conversion from the DB to an
internal structure.

Plus adds lost ResourceData::RESOURCE_TYPE_FONT_RESOURCE case in
chrome/browser/ui/webui/predictors/predictors_handler.cc

BUG=631966
==========

[commit-bot: I haz the power, 2016-09-26 18:16:36]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-26 18:18:12]

Description was changed from

==========
Refactor the resource_prefetch_predictor.

This removes ResourceRow object and uses proto message ResourceData instead of
it. It allows to get rid of the boilerplate conversion from the DB to an
internal structure.

Plus adds lost ResourceData::RESOURCE_TYPE_FONT_RESOURCE case in
chrome/browser/ui/webui/predictors/predictors_handler.cc

BUG=631966
==========

to

==========
Refactor the resource_prefetch_predictor.

This removes ResourceRow object and uses proto message ResourceData instead of
it. It allows to get rid of the boilerplate conversion from the DB to an
internal structure.

Plus adds lost ResourceData::RESOURCE_TYPE_FONT_RESOURCE case in
chrome/browser/ui/webui/predictors/predictors_handler.cc

BUG=631966

Committed: https://crrev.com/68ebcad9d6ab7491ee1f2f5c5a28e7eac1c4987f
Cr-Commit-Position: refs/heads/master@{#420935}
==========

[commit-bot: I haz the power, 2016-09-26 18:18:13]

Patchset 6 (id:??) landed as
https://crrev.com/68ebcad9d6ab7491ee1f2f5c5a28e7eac1c4987f
Cr-Commit-Position: refs/heads/master@{#420935}