Report downloaded resources at most once

Report downloaded resources at most once

Before, we were reporting used_resources and unused_resources. used_resources was reported only once when the resource is used but unused_resources was reported on every single prefetch session for as long as the resource was still unused.

This change removes unused_resources and adds downloaded resources (with URL parameter d), which reports the "cost" of a resource once. This allows to have a meaningful ratio of used_resources / downloaded_resources.

BUG=675732
Committed: https://crrev.com/02bf6c04e45ee64670eb9e6cd3a05c54d9a2eaa0
Cr-Commit-Position: refs/heads/master@{#440176}

[jamartin, 2016-12-19 21:49:30]

Description was changed from

==========
Report downloaded resources at most once

BUG=
==========

to

==========
Report downloaded resources at most once

BUG=675732
==========

[jamartin, 2016-12-19 22:07:31]

jamartin@chromium.org changed reviewers:
+ twifkak@chromium.org

[jamartin, 2016-12-19 22:07:32]

Please take a look at the TODO(jamartin) in precache_fetcher_unittest.cc

[jamartin, 2016-12-20 00:05:23]

On 2016/12/19 22:07:32, jamartin wrote:
> Please take a look at the TODO(jamartin) in precache_fetcher_unittest.cc

Fixed. PTAL.

[jamartin, 2016-12-20 00:05:46]

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

[commit-bot: I haz the power, 2016-12-20 00:06:20]

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-12-20 00:46:55]

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

[commit-bot: I haz the power, 2016-12-20 00:46:56]

Dry run: This issue passed the CQ dry run.

[twifkak, 2016-12-20 01:42:00]

https://codereview.chromium.org/2586813004/diff/20001/components/precache/cor...
File components/precache/core/precache_database_unittest.cc (right):

https://codereview.chromium.org/2586813004/diff/20001/components/precache/cor...
components/precache/core/precache_database_unittest.cc:559:
CHECK_NE(PrecacheReferrerHostEntry::kInvalidId, id);
Per the last bullet of
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...,
use ASSERT_NE instead.

https://codereview.chromium.org/2586813004/diff/20001/components/precache/cor...
components/precache/core/precache_database_unittest.cc:560:
precache_database_->GetURLListForReferrerHost(id, &unused_a, &unused_b);
I assume the purpose of this call is to mark the already_reported resources as
is_download_reported = 1. Leave a comment to that effect.

https://codereview.chromium.org/2586813004/diff/20001/components/precache/cor...
File components/precache/core/precache_url_table.cc (right):

https://codereview.chromium.org/2586813004/diff/20001/components/precache/cor...
components/precache/core/precache_url_table.cc:50: statement->BindString(3,
GetKey(url));
Make the order of the bindvars and the order of the function params the same.

