Report Precache.TimeSinceLastPrecache

Report Precache.TimeSinceLastPrecache

This will provide the time (in minutes) between the last time a precache
session was started and each of the URL fetches made by the user ever since.

This would allow us to better correlate byte savings/PLT with how recently a
precache happened.

BUG=619211
Committed: https://crrev.com/a973a3cd260653c8747a3df4d0a97ca3b915579e
Cr-Commit-Position: refs/heads/master@{#406624}

[jamartin, 2016-07-05 17:32:24]

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

[Raj, 2016-07-05 19:46:28]

rajendrant@chromium.org changed reviewers:
+ rajendrant@chromium.org

[Raj, 2016-07-05 19:46:29]

https://codereview.chromium.org/2123813002/diff/40001/components/precache/cor...
File components/precache/core/precache_database.h (right):

https://codereview.chromium.org/2123813002/diff/40001/components/precache/cor...
components/precache/core/precache_database.h:116: // Records the time since the
last prefetch.
/s/prefetch/precache

https://codereview.chromium.org/2123813002/diff/40001/components/precache/cor...
File components/precache/core/precache_session_table.cc (right):

https://codereview.chromium.org/2123813002/diff/40001/components/precache/cor...
components/precache/core/precache_session_table.cc:31: void
PrecacheSessionTable::SetLastPrecacheTimestamp(const base::Time& time) {
The previous precache task start time is stored in java preference.
May be we could use that instead of this table. But a jni wrapper is needed to
call java code.

Also the unittests will be a little harder.

https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

https://codereview.chromium.org/2123813002/diff/40001/components/precache/cor...
components/precache/core/precache_session_table.cc:33:
timestamp.set_seconds(time.ToDoubleT());
There seems to be a conversion here ?
static_cast<int64>(time.ToDoubleT())

https://codereview.chromium.org/2123813002/diff/40001/components/precache/cor...
components/precache/core/precache_session_table.cc:38: statement.BindString(1,
timestamp.SerializeAsString());
nit: Just wondering what is the need for timestamp proto.
Statement provides BindInt64() and BindDouble()

https://cs.chromium.org/chromium/src/sql/statement.h?rcl=0&l=108

[jamartin, 2016-07-06 14:14:14]

PTAL.

https://codereview.chromium.org/2123813002/diff/40001/components/precache/cor...
File components/precache/core/precache_database.h (right):

https://codereview.chromium.org/2123813002/diff/40001/components/precache/cor...
components/precache/core/precache_database.h:116: // Records the time since the
last prefetch.
On 2016/07/05 19:46:28, Raj wrote:
> /s/prefetch/precache

Done.

https://codereview.chromium.org/2123813002/diff/40001/components/precache/cor...
File components/precache/core/precache_session_table.cc (right):

https://codereview.chromium.org/2123813002/diff/40001/components/precache/cor...
components/precache/core/precache_session_table.cc:31: void
PrecacheSessionTable::SetLastPrecacheTimestamp(const base::Time& time) {
On 2016/07/05 19:46:28, Raj wrote:
> The previous precache task start time is stored in java preference.
> May be we could use that instead of this table. But a jni wrapper is needed to
> call java code.
> 
> Also the unittests will be a little harder.
> 
>
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...

What would be the benefit? If it is not clear, I'd rather not to complicate the
code and keep our data in the same place.

https://codereview.chromium.org/2123813002/diff/40001/components/precache/cor...
components/precache/core/precache_session_table.cc:33:
timestamp.set_seconds(time.ToDoubleT());
On 2016/07/05 19:46:28, Raj wrote:
> There seems to be a conversion here ?
> static_cast<int64>(time.ToDoubleT())

Good catch.

I got lucky and it was working OK before because base::Time already does
saturated casts on int64 internally. However, I've added the conversions here to
make sure we abide to the API, not the internal implementation.

https://codereview.chromium.org/2123813002/diff/40001/components/precache/cor...
components/precache/core/precache_session_table.cc:38: statement.BindString(1,
timestamp.SerializeAsString());
On 2016/07/05 19:46:28, Raj wrote:
> nit: Just wondering what is the need for timestamp proto.
> Statement provides BindInt64() and BindDouble()
> 
> https://cs.chromium.org/chromium/src/sql/statement.h?rcl=0&l=108

I had at least the following alternatives:
 a) Create an INTEGER column and store the seconds there. This would have
required to change the table schema, both creation and alter for existing users.
 b) Store the number as a string, without a proto. This works and is even less
code than my current approach.
 c) The current approach.

