predictors: Extract sql key-value tables into separate class.

predictors: Extract sql key-value tables into separate class.

This is the first step of Glowplug Database refactoring. As a result we should
get rid of boilerplate related to adding/removing tables from Glowplug.
The next step will be providing users the direct access to tables instead of
using Update{Resource,Redirect,etc.}Data wrappers.
The code uses templates instead of google::protobuf::MessageLite for the
type-safety.

BUG=715525
Review-Url: https://codereview.chromium.org/2851473002
Cr-Commit-Position: refs/heads/master@{#468046}
Committed: https://chromium.googlesource.com/chromium/src/+/6175b0a9f10d288f663f88c871c88ca3b5f0b9e8

[alexilin, 2017-04-27 12:04:09]

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

[alexilin, 2017-04-27 12:04:10]

Hi Benoit! Refactoring is coming!

[Benoit L, 2017-04-27 15:49:03]

Yeah templates! Yeah less code!

https://codereview.chromium.org/2851473002/diff/1/chrome/browser/predictors/g...
File chrome/browser/predictors/glowplug_key_value_table.h (right):

https://codereview.chromium.org/2851473002/diff/1/chrome/browser/predictors/g...
chrome/browser/predictors/glowplug_key_value_table.h:21: void
BindDataToStatement(const std::string& key,
Does this need to be a template?

https://codereview.chromium.org/2851473002/diff/1/chrome/browser/predictors/g...
chrome/browser/predictors/glowplug_key_value_table.h:39: virtual void
GetAllData(std::map<std::string, T>* data_map) const;
nit: Why virtual?

https://codereview.chromium.org/2851473002/diff/1/chrome/browser/predictors/g...
chrome/browser/predictors/glowplug_key_value_table.h:45: std::string
GetSelectAllSql() const {
Can these methods not be templates?
I mean, can they be outside the template class?

https://codereview.chromium.org/2851473002/diff/1/chrome/browser/predictors/g...
chrome/browser/predictors/glowplug_key_value_table.h:89: // Delete the older
data from the table.
Thinking about it, why do we delete and then insert?
Couldn't we use "INSERT OR REPLACE"?

Then we don't even need a transaction I believe.

https://codereview.chromium.org/2851473002/diff/1/chrome/browser/predictors/g...
chrome/browser/predictors/glowplug_key_value_table.h:111: sql::Statement
deleter(db_->GetUniqueStatement(GetDeleteSql().c_str()));
nit: put the statement outside the loop? Otherwise we do N unnecessary SQL
calls.

We could even cache them in the DB class if we think creating them is expensive.
It can be on complex DBs, don't think it should be too bad with SQLite though.

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

https://codereview.chromium.org/2851473002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_tables.h:174:
std::unique_ptr<sql::Statement> GetTableUpdateStatement(
We lose the cached statements, does it matter?

[alexilin, 2017-04-28 14:54:52]

https://codereview.chromium.org/2851473002/diff/1/chrome/browser/predictors/g...
File chrome/browser/predictors/glowplug_key_value_table.h (right):

https://codereview.chromium.org/2851473002/diff/1/chrome/browser/predictors/g...
chrome/browser/predictors/glowplug_key_value_table.h:21: void
BindDataToStatement(const std::string& key,
On 2017/04/27 15:49:02, Benoit L wrote:
> Does this need to be a template?

Alternatives? I could use protobuf::MessageLite instead of T here. Does it sound
good?
Other way is just to inline the code of this function because it's called only
once.

https://codereview.chromium.org/2851473002/diff/1/chrome/browser/predictors/g...
chrome/browser/predictors/glowplug_key_value_table.h:39: virtual void
GetAllData(std::map<std::string, T>* data_map) const;
On 2017/04/27 15:49:02, Benoit L wrote:
> nit: Why virtual?

Eventually, for testing.
Added a comment, thanks!

https://codereview.chromium.org/2851473002/diff/1/chrome/browser/predictors/g...
chrome/browser/predictors/glowplug_key_value_table.h:45: std::string
GetSelectAllSql() const {
On 2017/04/27 15:49:02, Benoit L wrote:
> Can these methods not be templates?
> I mean, can they be outside the template class?

Then I have to move the implementation of these methods to .cc file.
Done.

https://codereview.chromium.org/2851473002/diff/1/chrome/browser/predictors/g...
chrome/browser/predictors/glowplug_key_value_table.h:111: sql::Statement
deleter(db_->GetUniqueStatement(GetDeleteSql().c_str()));
On 2017/04/27 15:49:03, Benoit L wrote:
> nit: put the statement outside the loop? Otherwise we do N unnecessary SQL
> calls.
> 
> We could even cache them in the DB class if we think creating them is
expensive.
> It can be on complex DBs, don't think it should be too bad with SQLite though.

Put the statement outside the loop - done.

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

https://codereview.chromium.org/2851473002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_tables.h:174:
std::unique_ptr<sql::Statement> GetTableUpdateStatement(
On 2017/04/27 15:49:03, Benoit L wrote:
> We lose the cached statements, does it matter?

I wrote a little benchmark that creates sql::Statement 10000 times in a loop:
* Using GetCachedStatement: 0.154616 s
* Using GetUniqueStatement: 0.776918 s
* Cache the result of GetUniqueStatement manually: 0.10126 s

So the most efficient way would be to cache the statements inside the class.
Don't know whether this behavior supported by sql library.
I mean, that's how GetCachedStatement function works, it just saves result of
GetUniqueStatement in a map. But implementation details could be changed.

Using of GetCachedStatement requires passing some unique identifier inside of
GlowplugKeyValueTable class, that doesn't look great.

WDYT?

[Benoit L, 2017-04-28 15:12:35]

lgtm

https://codereview.chromium.org/2851473002/diff/1/chrome/browser/predictors/g...
File chrome/browser/predictors/glowplug_key_value_table.h (right):

https://codereview.chromium.org/2851473002/diff/1/chrome/browser/predictors/g...
chrome/browser/predictors/glowplug_key_value_table.h:21: void
BindDataToStatement(const std::string& key,
On 2017/04/28 14:54:52, alexilin wrote:
> On 2017/04/27 15:49:02, Benoit L wrote:
> > Does this need to be a template?
> 
> Alternatives? I could use protobuf::MessageLite instead of T here. Does it
sound
> good?
> Other way is just to inline the code of this function because it's called only
> once.

Thanks, that's better this way. The function is indeed called only once, but
since it's from a template, that adds up.

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

https://codereview.chromium.org/2851473002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_tables.h:174:
std::unique_ptr<sql::Statement> GetTableUpdateStatement(
On 2017/04/28 14:54:52, alexilin wrote:
> On 2017/04/27 15:49:03, Benoit L wrote:
> > We lose the cached statements, does it matter?
> 
> I wrote a little benchmark that creates sql::Statement 10000 times in a loop:
> * Using GetCachedStatement: 0.154616 s
> * Using GetUniqueStatement: 0.776918 s
> * Cache the result of GetUniqueStatement manually: 0.10126 s
> 
> So the most efficient way would be to cache the statements inside the class.
> Don't know whether this behavior supported by sql library.
> I mean, that's how GetCachedStatement function works, it just saves result of
> GetUniqueStatement in a map. But implementation details could be changed.
> 
> Using of GetCachedStatement requires passing some unique identifier inside of
> GlowplugKeyValueTable class, that doesn't look great.
> 
> WDYT?

Re-using a unique statement is OK I believe, as it's explicitely OK to bind a
statement to different values.
Cached statement looks heavyweight indeed, what about a lazily initialized
statement?

https://codereview.chromium.org/2851473002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/glowplug_key_value_table.h (right):

https://codereview.chromium.org/2851473002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/glowplug_key_value_table.h:64: auto it =
data_map->emplace(reader.ColumnString(0), T()).first;
That's quick, this was allowed only 4 days ago :-)

[Benoit L, 2017-04-28 15:12:36]

lgtm

[alexilin, 2017-04-28 16:27:26]

https://codereview.chromium.org/2851473002/diff/1/chrome/browser/predictors/g...
File chrome/browser/predictors/glowplug_key_value_table.h (right):

https://codereview.chromium.org/2851473002/diff/1/chrome/browser/predictors/g...
chrome/browser/predictors/glowplug_key_value_table.h:89: // Delete the older
data from the table.
On 2017/04/27 15:49:02, Benoit L wrote:
> Thinking about it, why do we delete and then insert?
> Couldn't we use "INSERT OR REPLACE"?
> 
> Then we don't even need a transaction I believe.

Done.

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

https://codereview.chromium.org/2851473002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_tables.h:174:
std::unique_ptr<sql::Statement> GetTableUpdateStatement(
On 2017/04/28 15:12:34, Benoit L wrote:
> On 2017/04/28 14:54:52, alexilin wrote:
> > On 2017/04/27 15:49:03, Benoit L wrote:
> > > We lose the cached statements, does it matter?
> > 
> > I wrote a little benchmark that creates sql::Statement 10000 times in a
loop:
> > * Using GetCachedStatement: 0.154616 s
> > * Using GetUniqueStatement: 0.776918 s
> > * Cache the result of GetUniqueStatement manually: 0.10126 s
> > 
> > So the most efficient way would be to cache the statements inside the class.
> > Don't know whether this behavior supported by sql library.
> > I mean, that's how GetCachedStatement function works, it just saves result
of
> > GetUniqueStatement in a map. But implementation details could be changed.
> > 
> > Using of GetCachedStatement requires passing some unique identifier inside
of
> > GlowplugKeyValueTable class, that doesn't look great.
> > 
> > WDYT?
> 
> Re-using a unique statement is OK I believe, as it's explicitely OK to bind a
> statement to different values.
> Cached statement looks heavyweight indeed, what about a lazily initialized
> statement?

It turned out that we can't store cached statements inside of the class because
the StatementRef is defined in the private section of sql::Connection.

https://codereview.chromium.org/2851473002/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/glowplug_key_value_table.h (right):

https://codereview.chromium.org/2851473002/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/glowplug_key_value_table.h:64: auto it =
data_map->emplace(reader.ColumnString(0), T()).first;
On 2017/04/28 15:12:34, Benoit L wrote:
> That's quick, this was allowed only 4 days ago :-)

It's a pleasure to be the one of the first users. :)

[alexilin, 2017-04-28 16:27:30]

The CQ bit was checked by alexilin@chromium.org

[alexilin, 2017-04-28 16:27:30]

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/2851473002/#ps60001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-04-28 16:28:11]

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, 2017-04-28 17:34:43]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493396850324270,
"parent_rev": "9d6b6c40b933564260a0a70567b90ac30656222a", "commit_rev":
"6175b0a9f10d288f663f88c871c88ca3b5f0b9e8"}

[commit-bot: I haz the power, 2017-04-28 17:34:53]

Description was changed from

==========
predictors: Extract sql key-value tables into separate class.

This is the first step of Glowplug Database refactoring. As a result we should
get rid of boilerplate related to adding/removing tables from Glowplug.
The next step will be providing users the direct access to tables instead of
using Update{Resource,Redirect,etc.}Data wrappers.
The code uses templates instead of google::protobuf::MessageLite for the
type-safety.

BUG=715525
==========

to

==========
predictors: Extract sql key-value tables into separate class.

This is the first step of Glowplug Database refactoring. As a result we should
get rid of boilerplate related to adding/removing tables from Glowplug.
The next step will be providing users the direct access to tables instead of
using Update{Resource,Redirect,etc.}Data wrappers.
The code uses templates instead of google::protobuf::MessageLite for the
type-safety.

BUG=715525

Review-Url: https://codereview.chromium.org/2851473002
Cr-Commit-Position: refs/heads/master@{#468046}
Committed:
https://chromium.googlesource.com/chromium/src/+/6175b0a9f10d288f663f88c871c8...
==========

[commit-bot: I haz the power, 2017-04-28 17:34:55]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/6175b0a9f10d288f663f88c871c8...