https://codereview.chromium.org/2586813004/diff/20001/components/precache/cor...
components/precache/core/precache_url_table.cc:60: if (is_precached &&
!was_cached) {
Note that "is_precached" here is a misnomer, since RecordURLPrefetch passes a
value of `!was_cached || precache_url_table_.GetURLInfo(url).is_precached`.
Since (A && B) || A == A, you can simplify your if statement to:

if (!was_cached) {

https://codereview.chromium.org/2586813004/diff/20001/components/precache/cor...
components/precache/core/precache_url_table.cc:82: if (!BindStatementAndRun(url,
referrer_host_id, is_precached, precache_time,
I'd rather move this logic out of PrecacheURLTable and into PrecacheDatabase,
for two reasons:

1. The only caller of AddURL is PrecacheDatabase::RecordURLPrefetch{,Internal},
which already has some confusing boolean logic. Spreading it across
caller/callee makes it unnecessarily more confusing. I think the intent of this
PrecacheURLTable class is to be as dumb as possible, and let the logic be in
PrecacheDatabase.

2. I'm not a fan of relying on an error condition (for a few reasons [*]), and
you should be able to avoid it by adding is_download_reported to PrecacheURLInfo
and moving this logic into RecordURLPrefetch, which already does a SELECT via
its GetURLInfo call.

That would change your FSM comment at the bottom to be
`AddUrl(!is_download_reported)`, which I guess is not entirely satisfying but
overall I think it'd be an improvement in readability. WDYT?

---

[*] a. As you state, it relies on some outside configuration, which is brittle
-- maybe they want to change the error_callback setting in the future, and will
have to contend with this code.

b. Maybe it's slower? I have no basis for this, other than many languages have
slow exceptions, but that's generally because they have to generate a backtrace.

c. (nit) What if it errors for some other reason than the "OR ABORT"? Per
https://sqlite.org/lang_conflict.html the abort happens if there's an
"applicable constraint violation", but maybe there's some other transient I/O
error or something. In this case, the result will be "It does a zero-row update"
which is equivalent to what would have happened anyway if the INSERT failed, so
this is just academic.

https://codereview.chromium.org/2586813004/diff/20001/components/precache/cor...
components/precache/core/precache_url_table.cc:146: if
(!downloaded_urls->empty()) {
Likewise, consider moving this block into a separate function and calling it
directly from PrecacheDatabase.

https://codereview.chromium.org/2586813004/diff/20001/components/precache/cor...
File components/precache/core/precache_url_table.h (right):

https://codereview.chromium.org/2586813004/diff/20001/components/precache/cor...
components/precache/core/precache_url_table.h:15: #include "sql/statement.h"
If this is for sql::Statement, just fwd declare:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

https://codereview.chromium.org/2586813004/diff/20001/components/precache/cor...
components/precache/core/precache_url_table.h:59: // precached, |time| is the
timestamp. Replaces the row if one already exists.
Update doc.

https://codereview.chromium.org/2586813004/diff/20001/components/precache/cor...
components/precache/core/precache_url_table.h:75: // Populates the used and
unused resource URLs for the referrer host with id
s/unused/downloaded

[twifkak, 2016-12-20 01:44:34]

Oh, also, please add detail to the commit description.

[jamartin, 2016-12-20 21:09:24]

Description was changed from

==========
Report downloaded resources at most once

BUG=675732
==========

to

==========
Report downloaded resources at most once

Before, we were reporting used_resources and unused_resources. used_resources
was reported only once when the resource is used but unused_resources was
reported on every single prefetch session for as long as the resource was still
unused.

This change removes unused_resources and adds downloaded resources (with URL
parameter d), which reports the "cost" of a resource once. This allows to have a
meaningful ratio of used_resources / downloaded_resources.

BUG=675732
==========

[jamartin, 2016-12-20 21:09:35]

PTAL.

https://codereview.chromium.org/2586813004/diff/20001/components/precache/cor...
File components/precache/core/precache_database_unittest.cc (right):

https://codereview.chromium.org/2586813004/diff/20001/components/precache/cor...
components/precache/core/precache_database_unittest.cc:559:
CHECK_NE(PrecacheReferrerHostEntry::kInvalidId, id);
On 2016/12/20 01:41:59, twifkak wrote:
> Per the last bullet of
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...,
> use ASSERT_NE instead.

Done.

https://codereview.chromium.org/2586813004/diff/20001/components/precache/cor...
components/precache/core/precache_database_unittest.cc:560:
precache_database_->GetURLListForReferrerHost(id, &unused_a, &unused_b);
On 2016/12/20 01:41:59, twifkak wrote:
> I assume the purpose of this call is to mark the already_reported resources as
> is_download_reported = 1. Leave a comment to that effect.

Done.

https://codereview.chromium.org/2586813004/diff/20001/components/precache/cor...
File components/precache/core/precache_url_table.cc (right):

https://codereview.chromium.org/2586813004/diff/20001/components/precache/cor...
components/precache/core/precache_url_table.cc:50: statement->BindString(3,
GetKey(url));
On 2016/12/20 01:41:59, twifkak wrote:
> Make the order of the bindvars and the order of the function params the same.

NA.

https://codereview.chromium.org/2586813004/diff/20001/components/precache/cor...
components/precache/core/precache_url_table.cc:60: if (is_precached &&
!was_cached) {
On 2016/12/20 01:41:59, twifkak wrote:
> Note that "is_precached" here is a misnomer, since RecordURLPrefetch passes a
> value of `!was_cached || precache_url_table_.GetURLInfo(url).is_precached`.
> Since (A && B) || A == A, you can simplify your if statement to:
> 
> if (!was_cached) {

Good catch.

This is not applicable anymore but, even though your logic is correct, I
wouldn't have made this change since it is easy to change the callsite or this
code and break this assumption. My code was superflueous but it was explicit.

https://codereview.chromium.org/2586813004/diff/20001/components/precache/cor...
components/precache/core/precache_url_table.cc:82: if (!BindStatementAndRun(url,
referrer_host_id, is_precached, precache_time,
On 2016/12/20 01:42:00, twifkak wrote:
> I'd rather move this logic out of PrecacheURLTable and into PrecacheDatabase,
> for two reasons:
> 
> 1. The only caller of AddURL is
PrecacheDatabase::RecordURLPrefetch{,Internal},
> which already has some confusing boolean logic. Spreading it across
> caller/callee makes it unnecessarily more confusing. I think the intent of
this
> PrecacheURLTable class is to be as dumb as possible, and let the logic be in
> PrecacheDatabase.
> 
> 2. I'm not a fan of relying on an error condition (for a few reasons [*]), and
> you should be able to avoid it by adding is_download_reported to
PrecacheURLInfo
> and moving this logic into RecordURLPrefetch, which already does a SELECT via
> its GetURLInfo call.
> 
> That would change your FSM comment at the bottom to be
> `AddUrl(!is_download_reported)`, which I guess is not entirely satisfying but
> overall I think it'd be an improvement in readability. WDYT?
> 
> ---
> 
> [*] a. As you state, it relies on some outside configuration, which is brittle
> -- maybe they want to change the error_callback setting in the future, and
will
> have to contend with this code.
> 
> b. Maybe it's slower? I have no basis for this, other than many languages have
> slow exceptions, but that's generally because they have to generate a
backtrace.
> 
> c. (nit) What if it errors for some other reason than the "OR ABORT"? Per
> https://sqlite.org/lang_conflict.html the abort happens if there's an
> "applicable constraint violation", but maybe there's some other transient I/O
> error or something. In this case, the result will be "It does a zero-row
update"
> which is equivalent to what would have happened anyway if the INSERT failed,
so
> this is just academic.

Done.

https://codereview.chromium.org/2586813004/diff/20001/components/precache/cor...
components/precache/core/precache_url_table.cc:146: if
(!downloaded_urls->empty()) {
On 2016/12/20 01:42:00, twifkak wrote:
> Likewise, consider moving this block into a separate function and calling it
> directly from PrecacheDatabase.

Done. I should have noticed this when I was testing the same logic in two
different places :-(

Very good review. Kudos.

https://codereview.chromium.org/2586813004/diff/20001/components/precache/cor...
File components/precache/core/precache_url_table.h (right):

https://codereview.chromium.org/2586813004/diff/20001/components/precache/cor...
components/precache/core/precache_url_table.h:15: #include "sql/statement.h"
On 2016/12/20 01:42:00, twifkak wrote:
> If this is for sql::Statement, just fwd declare:
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Done.

https://codereview.chromium.org/2586813004/diff/20001/components/precache/cor...
components/precache/core/precache_url_table.h:59: // precached, |time| is the
timestamp. Replaces the row if one already exists.
On 2016/12/20 01:42:00, twifkak wrote:
> Update doc.

Done.

https://codereview.chromium.org/2586813004/diff/20001/components/precache/cor...
components/precache/core/precache_url_table.h:75: // Populates the used and
unused resource URLs for the referrer host with id
On 2016/12/20 01:42:00, twifkak wrote:
> s/unused/downloaded

Done.

[twifkak, 2016-12-20 22:48:10]

lgtm

https://codereview.chromium.org/2586813004/diff/80001/components/precache/cor...
File components/precache/core/precache_database.cc (right):

https://codereview.chromium.org/2586813004/diff/80001/components/precache/cor...
components/precache/core/precache_database.cc:205: (!info.is_precached ||
was_cached) && info.is_download_reported));
Perhaps extract this logic to be more readable:

bool is_download_reported = info.is_download_reported;
// Reset to false if a new prefetch occurred:
if (info.is_precached && !was_cached)
  is_download_reported = false;

https://codereview.chromium.org/2586813004/diff/80001/components/precache/cor...
File components/precache/core/precache_database_unittest.cc (right):

https://codereview.chromium.org/2586813004/diff/80001/components/precache/cor...
components/precache/core/precache_database_unittest.cc:656:
EXPECT_TRUE(actual_used_urls.empty());
change this to EXPECT_THAT(actual_used_urls, IsEmpty()) (for a better error
message) and add a check for actual_downloaded_urls too

https://codereview.chromium.org/2586813004/diff/80001/components/precache/cor...
components/precache/core/precache_database_unittest.cc:670:
EXPECT_EQ(expected_url_table_map, GetActualURLTableMap());
Add a resource for which is_download_reported gets reset to 0.

Also, this test is getting pretty long. Maybe you want to split out the new URLs
in a separate test looking only at downloaded_urls?

https://codereview.chromium.org/2586813004/diff/80001/components/precache/cor...
File components/precache/core/precache_url_table.cc (right):

https://codereview.chromium.org/2586813004/diff/80001/components/precache/cor...
components/precache/core/precache_url_table.cc:177: "is_precached INTEGER, "
Um. Ugh. There is no default 1 for is_precached here but there is for the ALTER
TABLE variant below. Created a bug for this, since it's not new to your CL:
https://bugs.chromium.org/p/chromium/issues/detail?id=676120

[jamartin, 2016-12-20 23:12:32]

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

[commit-bot: I haz the power, 2016-12-20 23:13:29]

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

[jamartin, 2016-12-20 23:14:07]

Done. I'll submit as soon as this has been tested on the dev phone.

https://codereview.chromium.org/2586813004/diff/80001/components/precache/cor...
File components/precache/core/precache_database.cc (right):

https://codereview.chromium.org/2586813004/diff/80001/components/precache/cor...
components/precache/core/precache_database.cc:205: (!info.is_precached ||
was_cached) && info.is_download_reported));
On 2016/12/20 22:48:09, twifkak wrote:
> Perhaps extract this logic to be more readable:
> 
> bool is_download_reported = info.is_download_reported;
> // Reset to false if a new prefetch occurred:
> if (info.is_precached && !was_cached)
>   is_download_reported = false;

Done.

https://codereview.chromium.org/2586813004/diff/80001/components/precache/cor...
File components/precache/core/precache_database_unittest.cc (right):

https://codereview.chromium.org/2586813004/diff/80001/components/precache/cor...
components/precache/core/precache_database_unittest.cc:656:
EXPECT_TRUE(actual_used_urls.empty());
On 2016/12/20 22:48:09, twifkak wrote:
> change this to EXPECT_THAT(actual_used_urls, IsEmpty()) (for a better error
> message) and add a check for actual_downloaded_urls too

Done.

https://codereview.chromium.org/2586813004/diff/80001/components/precache/cor...
components/precache/core/precache_database_unittest.cc:670:
EXPECT_EQ(expected_url_table_map, GetActualURLTableMap());
On 2016/12/20 22:48:09, twifkak wrote:
> Add a resource for which is_download_reported gets reset to 0.
> 
> Also, this test is getting pretty long. Maybe you want to split out the new
URLs
> in a separate test looking only at downloaded_urls?

Done.

https://codereview.chromium.org/2586813004/diff/80001/components/precache/cor...
File components/precache/core/precache_url_table.cc (right):

https://codereview.chromium.org/2586813004/diff/80001/components/precache/cor...
components/precache/core/precache_url_table.cc:177: "is_precached INTEGER, "
On 2016/12/20 22:48:09, twifkak wrote:
> Um. Ugh. There is no default 1 for is_precached here but there is for the
ALTER
> TABLE variant below. Created a bug for this, since it's not new to your CL:
> https://bugs.chromium.org/p/chromium/issues/detail?id=676120

Acknowledged.

This may cause a bug in the future but, currently, I think it won't affect the
behavior since we never rely on the default value in the current implementation
of AddURL.

[commit-bot: I haz the power, 2016-12-20 23:55:03]

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

[commit-bot: I haz the power, 2016-12-20 23:55:04]

Dry run: This issue passed the CQ dry run.

[twifkak, 2016-12-21 02:26:19]

On 2016/12/20 23:14:07, jamartin wrote:
> Done. I'll submit as soon as this has been tested on the dev phone.

Seems to be working. I added three log statements to their respective functions:
  LOG(INFO) << "GetURLListForReferrerHost " << referrer_host_id;
  LOG(INFO) << "RecordURLPrefetch " << url << " " << referrer_host << " " <<
was_cached;
  LOG(INFO) << "RecordURLPrefetchInternal " << url << " " << referrer_host << "
(" << referrer_host_id << ") " << is_precached << " " << is_download_reported;
(Plus lots of other log statements I have stashed away for times like these.)

I let the precache run three times on a fresh install (empty cache but non-empty
history). Logs are here: https://hastebin.com/muhovuzewi.log

All the right trademarks are there:
1. GetURLList is called the second and third times.
2. RecordURLPrefetchInternal reports "1 0" first time, then a mix of "1 1" and
"1 0" the second time.
3. Manifest URLs contain no params the first time, then a big list of d the
second time, then a smaller list of d the third time.

[twifkak, 2016-12-21 19:16:12]

The CQ bit was checked by twifkak@chromium.org

[twifkak, 2016-12-21 19:16:12]

The patchset sent to the CQ was uploaded after l-g-t-m from twifkak@chromium.org
Link to the patchset: https://codereview.chromium.org/2586813004/#ps100001
(title: "Improved tests")

[commit-bot: I haz the power, 2016-12-21 19:16:38]

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-12-21 19:26:50]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1482347772223310,
"parent_rev": "dbf4cbbd74b596f01d915d3e82386dc450905664", "commit_rev":
"850ccdb2216b63b32b8743e7697a17478e13dd7d"}

[commit-bot: I haz the power, 2016-12-21 19:28:15]

Description was changed from

==========
Report downloaded resources at most once

Before, we were reporting used_resources and unused_resources. used_resources
was reported only once when the resource is used but unused_resources was
reported on every single prefetch session for as long as the resource was still
unused.

This change removes unused_resources and adds downloaded resources (with URL
parameter d), which reports the "cost" of a resource once. This allows to have a
meaningful ratio of used_resources / downloaded_resources.

BUG=675732
==========

to

==========
Report downloaded resources at most once

Before, we were reporting used_resources and unused_resources. used_resources
was reported only once when the resource is used but unused_resources was
reported on every single prefetch session for as long as the resource was still
unused.

This change removes unused_resources and adds downloaded resources (with URL
parameter d), which reports the "cost" of a resource once. This allows to have a
meaningful ratio of used_resources / downloaded_resources.

BUG=675732

Review-Url: https://codereview.chromium.org/2586813004
==========

[commit-bot: I haz the power, 2016-12-21 19:28:21]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-21 19:30:41]

Description was changed from

==========
Report downloaded resources at most once

Before, we were reporting used_resources and unused_resources. used_resources
was reported only once when the resource is used but unused_resources was
reported on every single prefetch session for as long as the resource was still
unused.

This change removes unused_resources and adds downloaded resources (with URL
parameter d), which reports the "cost" of a resource once. This allows to have a
meaningful ratio of used_resources / downloaded_resources.

BUG=675732

Review-Url: https://codereview.chromium.org/2586813004
==========

to

==========
Report downloaded resources at most once

Before, we were reporting used_resources and unused_resources. used_resources
was reported only once when the resource is used but unused_resources was
reported on every single prefetch session for as long as the resource was still
unused.

This change removes unused_resources and adds downloaded resources (with URL
parameter d), which reports the "cost" of a resource once. This allows to have a
meaningful ratio of used_resources / downloaded_resources.

BUG=675732

Committed: https://crrev.com/02bf6c04e45ee64670eb9e6cd3a05c54d9a2eaa0
Cr-Commit-Position: refs/heads/master@{#440176}
==========

[commit-bot: I haz the power, 2016-12-21 19:30:41]

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