The current approach has the mild benefits that it is consistent with the other
type, it is easy to extend without altering the schema, and has data validation
for free.

If you don't feel strongly about this, I'd rather keep it as it is.

[jamartin, 2016-07-06 16:28:01]

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

[jamartin, 2016-07-06 16:28:02]

@rkaplow: Please take a look at histogram.xml for ownership approval

[Raj, 2016-07-06 17:16:46]

lgtm % nits

I am not owner of those files.
So you would have to wait for twifkak@

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

https://codereview.chromium.org/2123813002/diff/60001/components/precache/cor...
components/precache/core/precache_database.cc:32: // A PrecacheDatabase can be
constructed on any thread.
nit: initialize last_precache_timestamp_ to 0

https://codereview.chromium.org/2123813002/diff/60001/components/precache/cor...
components/precache/core/precache_database.cc:107: base::Time
PrecacheDatabase::GetLastPrecacheTimestamp() {
nit: Can we check if the cached last_precache_timestamp_ is valid (!=0), and
return it and avoid the sql call.

[rkaplow, 2016-07-06 21:52:12]

https://codereview.chromium.org/2123813002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2123813002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:42579: +<histogram
name="Precache.TimeSinceLastPrecache" units="minutes">
it looks like you're subtracting timestamps, which should be ms. What is
preferred?

[jamartin, 2016-07-07 12:12:36]

PTAL.

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

https://codereview.chromium.org/2123813002/diff/60001/components/precache/cor...
components/precache/core/precache_database.cc:32: // A PrecacheDatabase can be
constructed on any thread.
On 2016/07/06 17:16:46, Raj wrote:
> nit: initialize last_precache_timestamp_ to 0

No need. The default constructor of base::Time already initializes it so that
is_null (sort of 0).

https://codereview.chromium.org/2123813002/diff/60001/components/precache/cor...
components/precache/core/precache_database.cc:107: base::Time
PrecacheDatabase::GetLastPrecacheTimestamp() {
On 2016/07/06 17:16:46, Raj wrote:
> nit: Can we check if the cached last_precache_timestamp_ is valid (!=0), and
> return it and avoid the sql call. 

Done.

Anyway, I think I put this function here only for testing purposes.

https://codereview.chromium.org/2123813002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2123813002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:42579: +<histogram
name="Precache.TimeSinceLastPrecache" units="minutes">
On 2016/07/06 21:52:12, rkaplow wrote:
> it looks like you're subtracting timestamps, which should be ms. What is
> preferred?

As you say, we are reporting a base::TimeDelta. However, this TimeDelta is
expected to be the difference between the time of the last precache (usually at
night without user interaction on the phone) and every subsequent time the user
navigates to a page. We expect this to be in the range of hours. Besides, we are
likely going to compare these values against max-age and other cache-related
values which are measured in seconds, so I'm changing the unit to seconds.

I don't think there is a need for more granularity than that. Please let me know
if I'm missing anything.

[rkaplow, 2016-07-07 15:01:35]

https://codereview.chromium.org/2123813002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2123813002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:42579: +<histogram
name="Precache.TimeSinceLastPrecache" units="minutes">
On 2016/07/07 12:12:36, jamartin wrote:
> On 2016/07/06 21:52:12, rkaplow wrote:
> > it looks like you're subtracting timestamps, which should be ms. What is
> > preferred?
> 
> As you say, we are reporting a base::TimeDelta. However, this TimeDelta is
> expected to be the difference between the time of the last precache (usually
at
> night without user interaction on the phone) and every subsequent time the
user
> navigates to a page. We expect this to be in the range of hours. Besides, we
are
> likely going to compare these values against max-age and other cache-related
> values which are measured in seconds, so I'm changing the unit to seconds.
> 
> I don't think there is a need for more granularity than that. Please let me
know
> if I'm missing anything.

There's no logic behind the 'units' tag here - it's just what shows up in the
UI. Storing as seconds (or hours -  I trust you know best) is fine - but you
still need to convert the number in ms into seconds, otherwise the values will
be incorrect.

[jamartin, 2016-07-07 21:58:22]

On 2016/07/07 15:01:35, rkaplow wrote:
>
https://codereview.chromium.org/2123813002/diff/60001/tools/metrics/histogram...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/2123813002/diff/60001/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:42579: +<histogram
> name="Precache.TimeSinceLastPrecache" units="minutes">
> On 2016/07/07 12:12:36, jamartin wrote:
> > On 2016/07/06 21:52:12, rkaplow wrote:
> > > it looks like you're subtracting timestamps, which should be ms. What is
> > > preferred?
> > 
> > As you say, we are reporting a base::TimeDelta. However, this TimeDelta is
> > expected to be the difference between the time of the last precache (usually
> at
> > night without user interaction on the phone) and every subsequent time the
> user
> > navigates to a page. We expect this to be in the range of hours. Besides, we
> are
> > likely going to compare these values against max-age and other cache-related
> > values which are measured in seconds, so I'm changing the unit to seconds.
> > 
> > I don't think there is a need for more granularity than that. Please let me
> know
> > if I'm missing anything.
> 
> There's no logic behind the 'units' tag here - it's just what shows up in the
> UI. Storing as seconds (or hours -  I trust you know best) is fine - but you
> still need to convert the number in ms into seconds, otherwise the values will
> be incorrect.

Ah, I blindly assumed some logic that simply wasn't there.

I've changed it to ms but now I'm using UMA_HISTOGRAM_CUSTOM_TIMES.

I'm not sure what are the best practices to pick the min and max for the
buckets. I would have liked to go beyond 1 day, but I'm afraid I'm going to lose
too much resolution by doing that.

[jamartin, 2016-07-08 09:08:10]

On 2016/07/07 21:58:22, jamartin wrote:
> On 2016/07/07 15:01:35, rkaplow wrote:
> >
>
https://codereview.chromium.org/2123813002/diff/60001/tools/metrics/histogram...
> > File tools/metrics/histograms/histograms.xml (right):
> > 
> >
>
https://codereview.chromium.org/2123813002/diff/60001/tools/metrics/histogram...
> > tools/metrics/histograms/histograms.xml:42579: +<histogram
> > name="Precache.TimeSinceLastPrecache" units="minutes">
> > On 2016/07/07 12:12:36, jamartin wrote:
> > > On 2016/07/06 21:52:12, rkaplow wrote:
> > > > it looks like you're subtracting timestamps, which should be ms. What is
> > > > preferred?
> > > 
> > > As you say, we are reporting a base::TimeDelta. However, this TimeDelta is
> > > expected to be the difference between the time of the last precache
(usually
> > at
> > > night without user interaction on the phone) and every subsequent time the
> > user
> > > navigates to a page. We expect this to be in the range of hours. Besides,
we
> > are
> > > likely going to compare these values against max-age and other
cache-related
> > > values which are measured in seconds, so I'm changing the unit to seconds.
> > > 
> > > I don't think there is a need for more granularity than that. Please let
me
> > know
> > > if I'm missing anything.
> > 
> > There's no logic behind the 'units' tag here - it's just what shows up in
the
> > UI. Storing as seconds (or hours -  I trust you know best) is fine - but you
> > still need to convert the number in ms into seconds, otherwise the values
will
> > be incorrect.
> 
> Ah, I blindly assumed some logic that simply wasn't there.
> 
> I've changed it to ms but now I'm using UMA_HISTOGRAM_CUSTOM_TIMES.
> 
> I'm not sure what are the best practices to pick the min and max for the
> buckets. I would have liked to go beyond 1 day, but I'm afraid I'm going to
lose
> too much resolution by doing that.

PTAL.

[rkaplow, 2016-07-08 14:37:59]

lgtm

https://codereview.chromium.org/2123813002/diff/120001/components/precache/co...
File components/precache/core/precache_database.cc (right):

https://codereview.chromium.org/2123813002/diff/120001/components/precache/co...
components/precache/core/precache_database.cc:249: kSecondsInMinute, 
kSecondsInDay, 100);
not really obvious to me you have a min of 60 - why not 0 here? (for
kSecondsInMinute)

https://codereview.chromium.org/2123813002/diff/120001/components/precache/co...
components/precache/core/precache_database.cc:249: kSecondsInMinute, 
kSecondsInDay, 100);
picking the max as a day seems fine though (responding to your comment about
unsure what to pick). THat is - if that is the full range you are interested in.

[jamartin, 2016-07-08 14:52:49]

Thanks. Waiting on twifkak@ for the final LGTM.

https://codereview.chromium.org/2123813002/diff/120001/components/precache/co...
File components/precache/core/precache_database.cc (right):

https://codereview.chromium.org/2123813002/diff/120001/components/precache/co...
components/precache/core/precache_database.cc:249: kSecondsInMinute, 
kSecondsInDay, 100);
On 2016/07/08 14:37:59, rkaplow wrote:
> not really obvious to me you have a min of 60 - why not 0 here? (for
> kSecondsInMinute)

0 is not a valid min as per Histogram::FactoryGet. Besides, this event is
supposed to happen in the middle of the night when there is no interaction with
the user. If the user navigated less than a minute after this took place, I
don't need to know the time delta with any more resolution than that.

https://codereview.chromium.org/2123813002/diff/120001/components/precache/co...
components/precache/core/precache_database.cc:249: kSecondsInMinute, 
kSecondsInDay, 100);
On 2016/07/08 14:37:59, rkaplow wrote:
> picking the max as a day seems fine though (responding to your comment about
> unsure what to pick). THat is - if that is the full range you are interested
in.

Thanks. Ideally I'd like more than a day, probably a week, but then the buckets
start to get too big (if split equally, now they would be about 15 min wide as
they currently are).

If twifkak@ agrees, we'll start with these buckets and we'll revisit the range
if we see that most of our entries are in the max.

[Raj, 2016-07-08 17:14:20]

On 2016/07/08 14:52:49, jamartin wrote:
> Thanks. Waiting on twifkak@ for the final LGTM.
> 
>
https://codereview.chromium.org/2123813002/diff/120001/components/precache/co...
> File components/precache/core/precache_database.cc (right):
> 
>
https://codereview.chromium.org/2123813002/diff/120001/components/precache/co...
> components/precache/core/precache_database.cc:249: kSecondsInMinute, 
> kSecondsInDay, 100);
> On 2016/07/08 14:37:59, rkaplow wrote:
> > not really obvious to me you have a min of 60 - why not 0 here? (for
> > kSecondsInMinute)
> 
> 0 is not a valid min as per Histogram::FactoryGet. Besides, this event is
> supposed to happen in the middle of the night when there is no interaction
with
> the user. If the user navigated less than a minute after this took place, I
> don't need to know the time delta with any more resolution than that.
> 
>
https://codereview.chromium.org/2123813002/diff/120001/components/precache/co...
> components/precache/core/precache_database.cc:249: kSecondsInMinute, 
> kSecondsInDay, 100);
> On 2016/07/08 14:37:59, rkaplow wrote:
> > picking the max as a day seems fine though (responding to your comment about
> > unsure what to pick). THat is - if that is the full range you are interested
> in.
> 
> Thanks. Ideally I'd like more than a day, probably a week, but then the
buckets
> start to get too big (if split equally, now they would be about 15 min wide as
> they currently are).
> 
> If twifkak@ agrees, we'll start with these buckets and we'll revisit the range
> if we see that most of our entries are in the max.

Sorry. Missed seeing this earlier.
The buckets are not equally split, they are exponentially distributed.
UMA_HISTOGRAM_CUSTOM_TIMES() could be used as well.
For example, pls see "Precache.Fetch.TimeToComplete"
But its little hard to read since it displays it as ms in UMA dashboard.

https://cs.chromium.org/chromium/src/components/precache/core/precache_fetche...

https://uma.googleplex.com/p/chrome/histograms?endDate=07-07-2016&dayCount=1&...

[twifkak, 2016-07-08 23:47:53]

lgtm

Sorry for the delay. Also, if you haven't yet and it's not too much pain, it
would be nice to test this manually on your phone.

https://codereview.chromium.org/2123813002/diff/120001/components/precache/co...
File components/precache/core/precache_database.cc (right):

https://codereview.chromium.org/2123813002/diff/120001/components/precache/co...
components/precache/core/precache_database.cc:111: if
(!last_precache_timestamp_.is_null() || !IsDatabaseAccessible()) {
Nit: Can get rid of the redundant return statement by negating the condition
clause:

base::Time PrecacheDatabase::GetLastPrecacheTimestamp() {
  if (last_precache_timestamp_.is_null() && IsDatableAccessible())
    last_precache_timestamp_ =
precache_session_table_.GetLastPrecacheTimestamp();
  return last_precache_timestamp_;
}

https://codereview.chromium.org/2123813002/diff/120001/components/precache/co...
components/precache/core/precache_database.cc:235: if
(last_precache_timestamp_.is_null()) {
Why don't you just call [this->]GetLastPrecacheTimestamp here?

https://codereview.chromium.org/2123813002/diff/120001/components/precache/co...
components/precache/core/precache_database.cc:249: kSecondsInMinute, 
kSecondsInDay, 100);
On 2016/07/08 14:52:49, jamartin wrote:
> On 2016/07/08 14:37:59, rkaplow wrote:
> > picking the max as a day seems fine though (responding to your comment about
> > unsure what to pick). THat is - if that is the full range you are interested
> in.
> 
> Thanks. Ideally I'd like more than a day, probably a week, but then the
buckets
> start to get too big (if split equally, now they would be about 15 min wide as
> they currently are).
> 
> If twifkak@ agrees, we'll start with these buckets and we'll revisit the range
> if we see that most of our entries are in the max.

IIRC, the advice I was given a while back was to make sure the 95%ile was not in
the overflow bucket, give or take. (Or was it 99%ile? I don't remember.)

Looking at
https://uma.googleplex.com/p/chrome/histograms/?endDate=07-07-2016&dayCount=1...,
we see that the 95%ile for precache interval is around 50 hours. This is
probably an underestimate, as that UMA only reports if precache runs a second
time.

If we assume that the time of a NonPrefetch request is independent from the time
of a Prefetch request, then 50/2=25 hours seems like a reasonable 95%ile for
time-since-precache then. But given this is a bit of guesswork, I'd be inclined
to go with ~36 hours. After all, if you overestimate your max, then your bucket
sizes are a bit too large. If you underestimate your max, then your last bucket
size is _way_ too large.

A couple of side notes:

1. By default buckets are not split linearly, but exponentially (i.e. UMA
assumes that your metric has lognormal distribution).

2. Buckets are not supposed to be changed on UMAs. One must deprecate the old
and rename the metric to BlahBlah2.

WDYT?

https://codereview.chromium.org/2123813002/diff/120001/components/precache/co...
File components/precache/core/precache_session_table.cc (right):

https://codereview.chromium.org/2123813002/diff/120001/components/precache/co...
components/precache/core/precache_session_table.cc:35:
timestamp.set_seconds(base::saturated_cast<int64_t>(time.ToDoubleT()));
Nit: I think `(time - base::Time::UnixEpoch()).InSeconds()` would avoid the
int64->double->int64 casts and express its meaning better.

https://codereview.chromium.org/2123813002/diff/120001/components/precache/co...
components/precache/core/precache_session_table.cc:51: return
base::Time::FromDoubleT(timestamp.seconds());
Likewise, this could become `base::Time::UnixEpoch() +
base::TimeDelta::FromSeconds(timestamp.seconds())`.

[jamartin, 2016-07-12 14:17:35]

> Sorry for the delay. Also, if you haven't yet and it's not too much pain, it
> would be nice to test this manually on your phone.

I installed the APK on my personal phone but I couldn't sign in Chromium (which
I believe is one of the conditions for the experiment) because it said "account
already exists on this device" and, when I tried to log in with the Chromium
account (which was not in my personal phone) it restarted the log in process and
then it complained that the account already existed in my device.

Did you encounter this issue?

I've ordered a USB-C cable in order to force myself into the experiment using
ADB. It will arrive tomorrow Wednesday, so I'll give it another go then.

[jamartin, 2016-07-12 14:18:44]

[Forgot to publish my comment replies, please check my previous question about
how to test this on my personal phone]

https://codereview.chromium.org/2123813002/diff/120001/components/precache/co...
File components/precache/core/precache_database.cc (right):

https://codereview.chromium.org/2123813002/diff/120001/components/precache/co...
components/precache/core/precache_database.cc:111: if
(!last_precache_timestamp_.is_null() || !IsDatabaseAccessible()) {
On 2016/07/08 23:47:53, twifkak wrote:
> Nit: Can get rid of the redundant return statement by negating the condition
> clause:
> 
> base::Time PrecacheDatabase::GetLastPrecacheTimestamp() {
>   if (last_precache_timestamp_.is_null() && IsDatableAccessible())
>     last_precache_timestamp_ =
> precache_session_table_.GetLastPrecacheTimestamp();
>   return last_precache_timestamp_;
> }

Good point.

https://codereview.chromium.org/2123813002/diff/120001/components/precache/co...
components/precache/core/precache_database.cc:235: if
(last_precache_timestamp_.is_null()) {
On 2016/07/08 23:47:53, twifkak wrote:
> Why don't you just call [this->]GetLastPrecacheTimestamp here?

Good point.

This grew organically. This GetLastPrecacheTimestamp was originally meant as
code just for testing, then Raj suggested an improvement, and now you ended up
suggesting using it in the main path.

https://codereview.chromium.org/2123813002/diff/120001/components/precache/co...
components/precache/core/precache_database.cc:249: kSecondsInMinute, 
kSecondsInDay, 100);
On 2016/07/08 23:47:53, twifkak wrote:
> On 2016/07/08 14:52:49, jamartin wrote:
> > On 2016/07/08 14:37:59, rkaplow wrote:
> > > picking the max as a day seems fine though (responding to your comment
about
> > > unsure what to pick). THat is - if that is the full range you are
interested
> > in.
> > 
> > Thanks. Ideally I'd like more than a day, probably a week, but then the
> buckets
> > start to get too big (if split equally, now they would be about 15 min wide
as
> > they currently are).
> > 
> > If twifkak@ agrees, we'll start with these buckets and we'll revisit the
range
> > if we see that most of our entries are in the max.
> 
> IIRC, the advice I was given a while back was to make sure the 95%ile was not
in
> the overflow bucket, give or take. (Or was it 99%ile? I don't remember.)
> 
> Looking at
>
https://uma.googleplex.com/p/chrome/histograms/?endDate=07-07-2016&dayCount=1...,
> we see that the 95%ile for precache interval is around 50 hours. This is
> probably an underestimate, as that UMA only reports if precache runs a second
> time.
> 
> If we assume that the time of a NonPrefetch request is independent from the
time
> of a Prefetch request, then 50/2=25 hours seems like a reasonable 95%ile for
> time-since-precache then. But given this is a bit of guesswork, I'd be
inclined
> to go with ~36 hours. After all, if you overestimate your max, then your
bucket
> sizes are a bit too large. If you underestimate your max, then your last
bucket
> size is _way_ too large.
> 
> A couple of side notes:
> 
> 1. By default buckets are not split linearly, but exponentially (i.e. UMA
> assumes that your metric has lognormal distribution).
> 
> 2. Buckets are not supposed to be changed on UMAs. One must deprecate the old
> and rename the metric to BlahBlah2.
> 
> WDYT?

Done (max = 36h).

BTW, what do you think of the minimum (currently 1 minute)?

https://codereview.chromium.org/2123813002/diff/120001/components/precache/co...
File components/precache/core/precache_session_table.cc (right):

https://codereview.chromium.org/2123813002/diff/120001/components/precache/co...
components/precache/core/precache_session_table.cc:35:
timestamp.set_seconds(base::saturated_cast<int64_t>(time.ToDoubleT()));
On 2016/07/08 23:47:53, twifkak wrote:
> Nit: I think `(time - base::Time::UnixEpoch()).InSeconds()` would avoid the
> int64->double->int64 casts and express its meaning better.

Done.

https://codereview.chromium.org/2123813002/diff/120001/components/precache/co...
components/precache/core/precache_session_table.cc:51: return
base::Time::FromDoubleT(timestamp.seconds());
On 2016/07/08 23:47:53, twifkak wrote:
> Likewise, this could become `base::Time::UnixEpoch() +
> base::TimeDelta::FromSeconds(timestamp.seconds())`.

Done.

[twifkak, 2016-07-16 00:39:33]

On 2016/07/12 14:17:35, jamartin wrote:
> > Sorry for the delay. Also, if you haven't yet and it's not too much pain, it
> > would be nice to test this manually on your phone.
> 
> I installed the APK on my personal phone but I couldn't sign in Chromium
(which
> I believe is one of the conditions for the experiment) because it said
"account
> already exists on this device" and, when I tried to log in with the Chromium
> account (which was not in my personal phone) it restarted the log in process
and
> then it complained that the account already existed in my device.
> 
> Did you encounter this issue?

I haven't, though I have had weird issues with sign-in for clankium before. I
sometimes get a lot of those stupid triangle notifications and have to click a
bunch of things before it works.

FWIW, you don't need to sign in with your chromium account; any account will
work (I use my personal; Greg has used a one-off IIRC).

Haven't had the chance to test your change myself. Spent a bunch of time fixing
my depot_tools and building and installing it but didn't actually do the testing
part! I will try again.

[twifkak, 2016-07-16 00:47:49]

lgtm

https://codereview.chromium.org/2123813002/diff/120001/components/precache/co...
File components/precache/core/precache_database.cc (right):

https://codereview.chromium.org/2123813002/diff/120001/components/precache/co...
components/precache/core/precache_database.cc:249: kSecondsInMinute, 
kSecondsInDay, 100);
On 2016/07/12 14:18:44, jamartin wrote:
> On 2016/07/08 23:47:53, twifkak wrote:
> > On 2016/07/08 14:52:49, jamartin wrote:
> > > On 2016/07/08 14:37:59, rkaplow wrote:
> > > > picking the max as a day seems fine though (responding to your comment
> about
> > > > unsure what to pick). THat is - if that is the full range you are
> interested
> > > in.
> > > 
> > > Thanks. Ideally I'd like more than a day, probably a week, but then the
> > buckets
> > > start to get too big (if split equally, now they would be about 15 min
wide
> as
> > > they currently are).
> > > 
> > > If twifkak@ agrees, we'll start with these buckets and we'll revisit the
> range
> > > if we see that most of our entries are in the max.
> > 
> > IIRC, the advice I was given a while back was to make sure the 95%ile was
not
> in
> > the overflow bucket, give or take. (Or was it 99%ile? I don't remember.)
> > 
> > Looking at
> >
>
https://uma.googleplex.com/p/chrome/histograms/?endDate=07-07-2016&dayCount=1...,
> > we see that the 95%ile for precache interval is around 50 hours. This is
> > probably an underestimate, as that UMA only reports if precache runs a
second
> > time.
> > 
> > If we assume that the time of a NonPrefetch request is independent from the
> time
> > of a Prefetch request, then 50/2=25 hours seems like a reasonable 95%ile for
> > time-since-precache then. But given this is a bit of guesswork, I'd be
> inclined
> > to go with ~36 hours. After all, if you overestimate your max, then your
> bucket
> > sizes are a bit too large. If you underestimate your max, then your last
> bucket
> > size is _way_ too large.
> > 
> > A couple of side notes:
> > 
> > 1. By default buckets are not split linearly, but exponentially (i.e. UMA
> > assumes that your metric has lognormal distribution).
> > 
> > 2. Buckets are not supposed to be changed on UMAs. One must deprecate the
old
> > and rename the metric to BlahBlah2.
> > 
> > WDYT?
> 
> Done (max = 36h).
> 
> BTW, what do you think of the minimum (currently 1 minute)?

Oh yeah, good question.

Short: I think it's fine.

Long: Most people precache every 10 hours, so again assuming uniformity, 1% of
requests will be < 6 minutes. So the underflow will be small.

Normally, I'd say too small and we should increase it for more granularity in
the middle, but since we're looking at correlating this with PLT and things
involving the cache, it would be interesting to look at things in the low end
(since e.g. DNS ttl is often 5 minutes or smaller).

BTW, if my rough understanding of the buckets is correct:

irb> def readable(mins); if mins > 60 * 24; "#{(mins / 60 / 24).round 2} days";
elsif mins > 60; "#{(mins / 60).round 2} hours"; else "#{mins.round 2} minutes"
end end
irb> puts (1..50).map {|i| (36.0 * 60) ** (i/50.0) }.map {|m| readable m }
1.17 minutes
1.36 minutes
1.59 minutes
1.85 minutes
2.15 minutes
2.51 minutes
2.93 minutes
3.42 minutes
3.98 minutes
4.64 minutes
5.41 minutes
6.31 minutes
7.36 minutes
8.58 minutes
10.01 minutes
11.67 minutes
13.61 minutes
15.86 minutes
18.5 minutes
21.57 minutes
25.15 minutes
29.32 minutes
34.19 minutes
39.86 minutes
46.48 minutes
54.19 minutes
1.05 hours
1.23 hours
1.43 hours
1.67 hours
1.95 hours
2.27 hours
2.65 hours
3.09 hours
3.6 hours
4.19 hours
4.89 hours
5.7 hours
6.65 hours
7.75 hours
9.04 hours
10.54 hours
12.29 hours
14.33 hours
16.71 hours
19.48 hours
22.71 hours
1.1 days
1.29 days
1.5 days

Anyway, could be wrong, but LGTM.

https://codereview.chromium.org/2123813002/diff/160001/components/precache/co...
File components/precache/core/precache_session_table.cc (right):

https://codereview.chromium.org/2123813002/diff/160001/components/precache/co...
components/precache/core/precache_session_table.cc:54: : base::Time();
Oops, I missed this edge case in the DoubleT behavior; I like this version
better already. :)

[twifkak, 2016-07-20 01:47:44]

On 2016/07/16 00:39:33, twifkak wrote:
> Haven't had the chance to test your change myself. Spent a bunch of time
fixing
> my depot_tools and building and installing it but didn't actually do the
testing
> part! I will try again.

Done! Seems to be working:

Histogram: Precache.TimeSinceLastPrecache recorded 164 samples, average = 100.9
(flags = 0x1)
0    ------------------------------------------------------------------------O
(138 = 84.1%)
60   ... 
477  --------------O                                                          
(26 = 15.9%) {84.1%}
514  ...

[jamartin, 2016-07-20 17:35:43]

The CQ bit was checked by jamartin@chromium.org

[jamartin, 2016-07-20 17:35:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
rajendrant@chromium.org, rkaplow@chromium.org, twifkak@chromium.org
Link to the patchset: https://codereview.chromium.org/2123813002/#ps180001
(title: "gclient sync")

[commit-bot: I haz the power, 2016-07-20 17:36:07]

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-07-20 18:34:23]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-07-20 18:34:31]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-20 18:36:50]

Description was changed from

==========
Report Precache.TimeSinceLastPrecache

This will provide the time (in minutes) between the last time a precache
session was started and each of the URL fetches made by the user ever since.

This would allow us to better correlate byte savings/PLT with how recently a
precache happened.

BUG=619211
==========

to

==========
Report Precache.TimeSinceLastPrecache

This will provide the time (in minutes) between the last time a precache
session was started and each of the URL fetches made by the user ever since.

This would allow us to better correlate byte savings/PLT with how recently a
precache happened.

BUG=619211

Committed: https://crrev.com/a973a3cd260653c8747a3df4d0a97ca3b915579e
Cr-Commit-Position: refs/heads/master@{#406624}
==========

[commit-bot: I haz the power, 2016-07-20 18:36:51]

Patchset 10 (id:??) landed as
https://crrev.com/a973a3cd260653c8747a3df4d0a97ca3b915579e
Cr-Commit-Position: refs/heads/master@{#406624}