evict larger entries first

Changes the eviction algorithm to take the size of the entry into
account. Weighting by size means that large entries will get thrown out
of the cache sooner and the end result is that the cache will have more,
but smaller entries.

Design doc: https://docs.google.com/document/d/1N07uwDiOipRcMG3SanxR8X8flvxrh9Axi1SLpfRNKCY/edit

BUG=700102, 736437
Review-Url: https://codereview.chromium.org/2918893002
Cr-Commit-Position: refs/heads/master@{#488058}
Committed: https://chromium.googlesource.com/chromium/src/+/a9eeb01aaf3e555b843f78298de19b2f1ac29fa8

[hubbe, 2017-06-01 22:12:22]

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

[hubbe, 2017-06-01 22:12:52]

hubbe@chromium.org changed reviewers:
+ jkarlin@chromium.org

[hubbe, 2017-06-01 22:12:52]

Strawman for how to let the disk cache handle media files better.

[commit-bot: I haz the power, 2017-06-01 22:13: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, 2017-06-01 22:46:19]

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

[commit-bot: I haz the power, 2017-06-01 22:46:19]

Dry run: Try jobs failed on following builders:
  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_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hubbe, 2017-06-01 23:07:54]

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

[commit-bot: I haz the power, 2017-06-01 23:09:04]

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, 2017-06-01 23:41:54]

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

[commit-bot: I haz the power, 2017-06-01 23:41:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jkarlin, 2017-06-02 00:29:58]

We have basically no data on how various cache parameters affect performance
(other than the size of the cache). That makes changes like this difficult to
reason about. I do think that it's an interesting idea. CC'ing gavinp@ and
morlovich@ who are more invested in the cache's (especially SimpleCache's
future).

[hubbe, 2017-06-02 17:24:49]

On 2017/06/02 00:29:58, jkarlin wrote:
> We have basically no data on how various cache parameters affect performance
> (other than the size of the cache). That makes changes like this difficult to
> reason about. I do think that it's an interesting idea. CC'ing gavinp@ and
> morlovich@ who are more invested in the cache's (especially SimpleCache's
> future).

The expected result would be that we get more cache hits, but fewer bytes
fetched from the cache.
(Basically trading off lower latency for more bytes fetched over the network.)
Do you guys have metrics that we could use to test that theory?
If so, I would suggest adding flag for this and then running a finch experiment.
If not, we need to start with adding the metrics.

Of course, a smarter eviction strategy (ARC perhaps) might give us fewer bytes
AND more hits.

[Maks Orlovich, 2017-06-02 17:31:49]

> The expected result would be that we get more cache hits, but fewer bytes
> fetched from the cache.
> (Basically trading off lower latency for more bytes fetched over the network.)
> Do you guys have metrics that we could use to test that theory?
> If so, I would suggest adding flag for this and then running a finch
experiment.
> If not, we need to start with adding the metrics.

There is HttpCache.Pattern.*, but I don't know how well they'd reflect this (and
also 
some of the cache metrics may exclude the media cache depending on the cache
backend...)


> Of course, a smarter eviction strategy (ARC perhaps) might give us fewer bytes
> AND more hits.

The font service folks were lobbying for LRU-2, FWIW, our big constraint for
fancier
replacement is not eating memory.

[chromium-reviews, 2017-06-02 17:40:37]

Yeah, LRU-2 seems like a pretty good choice, and it's really easy to
implement.
Might still want to combine it with something like this CL though.
So now we have two experiments to run.

    /Hubbe

On Fri, Jun 2, 2017 at 10:31 AM, <morlovich@chromium.org> wrote:

> > The expected result would be that we get more cache hits, but fewer bytes
> > fetched from the cache.
> > (Basically trading off lower latency for more bytes fetched over the
> network.)
> > Do you guys have metrics that we could use to test that theory?
> > If so, I would suggest adding flag for this and then running a finch
> experiment.
> > If not, we need to start with adding the metrics.
>
> There is HttpCache.Pattern.*, but I don't know how well they'd reflect
> this (and
> also
> some of the cache metrics may exclude the media cache depending on the
> cache
> backend...)
>
>
> > Of course, a smarter eviction strategy (ARC perhaps) might give us fewer
> bytes
> > AND more hits.
>
> The font service folks were lobbying for LRU-2, FWIW, our big constraint
> for
> fancier
> replacement is not eating memory.
>
>
>
>
>
> https://codereview.chromium.org/2918893002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hubbe, 2017-06-02 17:55:43]

The original LRU-K seems to be more complicated than we'd actually need.
It would probably be ok to just do something like:

  if (!penultimate_access) return (now - last_access) *
kSingleAccessAccelerator;
  return (now - penultimate_access);


On 2017/06/02 17:40:37, chromium-reviews wrote:
> Yeah, LRU-2 seems like a pretty good choice, and it's really easy to
> implement.
> Might still want to combine it with something like this CL though.
> So now we have two experiments to run.
> 
>     /Hubbe
> 
> On Fri, Jun 2, 2017 at 10:31 AM, <mailto:morlovich@chromium.org> wrote:
> 
> > > The expected result would be that we get more cache hits, but fewer bytes
> > > fetched from the cache.
> > > (Basically trading off lower latency for more bytes fetched over the
> > network.)
> > > Do you guys have metrics that we could use to test that theory?
> > > If so, I would suggest adding flag for this and then running a finch
> > experiment.
> > > If not, we need to start with adding the metrics.
> >
> > There is HttpCache.Pattern.*, but I don't know how well they'd reflect
> > this (and
> > also
> > some of the cache metrics may exclude the media cache depending on the
> > cache
> > backend...)
> >
> >
> > > Of course, a smarter eviction strategy (ARC perhaps) might give us fewer
> > bytes
> > > AND more hits.
> >
> > The font service folks were lobbying for LRU-2, FWIW, our big constraint
> > for
> > fancier
> > replacement is not eating memory.
> >
> >

[chromium-reviews, 2017-06-02 18:00:38]

PS: I actually think cache-tuning is fun. so if anybody wants to point me
to where to add the metrics, I can add metrics, run experiments and
everything. As long as you guys don't mind me doing it that is.


On Fri, Jun 2, 2017 at 10:55 AM, <hubbe@chromium.org> wrote:

> The original LRU-K seems to be more complicated than we'd actually need.
> It would probably be ok to just do something like:
>
> if (!penultimate_access) return (now - last_access) *
> kSingleAccessAccelerator;
> return (now - penultimate_access);
>
>
> On 2017/06/02 17:40:37, chromium-reviews wrote:
> > Yeah, LRU-2 seems like a pretty good choice, and it's really easy to
> > implement.
> > Might still want to combine it with something like this CL though.
> > So now we have two experiments to run.
> >
> > /Hubbe
> >
> > On Fri, Jun 2, 2017 at 10:31 AM, <mailto:morlovich@chromium.org> wrote:
> >
> > > > The expected result would be that we get more cache hits, but fewer
> bytes
> > > > fetched from the cache.
> > > > (Basically trading off lower latency for more bytes fetched over the
> > > network.)
> > > > Do you guys have metrics that we could use to test that theory?
> > > > If so, I would suggest adding flag for this and then running a finch
> > > experiment.
> > > > If not, we need to start with adding the metrics.
> > >
> > > There is HttpCache.Pattern.*, but I don't know how well they'd reflect
> > > this (and
> > > also
> > > some of the cache metrics may exclude the media cache depending on the
> > > cache
> > > backend...)
> > >
> > >
> > > > Of course, a smarter eviction strategy (ARC perhaps) might give us
> fewer
> > > bytes
> > > > AND more hits.
> > >
> > > The font service folks were lobbying for LRU-2, FWIW, our big
> constraint
> > > for
> > > fancier
> > > replacement is not eating memory.
> > >
> > >
>
>
> https://codereview.chromium.org/2918893002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jkarlin, 2017-06-05 13:00:48]

On 2017/06/02 18:00:38, chromium-reviews wrote:
> PS: I actually think cache-tuning is fun. so if anybody wants to point me
> to where to add the metrics, I can add metrics, run experiments and
> everything. As long as you guys don't mind me doing it that is.
> 
> 
> On Fri, Jun 2, 2017 at 10:55 AM, <mailto:hubbe@chromium.org> wrote:
> 
> > The original LRU-K seems to be more complicated than we'd actually need.
> > It would probably be ok to just do something like:
> >
> > if (!penultimate_access) return (now - last_access) *
> > kSingleAccessAccelerator;
> > return (now - penultimate_access);
> >
> >
> > On 2017/06/02 17:40:37, chromium-reviews wrote:
> > > Yeah, LRU-2 seems like a pretty good choice, and it's really easy to
> > > implement.
> > > Might still want to combine it with something like this CL though.
> > > So now we have two experiments to run.
> > >
> > > /Hubbe
> > >
> > > On Fri, Jun 2, 2017 at 10:31 AM, <mailto:morlovich@chromium.org> wrote:
> > >
> > > > > The expected result would be that we get more cache hits, but fewer
> > bytes
> > > > > fetched from the cache.
> > > > > (Basically trading off lower latency for more bytes fetched over the
> > > > network.)
> > > > > Do you guys have metrics that we could use to test that theory?
> > > > > If so, I would suggest adding flag for this and then running a finch
> > > > experiment.
> > > > > If not, we need to start with adding the metrics.
> > > >
> > > > There is HttpCache.Pattern.*, but I don't know how well they'd reflect
> > > > this (and
> > > > also
> > > > some of the cache metrics may exclude the media cache depending on the
> > > > cache
> > > > backend...)
> > > >
> > > >
> > > > > Of course, a smarter eviction strategy (ARC perhaps) might give us
> > fewer
> > > > bytes
> > > > > AND more hits.
> > > >
> > > > The font service folks were lobbying for LRU-2, FWIW, our big
> > constraint
> > > > for
> > > > fancier
> > > > replacement is not eating memory.
> > > >
> > > >
> >
> >
> > https://codereview.chromium.org/2918893002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

We have HttpCache.Pattern* for hit rate comparison. Use PageLoad.PaintTiming.*
for measuring page load performance and Net.HttpJob.PrefilterBytesRead* for
measuring the ratio of bytes from cache vs network.

Experimenting with caches is slow going. It takes weeks after starting to get
results for one experiment (because you first have to destroy the cache and wait
for it to reach a steady state). And in many cases we can only one run caching
experiment at a time. I don't mean to dissuade you, as I'm happy to see the work
done. I'm just setting expectations.

[hubbe, 2017-06-05 17:26:27]

On 2017/06/05 13:00:48, jkarlin wrote:
> On 2017/06/02 18:00:38, chromium-reviews wrote:
> > PS: I actually think cache-tuning is fun. so if anybody wants to point me
> > to where to add the metrics, I can add metrics, run experiments and
> > everything. As long as you guys don't mind me doing it that is.
> > 
> > 
> > On Fri, Jun 2, 2017 at 10:55 AM, <mailto:hubbe@chromium.org> wrote:
> > 
> > > The original LRU-K seems to be more complicated than we'd actually need.
> > > It would probably be ok to just do something like:
> > >
> > > if (!penultimate_access) return (now - last_access) *
> > > kSingleAccessAccelerator;
> > > return (now - penultimate_access);
> > >
> > >
> > > On 2017/06/02 17:40:37, chromium-reviews wrote:
> > > > Yeah, LRU-2 seems like a pretty good choice, and it's really easy to
> > > > implement.
> > > > Might still want to combine it with something like this CL though.
> > > > So now we have two experiments to run.
> > > >
> > > > /Hubbe
> > > >
> > > > On Fri, Jun 2, 2017 at 10:31 AM, <mailto:morlovich@chromium.org> wrote:
> > > >
> > > > > > The expected result would be that we get more cache hits, but fewer
> > > bytes
> > > > > > fetched from the cache.
> > > > > > (Basically trading off lower latency for more bytes fetched over the
> > > > > network.)
> > > > > > Do you guys have metrics that we could use to test that theory?
> > > > > > If so, I would suggest adding flag for this and then running a finch
> > > > > experiment.
> > > > > > If not, we need to start with adding the metrics.
> > > > >
> > > > > There is HttpCache.Pattern.*, but I don't know how well they'd reflect
> > > > > this (and
> > > > > also
> > > > > some of the cache metrics may exclude the media cache depending on the
> > > > > cache
> > > > > backend...)
> > > > >
> > > > >
> > > > > > Of course, a smarter eviction strategy (ARC perhaps) might give us
> > > fewer
> > > > > bytes
> > > > > > AND more hits.
> > > > >
> > > > > The font service folks were lobbying for LRU-2, FWIW, our big
> > > constraint
> > > > > for
> > > > > fancier
> > > > > replacement is not eating memory.
> > > > >
> > > > >
> > >
> > >
> > > https://codereview.chromium.org/2918893002/
> > >
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> We have HttpCache.Pattern* for hit rate comparison. Use PageLoad.PaintTiming.*
> for measuring page load performance and Net.HttpJob.PrefilterBytesRead* for
> measuring the ratio of bytes from cache vs network.
> 
> Experimenting with caches is slow going. It takes weeks after starting to get
> results for one experiment (because you first have to destroy the cache and
wait
> for it to reach a steady state). And in many cases we can only one run caching
> experiment at a time. I don't mean to dissuade you, as I'm happy to see the
work
> done. I'm just setting expectations.

Ok, so if we have the metrics, I'm going to fix this CL up and put it behind a
flag
so that we can run an experiment.

Weeks doesn't bother me. *everything* takes weeks when you're working on chrome.
:)

[jkarlin, 2017-06-05 17:40:05]

You'll want to check out simple_experiment.h/cc for creating a SimpleCache
experiment. There has only ever been one (and it's currently running,
though I can end it after I record some final data).

On Mon, Jun 5, 2017 at 1:26 PM, <hubbe@chromium.org> wrote:

> On 2017/06/05 13:00:48, jkarlin wrote:
> > On 2017/06/02 18:00:38, chromium-reviews wrote:
> > > PS: I actually think cache-tuning is fun. so if anybody wants to point
> me
> > > to where to add the metrics, I can add metrics, run experiments and
> > > everything. As long as you guys don't mind me doing it that is.
> > >
> > >
> > > On Fri, Jun 2, 2017 at 10:55 AM, <mailto:hubbe@chromium.org> wrote:
> > >
> > > > The original LRU-K seems to be more complicated than we'd actually
> need.
> > > > It would probably be ok to just do something like:
> > > >
> > > > if (!penultimate_access) return (now - last_access) *
> > > > kSingleAccessAccelerator;
> > > > return (now - penultimate_access);
> > > >
> > > >
> > > > On 2017/06/02 17:40:37, chromium-reviews wrote:
> > > > > Yeah, LRU-2 seems like a pretty good choice, and it's really easy
> to
> > > > > implement.
> > > > > Might still want to combine it with something like this CL though.
> > > > > So now we have two experiments to run.
> > > > >
> > > > > /Hubbe
> > > > >
> > > > > On Fri, Jun 2, 2017 at 10:31 AM, <mailto:morlovich@chromium.org>
> wrote:
> > > > >
> > > > > > > The expected result would be that we get more cache hits, but
> fewer
> > > > bytes
> > > > > > > fetched from the cache.
> > > > > > > (Basically trading off lower latency for more bytes fetched
> over the
> > > > > > network.)
> > > > > > > Do you guys have metrics that we could use to test that theory?
> > > > > > > If so, I would suggest adding flag for this and then running a
> finch
> > > > > > experiment.
> > > > > > > If not, we need to start with adding the metrics.
> > > > > >
> > > > > > There is HttpCache.Pattern.*, but I don't know how well they'd
> reflect
> > > > > > this (and
> > > > > > also
> > > > > > some of the cache metrics may exclude the media cache depending
> on the
> > > > > > cache
> > > > > > backend...)
> > > > > >
> > > > > >
> > > > > > > Of course, a smarter eviction strategy (ARC perhaps) might
> give us
> > > > fewer
> > > > > > bytes
> > > > > > > AND more hits.
> > > > > >
> > > > > > The font service folks were lobbying for LRU-2, FWIW, our big
> > > > constraint
> > > > > > for
> > > > > > fancier
> > > > > > replacement is not eating memory.
> > > > > >
> > > > > >
> > > >
> > > >
> > > > https://codereview.chromium.org/2918893002/
> > > >
> > >
> > > --
> > > You received this message because you are subscribed to the Google
> Groups
> > > "Chromium-reviews" group.
> > > To unsubscribe from this group and stop receiving emails from it, send
> an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> > We have HttpCache.Pattern* for hit rate comparison. Use
> PageLoad.PaintTiming.*
> > for measuring page load performance and Net.HttpJob.PrefilterBytesRead*
> for
> > measuring the ratio of bytes from cache vs network.
> >
> > Experimenting with caches is slow going. It takes weeks after starting
> to get
> > results for one experiment (because you first have to destroy the cache
> and
> wait
> > for it to reach a steady state). And in many cases we can only one run
> caching
> > experiment at a time. I don't mean to dissuade you, as I'm happy to see
> the
> work
> > done. I'm just setting expectations.
>
> Ok, so if we have the metrics, I'm going to fix this CL up and put it
> behind a
> flag
> so that we can run an experiment.
>
> Weeks doesn't bother me. *everything* takes weeks when you're working on
> chrome.
> :)
>
>
> https://codereview.chromium.org/2918893002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2017-06-05 18:26:35]

I'm not sure what simple_experiment.cc is for.
I mean, normally I just add a base::Feature and then configure the
experiment to enable that feature.
Am I missing something?


On Mon, Jun 5, 2017 at 10:39 AM, Josh Karlin <jkarlin@chromium.org> wrote:

> You'll want to check out simple_experiment.h/cc for creating a SimpleCache
> experiment. There has only ever been one (and it's currently running,
> though I can end it after I record some final data).
>
> On Mon, Jun 5, 2017 at 1:26 PM, <hubbe@chromium.org> wrote:
>
>> On 2017/06/05 13:00:48, jkarlin wrote:
>> > On 2017/06/02 18:00:38, chromium-reviews wrote:
>> > > PS: I actually think cache-tuning is fun. so if anybody wants to
>> point me
>> > > to where to add the metrics, I can add metrics, run experiments and
>> > > everything. As long as you guys don't mind me doing it that is.
>> > >
>> > >
>> > > On Fri, Jun 2, 2017 at 10:55 AM, <mailto:hubbe@chromium.org> wrote:
>> > >
>> > > > The original LRU-K seems to be more complicated than we'd actually
>> need.
>> > > > It would probably be ok to just do something like:
>> > > >
>> > > > if (!penultimate_access) return (now - last_access) *
>> > > > kSingleAccessAccelerator;
>> > > > return (now - penultimate_access);
>> > > >
>> > > >
>> > > > On 2017/06/02 17:40:37, chromium-reviews wrote:
>> > > > > Yeah, LRU-2 seems like a pretty good choice, and it's really easy
>> to
>> > > > > implement.
>> > > > > Might still want to combine it with something like this CL though.
>> > > > > So now we have two experiments to run.
>> > > > >
>> > > > > /Hubbe
>> > > > >
>> > > > > On Fri, Jun 2, 2017 at 10:31 AM, <mailto:morlovich@chromium.org>
>> wrote:
>> > > > >
>> > > > > > > The expected result would be that we get more cache hits, but
>> fewer
>> > > > bytes
>> > > > > > > fetched from the cache.
>> > > > > > > (Basically trading off lower latency for more bytes fetched
>> over the
>> > > > > > network.)
>> > > > > > > Do you guys have metrics that we could use to test that
>> theory?
>> > > > > > > If so, I would suggest adding flag for this and then running
>> a finch
>> > > > > > experiment.
>> > > > > > > If not, we need to start with adding the metrics.
>> > > > > >
>> > > > > > There is HttpCache.Pattern.*, but I don't know how well they'd
>> reflect
>> > > > > > this (and
>> > > > > > also
>> > > > > > some of the cache metrics may exclude the media cache depending
>> on the
>> > > > > > cache
>> > > > > > backend...)
>> > > > > >
>> > > > > >
>> > > > > > > Of course, a smarter eviction strategy (ARC perhaps) might
>> give us
>> > > > fewer
>> > > > > > bytes
>> > > > > > > AND more hits.
>> > > > > >
>> > > > > > The font service folks were lobbying for LRU-2, FWIW, our big
>> > > > constraint
>> > > > > > for
>> > > > > > fancier
>> > > > > > replacement is not eating memory.
>> > > > > >
>> > > > > >
>> > > >
>> > > >
>> > > > https://codereview.chromium.org/2918893002/
>> > > >
>> > >
>> > > --
>> > > You received this message because you are subscribed to the Google
>> Groups
>> > > "Chromium-reviews" group.
>> > > To unsubscribe from this group and stop receiving emails from it,
>> send an
>> > email
>> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
>> >
>> > We have HttpCache.Pattern* for hit rate comparison. Use
>> PageLoad.PaintTiming.*
>> > for measuring page load performance and Net.HttpJob.PrefilterBytesRead*
>> for
>> > measuring the ratio of bytes from cache vs network.
>> >
>> > Experimenting with caches is slow going. It takes weeks after starting
>> to get
>> > results for one experiment (because you first have to destroy the cache
>> and
>> wait
>> > for it to reach a steady state). And in many cases we can only one run
>> caching
>> > experiment at a time. I don't mean to dissuade you, as I'm happy to see
>> the
>> work
>> > done. I'm just setting expectations.
>>
>> Ok, so if we have the metrics, I'm going to fix this CL up and put it
>> behind a
>> flag
>> so that we can run an experiment.
>>
>> Weeks doesn't bother me. *everything* takes weeks when you're working on
>> chrome.
>> :)
>>
>>
>> https://codereview.chromium.org/2918893002/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hubbe, 2017-06-05 19:31:14]

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

[commit-bot: I haz the power, 2017-06-05 19:31:51]

Dry run: CQ is trying da patch.

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

[hubbe, 2017-06-05 19:33:35]

On 2017/06/05 18:26:35, chromium-reviews wrote:
> I'm not sure what simple_experiment.cc is for.
> I mean, normally I just add a base::Feature and then configure the
> experiment to enable that feature.
> Am I missing something?
> 
> 
> On Mon, Jun 5, 2017 at 10:39 AM, Josh Karlin <mailto:jkarlin@chromium.org>
wrote:
> 
> > You'll want to check out simple_experiment.h/cc for creating a SimpleCache
> > experiment. There has only ever been one (and it's currently running,
> > though I can end it after I record some final data).
> >
> > On Mon, Jun 5, 2017 at 1:26 PM, <mailto:hubbe@chromium.org> wrote:
> >
> >> On 2017/06/05 13:00:48, jkarlin wrote:
> >> > On 2017/06/02 18:00:38, chromium-reviews wrote:
> >> > > PS: I actually think cache-tuning is fun. so if anybody wants to
> >> point me
> >> > > to where to add the metrics, I can add metrics, run experiments and
> >> > > everything. As long as you guys don't mind me doing it that is.
> >> > >
> >> > >
> >> > > On Fri, Jun 2, 2017 at 10:55 AM, <mailto:hubbe@chromium.org> wrote:
> >> > >
> >> > > > The original LRU-K seems to be more complicated than we'd actually
> >> need.
> >> > > > It would probably be ok to just do something like:
> >> > > >
> >> > > > if (!penultimate_access) return (now - last_access) *
> >> > > > kSingleAccessAccelerator;
> >> > > > return (now - penultimate_access);
> >> > > >
> >> > > >
> >> > > > On 2017/06/02 17:40:37, chromium-reviews wrote:
> >> > > > > Yeah, LRU-2 seems like a pretty good choice, and it's really easy
> >> to
> >> > > > > implement.
> >> > > > > Might still want to combine it with something like this CL though.
> >> > > > > So now we have two experiments to run.
> >> > > > >
> >> > > > > /Hubbe
> >> > > > >
> >> > > > > On Fri, Jun 2, 2017 at 10:31 AM, <mailto:morlovich@chromium.org>
> >> wrote:
> >> > > > >
> >> > > > > > > The expected result would be that we get more cache hits, but
> >> fewer
> >> > > > bytes
> >> > > > > > > fetched from the cache.
> >> > > > > > > (Basically trading off lower latency for more bytes fetched
> >> over the
> >> > > > > > network.)
> >> > > > > > > Do you guys have metrics that we could use to test that
> >> theory?
> >> > > > > > > If so, I would suggest adding flag for this and then running
> >> a finch
> >> > > > > > experiment.
> >> > > > > > > If not, we need to start with adding the metrics.
> >> > > > > >
> >> > > > > > There is HttpCache.Pattern.*, but I don't know how well they'd
> >> reflect
> >> > > > > > this (and
> >> > > > > > also
> >> > > > > > some of the cache metrics may exclude the media cache depending
> >> on the
> >> > > > > > cache
> >> > > > > > backend...)
> >> > > > > >
> >> > > > > >
> >> > > > > > > Of course, a smarter eviction strategy (ARC perhaps) might
> >> give us
> >> > > > fewer
> >> > > > > > bytes
> >> > > > > > > AND more hits.
> >> > > > > >
> >> > > > > > The font service folks were lobbying for LRU-2, FWIW, our big
> >> > > > constraint
> >> > > > > > for
> >> > > > > > fancier
> >> > > > > > replacement is not eating memory.
> >> > > > > >
> >> > > > > >
> >> > > >
> >> > > >
> >> > > > https://codereview.chromium.org/2918893002/
> >> > > >
> >> > >
> >> > > --
> >> > > You received this message because you are subscribed to the Google
> >> Groups
> >> > > "Chromium-reviews" group.
> >> > > To unsubscribe from this group and stop receiving emails from it,
> >> send an
> >> > email
> >> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> >> >
> >> > We have HttpCache.Pattern* for hit rate comparison. Use
> >> PageLoad.PaintTiming.*
> >> > for measuring page load performance and Net.HttpJob.PrefilterBytesRead*
> >> for
> >> > measuring the ratio of bytes from cache vs network.
> >> >
> >> > Experimenting with caches is slow going. It takes weeks after starting
> >> to get
> >> > results for one experiment (because you first have to destroy the cache
> >> and
> >> wait
> >> > for it to reach a steady state). And in many cases we can only one run
> >> caching
> >> > experiment at a time. I don't mean to dissuade you, as I'm happy to see
> >> the
> >> work
> >> > done. I'm just setting expectations.
> >>
> >> Ok, so if we have the metrics, I'm going to fix this CL up and put it
> >> behind a
> >> flag
> >> so that we can run an experiment.
> >>
> >> Weeks doesn't bother me. *everything* takes weeks when you're working on
> >> chrome.
> >> :)
> >>
> >>
> >> https://codereview.chromium.org/2918893002/
> >>
> >
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Ok, so I put everything behind a flag, and I reverted the change to actually
allow larger files for now.
(Allowing larger files should probably be a separate experiment.)
I almost decided to implement LRU2 as well, but I realized that I didn't know
how to handle pickling the extra value in a backwards/forwards-compatible way.

[hubbe, 2017-06-05 19:33:51]

Description was changed from

==========
allow caching larger files and evict larger entries first

BUG=700102
==========

to

==========
evict larger entries first

BUG=700102
==========

[Maks Orlovich, 2017-06-05 20:14:52]

> Ok, so I put everything behind a flag, and I reverted the change to actually
> allow larger files for now.
> (Allowing larger files should probably be a separate experiment.)
> I almost decided to implement LRU2 as well, but I realized that I didn't know
> how to handle pickling the extra value in a backwards/forwards-compatible way.

The big question is whether it can be done without increasing memory usage
somehow, and if
not, if the memory use is worth it. WRT to pickling, the diff between revisions
2 and 1 of 
https://codereview.chromium.org/2922973003/ should offer some idea (though that
only changes
unpickling)

[hubbe, 2017-06-05 21:56:31]

On 2017/06/05 20:14:52, Maks Orlovich wrote:
> > Ok, so I put everything behind a flag, and I reverted the change to actually
> > allow larger files for now.
> > (Allowing larger files should probably be a separate experiment.)
> > I almost decided to implement LRU2 as well, but I realized that I didn't
know
> > how to handle pickling the extra value in a backwards/forwards-compatible
way.
> 
> The big question is whether it can be done without increasing memory usage
> somehow, and if
> not, if the memory use is worth it. WRT to pickling, the diff between
revisions
> 2 and 1 of 
> https://codereview.chromium.org/2922973003/ should offer some idea (though
that
> only changes
> unpickling)

Hmm, I wonder. The current approach in this CL basically uses (now - last_used)
* size.
However, that means that large files from a previous session would be punished a
LOT more
than what is reasonable.

Perhaps I should instead make two sort passes, first sort by last used, and then
multiply the *rank* by the size and sort again. The idea being that it's the
order
of usage that counts, not the actual time frame.

What do you guys think?

[hubbe, 2017-06-05 22:56:31]

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

[commit-bot: I haz the power, 2017-06-05 22:57:25]

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, 2017-06-06 01:10:05]

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

[commit-bot: I haz the power, 2017-06-06 01:10:06]

Dry run: This issue passed the CQ dry run.

[jkarlin, 2017-06-06 14:09:13]

> I'm not sure what simple_experiment.cc is for.
> I mean, normally I just add a base::Feature and then configure the
> experiment to enable that feature.
> Am I missing something?

simple_experiment is for clean-slate cache experiments. When joining the
experiment (the finch feature is enabled) the cache is cleared and the
SimpleExperimentType is written in its metadata. When exiting the experiment the
cache is once again cleared and returned to a vanilla cache
(SimpleExperimentType::None). This let's you have an apples-to-apples control vs
experiment study where clients in both are starting from brand new caches with
the intended behavior. It's important to use persistent finch experiments rather
than session experiments here.

[hubbe, 2017-06-06 18:14:53]

On 2017/06/06 14:09:13, jkarlin wrote:
> > I'm not sure what simple_experiment.cc is for.
> > I mean, normally I just add a base::Feature and then configure the
> > experiment to enable that feature.
> > Am I missing something?
> 
> simple_experiment is for clean-slate cache experiments. When joining the
> experiment (the finch feature is enabled) the cache is cleared and the
> SimpleExperimentType is written in its metadata. When exiting the experiment
the
> cache is once again cleared and returned to a vanilla cache
> (SimpleExperimentType::None). This let's you have an apples-to-apples control
vs
> experiment study where clients in both are starting from brand new caches with
> the intended behavior. It's important to use persistent finch experiments
rather
> than session experiments here.

So, do you think we need clean-slate experiments here?
The drawback would seem to be that overlapping experiments doesn't really work.
(I was going to make one 50/50 canary/dev "with size" experiment, and another
50/50 canary/dev LRU2 experiment.)
It seems to me that you get data quicker if you clear the cache. If you don't
clear the cache you have to wait
some time to let the cache state fade into the noise.

[jkarlin, 2017-06-07 18:58:22]

On 2017/06/06 18:14:53, hubbe wrote:
> On 2017/06/06 14:09:13, jkarlin wrote:
> > > I'm not sure what simple_experiment.cc is for.
> > > I mean, normally I just add a base::Feature and then configure the
> > > experiment to enable that feature.
> > > Am I missing something?
> > 
> > simple_experiment is for clean-slate cache experiments. When joining the
> > experiment (the finch feature is enabled) the cache is cleared and the
> > SimpleExperimentType is written in its metadata. When exiting the experiment
> the
> > cache is once again cleared and returned to a vanilla cache
> > (SimpleExperimentType::None). This let's you have an apples-to-apples
control
> vs
> > experiment study where clients in both are starting from brand new caches
with
> > the intended behavior. It's important to use persistent finch experiments
> rather
> > than session experiments here.
> 
> So, do you think we need clean-slate experiments here?
> The drawback would seem to be that overlapping experiments doesn't really
work.
> (I was going to make one 50/50 canary/dev "with size" experiment, and another
> 50/50 canary/dev LRU2 experiment.)
> It seems to me that you get data quicker if you clear the cache. If you don't
> clear the cache you have to wait
> some time to let the cache state fade into the noise.

The question is how long do you need to wait until you're sure? Better to start
clean and know your data is good.

Also, I'm concerned about how the two proposed 50/50 experiments would interact
if you ran them together.

[hubbe, 2017-06-07 19:38:44]

On 2017/06/07 18:58:22, jkarlin wrote:
> On 2017/06/06 18:14:53, hubbe wrote:
> > On 2017/06/06 14:09:13, jkarlin wrote:
> > > > I'm not sure what simple_experiment.cc is for.
> > > > I mean, normally I just add a base::Feature and then configure the
> > > > experiment to enable that feature.
> > > > Am I missing something?
> > > 
> > > simple_experiment is for clean-slate cache experiments. When joining the
> > > experiment (the finch feature is enabled) the cache is cleared and the
> > > SimpleExperimentType is written in its metadata. When exiting the
experiment
> > the
> > > cache is once again cleared and returned to a vanilla cache
> > > (SimpleExperimentType::None). This let's you have an apples-to-apples
> control
> > vs
> > > experiment study where clients in both are starting from brand new caches
> with
> > > the intended behavior. It's important to use persistent finch experiments
> > rather
> > > than session experiments here.
> > 
> > So, do you think we need clean-slate experiments here?
> > The drawback would seem to be that overlapping experiments doesn't really
> work.
> > (I was going to make one 50/50 canary/dev "with size" experiment, and
another
> > 50/50 canary/dev LRU2 experiment.)
> > It seems to me that you get data quicker if you clear the cache. If you
don't
> > clear the cache you have to wait
> > some time to let the cache state fade into the noise.
>
> The question is how long do you need to wait until you're sure? Better to
start
> clean and know your data is good.
> 

It should be pretty obvious from the graphs. I mean, we don't care about
absolute
numbers, just better/worse. Since these changes are fairly radical, I would
expect
changes to be fairly radical as well.

In fact, I would argue that if we empty the cache, it might take longer to get
data since cache eviction doesn't run until the cache is full... (Not sure how
long that takes on average though.)

> Also, I'm concerned about how the two proposed 50/50 experiments would
interact
> if you ran them together.

I could run one experiment that uses four 25%-groups instead.
Only difference would be how the data is presented though.

[jkarlin, 2017-06-07 19:46:08]

On 2017/06/07 19:38:44, hubbe wrote:
> On 2017/06/07 18:58:22, jkarlin wrote:
> > On 2017/06/06 18:14:53, hubbe wrote:
> > > On 2017/06/06 14:09:13, jkarlin wrote:
> > > > > I'm not sure what simple_experiment.cc is for.
> > > > > I mean, normally I just add a base::Feature and then configure the
> > > > > experiment to enable that feature.
> > > > > Am I missing something?
> > > > 
> > > > simple_experiment is for clean-slate cache experiments. When joining the
> > > > experiment (the finch feature is enabled) the cache is cleared and the
> > > > SimpleExperimentType is written in its metadata. When exiting the
> experiment
> > > the
> > > > cache is once again cleared and returned to a vanilla cache
> > > > (SimpleExperimentType::None). This let's you have an apples-to-apples
> > control
> > > vs
> > > > experiment study where clients in both are starting from brand new
caches
> > with
> > > > the intended behavior. It's important to use persistent finch
experiments
> > > rather
> > > > than session experiments here.
> > > 
> > > So, do you think we need clean-slate experiments here?
> > > The drawback would seem to be that overlapping experiments doesn't really
> > work.
> > > (I was going to make one 50/50 canary/dev "with size" experiment, and
> another
> > > 50/50 canary/dev LRU2 experiment.)
> > > It seems to me that you get data quicker if you clear the cache. If you
> don't
> > > clear the cache you have to wait
> > > some time to let the cache state fade into the noise.
> >
> > The question is how long do you need to wait until you're sure? Better to
> start
> > clean and know your data is good.
> > 
> 
> It should be pretty obvious from the graphs. I mean, we don't care about
> absolute
> numbers, just better/worse. Since these changes are fairly radical, I would
> expect
> changes to be fairly radical as well.

I'm willing to give it a shot and transition to clean-cache if the results are
unclear.

> 
> In fact, I would argue that if we empty the cache, it might take longer to get
> data since cache eviction doesn't run until the cache is full... (Not sure how
> long that takes on average though.)
> 
> > Also, I'm concerned about how the two proposed 50/50 experiments would
> interact
> > if you ran them together.
> 

> I could run one experiment that uses four 25%-groups instead.
> Only difference would be how the data is presented though.

Yes, let's do the 25% groups.

[jkarlin, 2017-06-08 14:32:49]

https://codereview.chromium.org/2918893002/diff/60001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/2918893002/diff/60001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_index.cc:300: struct EvictionSortHelper {
Struct needs comment

https://codereview.chromium.org/2918893002/diff/60001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_index.cc:308:
EntryMetadata::kEstimatedEntryOverhead);
The kEstimatedEntryOverhead bit is strange.

Each sort_value_ has (rank * kEstimatedEntryOverhead) added to it.

You could divide kEstimatedEntryOverhead out of each sort_value_ without
affecting the order, which means that this is effectively:

sort_value_ = rank + rank*entry->second.GetEntrySize();

https://codereview.chromium.org/2918893002/diff/60001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_index.cc:342: return a.SortValue1() <
b.SortValue1();
If you use > here instead of < then you don't need the entries.sizes()-i bit
below, which is a bit clearer.

[hubbe, 2017-06-08 17:43:39]

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

[commit-bot: I haz the power, 2017-06-08 17:44:14]

Dry run: CQ is trying da patch.

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

[hubbe, 2017-06-08 17:45:03]

https://codereview.chromium.org/2918893002/diff/60001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/2918893002/diff/60001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_index.cc:300: struct EvictionSortHelper {
On 2017/06/08 14:32:49, jkarlin wrote:
> Struct needs comment

Done.

https://codereview.chromium.org/2918893002/diff/60001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_index.cc:308:
EntryMetadata::kEstimatedEntryOverhead);
On 2017/06/08 14:32:49, jkarlin wrote:
> The kEstimatedEntryOverhead bit is strange.
> 
> Each sort_value_ has (rank * kEstimatedEntryOverhead) added to it.
> 
> You could divide kEstimatedEntryOverhead out of each sort_value_ without
> affecting the order, which means that this is effectively:
> 
> sort_value_ = rank + rank*entry->second.GetEntrySize();

Eh? No?
sv = rank * (size + overhead) =>
sv = rank * size + rank * overhead =>
sv / overhead = rank * size / overhead + rank

https://codereview.chromium.org/2918893002/diff/60001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_index.cc:342: return a.SortValue1() <
b.SortValue1();
On 2017/06/08 14:32:49, jkarlin wrote:
> If you use > here instead of < then you don't need the entries.sizes()-i bit
> below, which is a bit clearer.

That won't work when kSimpleCacheEvictionWithSize is not enabled though. But it
can be cleaned up later if we decide that kSimpleCacheEvictionWithSize should
always be on.

[jkarlin, 2017-06-08 18:18:53]

https://codereview.chromium.org/2918893002/diff/60001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/2918893002/diff/60001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_index.cc:308:
EntryMetadata::kEstimatedEntryOverhead);
On 2017/06/08 17:45:03, hubbe wrote:
> On 2017/06/08 14:32:49, jkarlin wrote:
> > The kEstimatedEntryOverhead bit is strange.
> > 
> > Each sort_value_ has (rank * kEstimatedEntryOverhead) added to it.
> > 
> > You could divide kEstimatedEntryOverhead out of each sort_value_ without
> > affecting the order, which means that this is effectively:
> > 
> > sort_value_ = rank + rank*entry->second.GetEntrySize();
> 
> Eh? No?
> sv = rank * (size + overhead) =>
> sv = rank * size + rank * overhead =>
> sv / overhead = rank * size / overhead + rank

Whoops :) Still, where does the size of overhead come from?

https://codereview.chromium.org/2918893002/diff/60001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_index.cc:342: return a.SortValue1() <
b.SortValue1();
On 2017/06/08 17:45:03, hubbe wrote:
> On 2017/06/08 14:32:49, jkarlin wrote:
> > If you use > here instead of < then you don't need the entries.sizes()-i bit
> > below, which is a bit clearer.
> 
> That won't work when kSimpleCacheEvictionWithSize is not enabled though. But
it
> can be cleaned up later if we decide that kSimpleCacheEvictionWithSize should
> always be on.
> 

Acknowledged.

[hubbe, 2017-06-08 18:23:36]

https://codereview.chromium.org/2918893002/diff/60001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/2918893002/diff/60001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_index.cc:308:
EntryMetadata::kEstimatedEntryOverhead);
On 2017/06/08 18:18:53, jkarlin wrote:
> On 2017/06/08 17:45:03, hubbe wrote:
> > On 2017/06/08 14:32:49, jkarlin wrote:
> > > The kEstimatedEntryOverhead bit is strange.
> > > 
> > > Each sort_value_ has (rank * kEstimatedEntryOverhead) added to it.
> > > 
> > > You could divide kEstimatedEntryOverhead out of each sort_value_ without
> > > affecting the order, which means that this is effectively:
> > > 
> > > sort_value_ = rank + rank*entry->second.GetEntrySize();
> > 
> > Eh? No?
> > sv = rank * (size + overhead) =>
> > sv = rank * size + rank * overhead =>
> > sv / overhead = rank * size / overhead + rank
> 
> Whoops :) Still, where does the size of overhead come from?

The overhead comes from the block size of most devices.
However, it also serves a separate purpose: It moderates the effect of the size
for small files. The idea being that saving a 1-byte file twice as long as a
2-byte file makes no sense.

Maybe I should write a design doc for this change too?

[commit-bot: I haz the power, 2017-06-08 19:10:07]

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

[commit-bot: I haz the power, 2017-06-08 19:10:08]

Dry run: This issue passed the CQ dry run.

[jkarlin, 2017-06-09 15:31:34]

On 2017/06/08 18:23:36, hubbe wrote:
>
https://codereview.chromium.org/2918893002/diff/60001/net/disk_cache/simple/s...
> File net/disk_cache/simple/simple_index.cc (right):
> 
>
https://codereview.chromium.org/2918893002/diff/60001/net/disk_cache/simple/s...
> net/disk_cache/simple/simple_index.cc:308:
> EntryMetadata::kEstimatedEntryOverhead);
> On 2017/06/08 18:18:53, jkarlin wrote:
> > On 2017/06/08 17:45:03, hubbe wrote:
> > > On 2017/06/08 14:32:49, jkarlin wrote:
> > > > The kEstimatedEntryOverhead bit is strange.
> > > > 
> > > > Each sort_value_ has (rank * kEstimatedEntryOverhead) added to it.
> > > > 
> > > > You could divide kEstimatedEntryOverhead out of each sort_value_ without
> > > > affecting the order, which means that this is effectively:
> > > > 
> > > > sort_value_ = rank + rank*entry->second.GetEntrySize();
> > > 
> > > Eh? No?
> > > sv = rank * (size + overhead) =>
> > > sv = rank * size + rank * overhead =>
> > > sv / overhead = rank * size / overhead + rank
> > 
> > Whoops :) Still, where does the size of overhead come from?
> 
> The overhead comes from the block size of most devices.
> However, it also serves a separate purpose: It moderates the effect of the
size
> for small files. The idea being that saving a 1-byte file twice as long as a
> 2-byte file makes no sense.
> 
> Maybe I should write a design doc for this change too?

Huh, I thought I responded to this yesterday.

Anyway, yes a design doc would be good. It'd be nice to see related work on how
other caches deal with evicting larger resources first and why you chose this
particular approach.

[hubbe, 2017-06-09 17:34:01]

Description was changed from

==========
evict larger entries first

BUG=700102
==========

to

==========
evict larger entries first

Design doc:
https://docs.google.com/a/chromium.org/document/d/1QwSsevekzxQI1gAGEonTnYoVnY...

BUG=700102
==========

[hubbe, 2017-06-09 17:34:31]

On 2017/06/09 15:31:34, jkarlin wrote:
> On 2017/06/08 18:23:36, hubbe wrote:
> >
>
https://codereview.chromium.org/2918893002/diff/60001/net/disk_cache/simple/s...
> > File net/disk_cache/simple/simple_index.cc (right):
> > 
> >
>
https://codereview.chromium.org/2918893002/diff/60001/net/disk_cache/simple/s...
> > net/disk_cache/simple/simple_index.cc:308:
> > EntryMetadata::kEstimatedEntryOverhead);
> > On 2017/06/08 18:18:53, jkarlin wrote:
> > > On 2017/06/08 17:45:03, hubbe wrote:
> > > > On 2017/06/08 14:32:49, jkarlin wrote:
> > > > > The kEstimatedEntryOverhead bit is strange.
> > > > > 
> > > > > Each sort_value_ has (rank * kEstimatedEntryOverhead) added to it.
> > > > > 
> > > > > You could divide kEstimatedEntryOverhead out of each sort_value_
without
> > > > > affecting the order, which means that this is effectively:
> > > > > 
> > > > > sort_value_ = rank + rank*entry->second.GetEntrySize();
> > > > 
> > > > Eh? No?
> > > > sv = rank * (size + overhead) =>
> > > > sv = rank * size + rank * overhead =>
> > > > sv / overhead = rank * size / overhead + rank
> > > 
> > > Whoops :) Still, where does the size of overhead come from?
> > 
> > The overhead comes from the block size of most devices.
> > However, it also serves a separate purpose: It moderates the effect of the
> size
> > for small files. The idea being that saving a 1-byte file twice as long as a
> > 2-byte file makes no sense.
> > 
> > Maybe I should write a design doc for this change too?
> 
> Huh, I thought I responded to this yesterday.
> 
> Anyway, yes a design doc would be good. It'd be nice to see related work on
how
> other caches deal with evicting larger resources first and why you chose this
> particular approach.

Brief design doc added to CL description.

[hubbe, 2017-06-21 22:39:28]

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

[commit-bot: I haz the power, 2017-06-21 22:39:50]

Dry run: CQ is trying da patch.

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

[hubbe, 2017-06-21 22:46:28]

Description was changed from

==========
evict larger entries first

Design doc:
https://docs.google.com/a/chromium.org/document/d/1QwSsevekzxQI1gAGEonTnYoVnY...

BUG=700102
==========

to

==========
evict larger entries first

Design doc:
https://docs.google.com/document/d/1N07uwDiOipRcMG3SanxR8X8flvxrh9Axi1SLpfRNK...

BUG=700102
==========

[hubbe, 2017-06-21 22:49:40]

hubbe@chromium.org changed reviewers:
+ morlovich@chromium.org, pasko@chromium.org

[hubbe, 2017-06-21 22:49:40]

Based on one-off research (linked in the design doc) I have dusted off, cleaned
and updated this CL, as cache eviction based on cache size seems to be more
rewarding than any of the other algorithms I tried. (Including LRU2)

[commit-bot: I haz the power, 2017-06-22 00:08:25]

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

[commit-bot: I haz the power, 2017-06-22 00:08:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Maks Orlovich, 2017-06-22 16:30:51]

LGTM modulo details, but I have no approval rights anyway.

https://codereview.chromium.org/2918893002/diff/100001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/2918893002/diff/100001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:55: static const int
kEstimatedEntryOverhead = 512;
FWIW, GetEntrySize is the size of the file, not of payload (though it doesn't
include things like block overhead and FS costs of tracking it)

https://codereview.chromium.org/2918893002/diff/100001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:328: // Subtract so we don't need a custom
comparitor.
typo, s/comparitor/comparator/

https://codereview.chromium.org/2918893002/diff/100001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:387: if (evicted_so_far_size >
amount_to_evict)
>=, I think?

[hubbe, 2017-06-22 17:35:41]

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

[commit-bot: I haz the power, 2017-06-22 17:35:56]

Dry run: CQ is trying da patch.

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

[hubbe, 2017-06-22 17:36:11]

https://codereview.chromium.org/2918893002/diff/100001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/2918893002/diff/100001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:55: static const int
kEstimatedEntryOverhead = 512;
On 2017/06/22 16:30:51, Maks Orlovich wrote:
> FWIW, GetEntrySize is the size of the file, not of payload (though it doesn't
> include things like block overhead and FS costs of tracking it)

Updated comment.

https://codereview.chromium.org/2918893002/diff/100001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:328: // Subtract so we don't need a custom
comparitor.
On 2017/06/22 16:30:51, Maks Orlovich wrote:
> typo, s/comparitor/comparator/

Done.

https://codereview.chromium.org/2918893002/diff/100001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:387: if (evicted_so_far_size >
amount_to_evict)
On 2017/06/22 16:30:51, Maks Orlovich wrote:
> >=, I think?

Done.

[hubbe, 2017-06-22 17:37:00]

On 2017/06/22 17:36:11, hubbe wrote:
>
https://codereview.chromium.org/2918893002/diff/100001/net/disk_cache/simple/...
> File net/disk_cache/simple/simple_index.cc (right):
> 
>
https://codereview.chromium.org/2918893002/diff/100001/net/disk_cache/simple/...
> net/disk_cache/simple/simple_index.cc:55: static const int
> kEstimatedEntryOverhead = 512;
> On 2017/06/22 16:30:51, Maks Orlovich wrote:
> > FWIW, GetEntrySize is the size of the file, not of payload (though it
doesn't
> > include things like block overhead and FS costs of tracking it)
> 
> Updated comment.
> 
>
https://codereview.chromium.org/2918893002/diff/100001/net/disk_cache/simple/...
> net/disk_cache/simple/simple_index.cc:328: // Subtract so we don't need a
custom
> comparitor.
> On 2017/06/22 16:30:51, Maks Orlovich wrote:
> > typo, s/comparitor/comparator/
> 
> Done.
> 
>
https://codereview.chromium.org/2918893002/diff/100001/net/disk_cache/simple/...
> net/disk_cache/simple/simple_index.cc:387: if (evicted_so_far_size >
> amount_to_evict)
> On 2017/06/22 16:30:51, Maks Orlovich wrote:
> > >=, I think?
> 
> Done.

How long is jkarlin out for?
Should I wait for him to come back or find someone else who can approve?

[Maks Orlovich, 2017-06-22 17:43:44]

LGTM

> How long is jkarlin out for?
> Should I wait for him to come back or find someone else who can approve?

Just this week, and pasko@ has approve bits for net/disk_cache/simple as well.

[commit-bot: I haz the power, 2017-06-22 19:12:45]

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

[commit-bot: I haz the power, 2017-06-22 19:12:46]

Dry run: This issue passed the CQ dry run.

[pasko, 2017-06-23 15:10:31]

Exciting! Thank you for doing this. Though I would like to go through a few
formalities first.

I left a few comments in the design document. Please address them.

Also please create a launch bug ("simple launch" template) and describe how you
want to experiment. We can help you with determining how to answer questions in
the template (like what metrics to look at, etc etc) - feel free to ask.

I would prefer to have the above two things first, and land this change only
after we have them.

https://codereview.chromium.org/2918893002/diff/120001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_experiment.cc (right):

https://codereview.chromium.org/2918893002/diff/120001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:16: const base::Feature
kSimpleSizeExperiment = {"SimpleSizeExperiment",
maybe allow a param like log/sqrt/whatever that you initially considered? Given
that we are running an experiment anyway, it would be a cheap way to discover
whether typical uses are matching the data you experimented with locally.

I'm OK with doing it in a later change.

https://codereview.chromium.org/2918893002/diff/120001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:19: "SimpleCacheEvictionWithSize",
base::FEATURE_DISABLED_BY_DEFAULT};
does finch config allow enabling more than one feature and providing params for
each one of them?

I think in order for this to allow dropping caches in a no-op group, we would
either need to enable SimpleSizeExperiment or use the similar logic as
CheckForSimpleSizeExperiment/SimpleExperimentMatches.

[hubbe, 2017-06-23 18:24:21]

Description was changed from

==========
evict larger entries first

Design doc:
https://docs.google.com/document/d/1N07uwDiOipRcMG3SanxR8X8flvxrh9Axi1SLpfRNK...

BUG=700102
==========

to

==========
evict larger entries first

Design doc:
https://docs.google.com/document/d/1N07uwDiOipRcMG3SanxR8X8flvxrh9Axi1SLpfRNK...

BUG=700102, 736437
==========

[hubbe, 2017-06-23 18:25:52]

Updated design doc.
Created Launch Bug (736437)
Replied to comments.

https://codereview.chromium.org/2918893002/diff/120001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_experiment.cc (right):

https://codereview.chromium.org/2918893002/diff/120001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:16: const base::Feature
kSimpleSizeExperiment = {"SimpleSizeExperiment",
On 2017/06/23 15:10:30, pasko wrote:
> maybe allow a param like log/sqrt/whatever that you initially considered?
Given
> that we are running an experiment anyway, it would be a cheap way to discover
> whether typical uses are matching the data you experimented with locally.
> 
> I'm OK with doing it in a later change.

log & sqrt are too slow, don't make mathematical sense and they don't performed
well in my experiments so far. I'm not going to try that unless there is some
reason suggesting that it would be better.

https://codereview.chromium.org/2918893002/diff/120001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:19: "SimpleCacheEvictionWithSize",
base::FEATURE_DISABLED_BY_DEFAULT};
On 2017/06/23 15:10:30, pasko wrote:
> does finch config allow enabling more than one feature and providing params
for
> each one of them?
> 
> I think in order for this to allow dropping caches in a no-op group, we would
> either need to enable SimpleSizeExperiment or use the similar logic as
> CheckForSimpleSizeExperiment/SimpleExperimentMatches.

I don't intend to drop caches in the finch experiment.
If I did, I would use the SimpleExperiment logic in this file to do that.

[pasko, 2017-06-26 13:32:13]

https://codereview.chromium.org/2918893002/diff/120001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_experiment.cc (right):

https://codereview.chromium.org/2918893002/diff/120001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:19: "SimpleCacheEvictionWithSize",
base::FEATURE_DISABLED_BY_DEFAULT};
On 2017/06/23 18:25:52, hubbe wrote:
> On 2017/06/23 15:10:30, pasko wrote:
> > does finch config allow enabling more than one feature and providing params
> for
> > each one of them?
> > 
> > I think in order for this to allow dropping caches in a no-op group, we
would
> > either need to enable SimpleSizeExperiment or use the similar logic as
> > CheckForSimpleSizeExperiment/SimpleExperimentMatches.
> 
> I don't intend to drop caches in the finch experiment.
> If I did, I would use the SimpleExperiment logic in this file to do that.

Thank you. I have a few reservations about it, so I started a discussion in the
document.

not lgtm until we reach agreement in the discussion, feel free to throw a VC to
figure out what works for us.

[hubbe, 2017-06-28 22:46:20]

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

[commit-bot: I haz the power, 2017-06-28 22:47:01]

Dry run: CQ is trying da patch.

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

[hubbe, 2017-06-28 22:48:52]

Updated to use simple_experiment to clear cache when entering/exiting the
experiment.

[commit-bot: I haz the power, 2017-06-29 00:36:50]

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

[commit-bot: I haz the power, 2017-06-29 00:36:51]

Dry run: This issue passed the CQ dry run.

[pasko, 2017-06-29 15:27:25]

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_experiment.cc (right):

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:26: void
CheckForSimpleCacheEvictionWithSizeExperiment(
nit: "SimpleCache" looks redundant here, I'd suggest s/SimpleCache//

Another option is just to remove "Simple" to be consistent with
"CheckForSimpleSizeExperiment", but I'd better remove "Simple" from the latter.

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:31: if
(base::FeatureList::GetFieldTrial(kSimpleCacheEvictionWithSize)) {
nit: the common convention is not to put curly braces around a single line

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:74:
CheckForSimpleSizeExperiment(&experiment);
jkarlin: why are we ignoring the return value here?

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:75:
CheckForSimpleCacheEvictionWithSizeExperiment(&experiment);
this has the ability to drop the cache in the experiment group, but afaict it
cannot drop cache in the control group. Is it intentional?

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:321: typedef std::pair<const uint64_t,
EntryMetadata> Entry;
Entry is an overloaded term. Maybe HashAndEntryPair?

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:323: std::vector<SortHelper> entries;
const SortHelper?

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:331: sort_value *=
i->second.GetEntrySize() + kEstimatedEntryOverhead;
nit: maybe worth a comment to say that it would not overflow because entry size
is topped at 2GiB and the time is in seconds? okay, maybe that's obvious..

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:346: // std::sort() to avoid N-squared
worst case scenario.
If it is only marginally faster than std::sort, I would prefer the latter.

[jkarlin, 2017-06-29 16:26:12]

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_experiment.cc (right):

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:32: experiment->type =
SimpleExperimentType::EVICT_WITH_SIZE;
You should also parse the parameter and use the parameter as your enable/disable
switch. That way both control and experiment have EVICT_WITH_SIZE cache types
(causing the caches to reset) but only if the param is 1 will you actually
enable the feature.

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:74:
CheckForSimpleSizeExperiment(&experiment);
On 2017/06/29 15:27:25, pasko wrote:
> jkarlin: why are we ignoring the return value here?

You're right, we should early return if it returns true.

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:75:
CheckForSimpleCacheEvictionWithSizeExperiment(&experiment);
On 2017/06/29 15:27:24, pasko wrote:
> this has the ability to drop the cache in the experiment group, but afaict it
> cannot drop cache in the control group. Is it intentional?

Discussed in a comment above.

[hubbe, 2017-06-29 18:55:27]

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_experiment.cc (right):

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:26: void
CheckForSimpleCacheEvictionWithSizeExperiment(
On 2017/06/29 15:27:25, pasko (OOO) wrote:
> nit: "SimpleCache" looks redundant here, I'd suggest s/SimpleCache//
> 
> Another option is just to remove "Simple" to be consistent with
> "CheckForSimpleSizeExperiment", but I'd better remove "Simple" from the
latter.

Done.

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:31: if
(base::FeatureList::GetFieldTrial(kSimpleCacheEvictionWithSize)) {
On 2017/06/29 15:27:25, pasko (OOO) wrote:
> nit: the common convention is not to put curly braces around a single line

Done.

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:32: experiment->type =
SimpleExperimentType::EVICT_WITH_SIZE;
On 2017/06/29 16:26:11, jkarlin_slow wrote:
> You should also parse the parameter and use the parameter as your
enable/disable
> switch. That way both control and experiment have EVICT_WITH_SIZE cache types
> (causing the caches to reset) but only if the param is 1 will you actually
> enable the feature.

My intention was to have the control group disable the
SimpleCacheEvictionWithSize feature so that the experiment is associated with
the feature for both groups.

If I follow your suggestion, how do I enable the new cache eviction from the
command line?

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:75:
CheckForSimpleCacheEvictionWithSizeExperiment(&experiment);
On 2017/06/29 15:27:24, pasko (OOO) wrote:
> this has the ability to drop the cache in the experiment group, but afaict it
> cannot drop cache in the control group. Is it intentional?

See comment on line 32.

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:75:
CheckForSimpleCacheEvictionWithSizeExperiment(&experiment);
On 2017/06/29 16:26:11, jkarlin_slow wrote:
> On 2017/06/29 15:27:24, pasko wrote:
> > this has the ability to drop the cache in the experiment group, but afaict
it
> > cannot drop cache in the control group. Is it intentional?
> 
> Discussed in a comment above.

Acknowledged.

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:321: typedef std::pair<const uint64_t,
EntryMetadata> Entry;
On 2017/06/29 15:27:25, pasko (OOO) wrote:
> Entry is an overloaded term. Maybe HashAndEntryPair?

I think I'll just replace it with EntrySet::value_type

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:323: std::vector<SortHelper> entries;
On 2017/06/29 15:27:25, pasko (OOO) wrote:
> const SortHelper?

Creates a long list of errors when I try to compile.
Not sure why.

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:331: sort_value *=
i->second.GetEntrySize() + kEstimatedEntryOverhead;
On 2017/06/29 15:27:25, pasko (OOO) wrote:
> nit: maybe worth a comment to say that it would not overflow because entry
size
> is topped at 2GiB and the time is in seconds? okay, maybe that's obvious..

Might reduce reader friction slightly, so I'll put it in.

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:346: // std::sort() to avoid N-squared
worst case scenario.
On 2017/06/29 15:27:25, pasko (OOO) wrote:
> If it is only marginally faster than std::sort, I would prefer the latter.

Depends on what you mean by marginal.
The introselect algorithm is linear on average, so the more entries that are in
the cache, the more we gain from it. Since evicting by size will keep more
entries in the cache, I felt that this might be important.

[jkarlin, 2017-06-29 19:17:01]

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_experiment.cc (right):

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:32: experiment->type =
SimpleExperimentType::EVICT_WITH_SIZE;
On 2017/06/29 18:55:26, hubbe wrote:
> On 2017/06/29 16:26:11, jkarlin_slow wrote:
> > You should also parse the parameter and use the parameter as your
> enable/disable
> > switch. That way both control and experiment have EVICT_WITH_SIZE cache
types
> > (causing the caches to reset) but only if the param is 1 will you actually
> > enable the feature.
> 
> My intention was to have the control group disable the
> SimpleCacheEvictionWithSize feature so that the experiment is associated with
> the feature for both groups.

But then you wouldn't clear the control's cache? Seems better to clear it to get
apples-to-apples comparisons.

> 
> If I follow your suggestion, how do I enable the new cache eviction from the
> command line?

According to the experiment params portion of the field trial docs:

chrome.exe --enable-features="MyFeatureName<MyStudyName"
  --force-fieldtrials=MyStudyName/Group1
  --force-fieldtrial-params=MyStudyName.Group1:x/100/y/Test

[hubbe, 2017-06-29 19:26:48]

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_experiment.cc (right):

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:32: experiment->type =
SimpleExperimentType::EVICT_WITH_SIZE;
On 2017/06/29 19:17:01, jkarlin_slow wrote:
> On 2017/06/29 18:55:26, hubbe wrote:
> > On 2017/06/29 16:26:11, jkarlin_slow wrote:
> > > You should also parse the parameter and use the parameter as your
> > enable/disable
> > > switch. That way both control and experiment have EVICT_WITH_SIZE cache
> types
> > > (causing the caches to reset) but only if the param is 1 will you actually
> > > enable the feature.
> > 
> > My intention was to have the control group disable the
> > SimpleCacheEvictionWithSize feature so that the experiment is associated
with
> > the feature for both groups.
> 
> But then you wouldn't clear the control's cache?

I don't understand, why wouldn't it?
My intention was to have the cache cleared for both groups.
Wouldn't the experiment be associated with the feature for the control group as
well?

> Seems better to clear it to get
> apples-to-apples comparisons.
> 
> > 
> > If I follow your suggestion, how do I enable the new cache eviction from the
> > command line?
> 
> According to the experiment params portion of the field trial docs:
> 
> chrome.exe --enable-features="MyFeatureName<MyStudyName"
>   --force-fieldtrials=MyStudyName/Group1
>   --force-fieldtrial-params=MyStudyName.Group1:x/100/y/Test
> 

So the control group needs to have
--disable-features="SimpleCacheEvictionWithSize<Study" or possibly
--enable-features="*SimpleCacheEvictionWithSize<Study" and then it should work,
right?

[jkarlin, 2017-06-30 11:57:27]

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_experiment.cc (right):

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:32: experiment->type =
SimpleExperimentType::EVICT_WITH_SIZE;
On 2017/06/29 19:26:48, hubbe wrote:
> On 2017/06/29 19:17:01, jkarlin_slow wrote:
> > On 2017/06/29 18:55:26, hubbe wrote:
> > > On 2017/06/29 16:26:11, jkarlin_slow wrote:
> > > > You should also parse the parameter and use the parameter as your
> > > enable/disable
> > > > switch. That way both control and experiment have EVICT_WITH_SIZE cache
> > types
> > > > (causing the caches to reset) but only if the param is 1 will you
actually
> > > > enable the feature.
> > > 
> > > My intention was to have the control group disable the
> > > SimpleCacheEvictionWithSize feature so that the experiment is associated
> with
> > > the feature for both groups.
> > 
> > But then you wouldn't clear the control's cache?
> 
> I don't understand, why wouldn't it?
> My intention was to have the cache cleared for both groups.
> Wouldn't the experiment be associated with the feature for the control group
as
> well?
> 
> > Seems better to clear it to get
> > apples-to-apples comparisons.
> > 
> > > 
> > > If I follow your suggestion, how do I enable the new cache eviction from
the
> > > command line?
> > 
> > According to the experiment params portion of the field trial docs:
> > 
> > chrome.exe --enable-features="MyFeatureName<MyStudyName"
> >   --force-fieldtrials=MyStudyName/Group1
> >   --force-fieldtrial-params=MyStudyName.Group1:x/100/y/Test
> > 
> 
> So the control group needs to have
> --disable-features="SimpleCacheEvictionWithSize<Study" or possibly
> --enable-features="*SimpleCacheEvictionWithSize<Study" and then it should
work,
> right?

Ah, I think I understand now. You want to use the same field trial config file
as the size study, and you want to use the same control group. 

First, note that the size study expires at the end of today, the 30th. So there
is no issue with starting a whole new field trial experiment.

Second, if it didn't expire today, you still wouldn't want to use that control
group in your new experiment as it wouldn't be apples-to-apples since it started
months ago.

[hubbe, 2017-06-30 17:30:34]

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_experiment.cc (right):

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:32: experiment->type =
SimpleExperimentType::EVICT_WITH_SIZE;
On 2017/06/30 11:57:27, jkarlin_slow wrote:
> On 2017/06/29 19:26:48, hubbe wrote:
> > On 2017/06/29 19:17:01, jkarlin_slow wrote:
> > > On 2017/06/29 18:55:26, hubbe wrote:
> > > > On 2017/06/29 16:26:11, jkarlin_slow wrote:
> > > > > You should also parse the parameter and use the parameter as your
> > > > enable/disable
> > > > > switch. That way both control and experiment have EVICT_WITH_SIZE
cache
> > > types
> > > > > (causing the caches to reset) but only if the param is 1 will you
> actually
> > > > > enable the feature.
> > > > 
> > > > My intention was to have the control group disable the
> > > > SimpleCacheEvictionWithSize feature so that the experiment is associated
> > with
> > > > the feature for both groups.
> > > 
> > > But then you wouldn't clear the control's cache?
> > 
> > I don't understand, why wouldn't it?
> > My intention was to have the cache cleared for both groups.
> > Wouldn't the experiment be associated with the feature for the control group
> as
> > well?
> > 
> > > Seems better to clear it to get
> > > apples-to-apples comparisons.
> > > 
> > > > 
> > > > If I follow your suggestion, how do I enable the new cache eviction from
> the
> > > > command line?
> > > 
> > > According to the experiment params portion of the field trial docs:
> > > 
> > > chrome.exe --enable-features="MyFeatureName<MyStudyName"
> > >   --force-fieldtrials=MyStudyName/Group1
> > >   --force-fieldtrial-params=MyStudyName.Group1:x/100/y/Test
> > > 
> > 
> > So the control group needs to have
> > --disable-features="SimpleCacheEvictionWithSize<Study" or possibly
> > --enable-features="*SimpleCacheEvictionWithSize<Study" and then it should
> work,
> > right?
> 
> Ah, I think I understand now. You want to use the same field trial config file
> as the size study, and you want to use the same control group. 
> 
> First, note that the size study expires at the end of today, the 30th. So
there
> is no issue with starting a whole new field trial experiment.
> 
> Second, if it didn't expire today, you still wouldn't want to use that control
> group in your new experiment as it wouldn't be apples-to-apples since it
started
> months ago. 
> 

No, I wasn't planning on using the same field trial.
As far as I can tell, in your experiment, the Feature controls if the experiment
is on or off, and the parameter controls what to actually do.

In my experiment, I want the Feature to directly control weather we evict by
size or not so that I can still enable that on the command line for my own
testing. The experiment would simply turn the feature on or off, and in doing
so, the experiment gets associated with the feature.

Maybe I've misunderstood how something works?

[jkarlin, 2017-06-30 18:18:48]

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_experiment.cc (right):

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:32: experiment->type =
SimpleExperimentType::EVICT_WITH_SIZE;
On 2017/06/30 17:30:34, hubbe wrote:
> On 2017/06/30 11:57:27, jkarlin_slow wrote:
> > On 2017/06/29 19:26:48, hubbe wrote:
> > > On 2017/06/29 19:17:01, jkarlin_slow wrote:
> > > > On 2017/06/29 18:55:26, hubbe wrote:
> > > > > On 2017/06/29 16:26:11, jkarlin_slow wrote:
> > > > > > You should also parse the parameter and use the parameter as your
> > > > > enable/disable
> > > > > > switch. That way both control and experiment have EVICT_WITH_SIZE
> cache
> > > > types
> > > > > > (causing the caches to reset) but only if the param is 1 will you
> > actually
> > > > > > enable the feature.
> > > > > 
> > > > > My intention was to have the control group disable the
> > > > > SimpleCacheEvictionWithSize feature so that the experiment is
associated
> > > with
> > > > > the feature for both groups.
> > > > 
> > > > But then you wouldn't clear the control's cache?
> > > 
> > > I don't understand, why wouldn't it?
> > > My intention was to have the cache cleared for both groups.
> > > Wouldn't the experiment be associated with the feature for the control
group
> > as
> > > well?
> > > 
> > > > Seems better to clear it to get
> > > > apples-to-apples comparisons.
> > > > 
> > > > > 
> > > > > If I follow your suggestion, how do I enable the new cache eviction
from
> > the
> > > > > command line?
> > > > 
> > > > According to the experiment params portion of the field trial docs:
> > > > 
> > > > chrome.exe --enable-features="MyFeatureName<MyStudyName"
> > > >   --force-fieldtrials=MyStudyName/Group1
> > > >   --force-fieldtrial-params=MyStudyName.Group1:x/100/y/Test
> > > > 
> > > 
> > > So the control group needs to have
> > > --disable-features="SimpleCacheEvictionWithSize<Study" or possibly
> > > --enable-features="*SimpleCacheEvictionWithSize<Study" and then it should
> > work,
> > > right?
> > 
> > Ah, I think I understand now. You want to use the same field trial config
file
> > as the size study, and you want to use the same control group. 
> > 
> > First, note that the size study expires at the end of today, the 30th. So
> there
> > is no issue with starting a whole new field trial experiment.
> > 
> > Second, if it didn't expire today, you still wouldn't want to use that
control
> > group in your new experiment as it wouldn't be apples-to-apples since it
> started
> > months ago. 
> > 
> 
> No, I wasn't planning on using the same field trial.
> As far as I can tell, in your experiment, the Feature controls if the
experiment
> is on or off, and the parameter controls what to actually do.

Correct. And setting the parameter to 100 is a noop, and behaves as a control.

> 
> In my experiment, I want the Feature to directly control weather we evict by
> size or not so that I can still enable that on the command line for my own
> testing. The experiment would simply turn the feature on or off, and in doing
> so, the experiment gets associated with the feature.

You can do that, but then you don't have a control. If the control is simply to
disable the feature, then the control group will just be the normal user
population which doesn't create a new cache.

> 
> Maybe I've misunderstood how something works?

The SimpleExperiment works by writing the experiment type and param to the
SimpleCache's metadata on disk. If either the type or param change, the old
cache is destroyed and a new one is built with the new type and param. So if you
want a control group, you have to change the feature, param, or both, from the
default.

[hubbe, 2017-06-30 19:00:43]

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_experiment.cc (right):

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:32: experiment->type =
SimpleExperimentType::EVICT_WITH_SIZE;
On 2017/06/30 18:18:48, jkarlin_slow wrote:
> On 2017/06/30 17:30:34, hubbe wrote:
> > On 2017/06/30 11:57:27, jkarlin_slow wrote:
> > > On 2017/06/29 19:26:48, hubbe wrote:
> > > > On 2017/06/29 19:17:01, jkarlin_slow wrote:
> > > > > On 2017/06/29 18:55:26, hubbe wrote:
> > > > > > On 2017/06/29 16:26:11, jkarlin_slow wrote:
> > > > > > > You should also parse the parameter and use the parameter as your
> > > > > > enable/disable
> > > > > > > switch. That way both control and experiment have EVICT_WITH_SIZE
> > cache
> > > > > types
> > > > > > > (causing the caches to reset) but only if the param is 1 will you
> > > actually
> > > > > > > enable the feature.
> > > > > > 
> > > > > > My intention was to have the control group disable the
> > > > > > SimpleCacheEvictionWithSize feature so that the experiment is
> associated
> > > > with
> > > > > > the feature for both groups.
> > > > > 
> > > > > But then you wouldn't clear the control's cache?
> > > > 
> > > > I don't understand, why wouldn't it?
> > > > My intention was to have the cache cleared for both groups.
> > > > Wouldn't the experiment be associated with the feature for the control
> group
> > > as
> > > > well?
> > > > 
> > > > > Seems better to clear it to get
> > > > > apples-to-apples comparisons.
> > > > > 
> > > > > > 
> > > > > > If I follow your suggestion, how do I enable the new cache eviction
> from
> > > the
> > > > > > command line?
> > > > > 
> > > > > According to the experiment params portion of the field trial docs:
> > > > > 
> > > > > chrome.exe --enable-features="MyFeatureName<MyStudyName"
> > > > >   --force-fieldtrials=MyStudyName/Group1
> > > > >   --force-fieldtrial-params=MyStudyName.Group1:x/100/y/Test
> > > > > 
> > > > 
> > > > So the control group needs to have
> > > > --disable-features="SimpleCacheEvictionWithSize<Study" or possibly
> > > > --enable-features="*SimpleCacheEvictionWithSize<Study" and then it
should
> > > work,
> > > > right?
> > > 
> > > Ah, I think I understand now. You want to use the same field trial config
> file
> > > as the size study, and you want to use the same control group. 
> > > 
> > > First, note that the size study expires at the end of today, the 30th. So
> > there
> > > is no issue with starting a whole new field trial experiment.
> > > 
> > > Second, if it didn't expire today, you still wouldn't want to use that
> control
> > > group in your new experiment as it wouldn't be apples-to-apples since it
> > started
> > > months ago. 
> > > 
> > 
> > No, I wasn't planning on using the same field trial.
> > As far as I can tell, in your experiment, the Feature controls if the
> experiment
> > is on or off, and the parameter controls what to actually do.
> 
> Correct. And setting the parameter to 100 is a noop, and behaves as a control.
> 
> > 
> > In my experiment, I want the Feature to directly control weather we evict by
> > size or not so that I can still enable that on the command line for my own
> > testing. The experiment would simply turn the feature on or off, and in
doing
> > so, the experiment gets associated with the feature.
> 
> You can do that, but then you don't have a control. If the control is simply
to
> disable the feature, then the control group will just be the normal user
> population which doesn't create a new cache.
> 
> > 
> > Maybe I've misunderstood how something works?
> 
> The SimpleExperiment works by writing the experiment type and param to the
> SimpleCache's metadata on disk. If either the type or param change, the old
> cache is destroyed and a new one is built with the new type and param. So if
you
> want a control group, you have to change the feature, param, or both, from the
> default.

I just change experiment->type.

The control group will have the experiment explicitly disabling the feature,
which will associate the experiment with the feature. This will make this code
set the experiment type, which will clear the cache for the control group. If
this doesn't work, then I am misunderstanding how something works.

I didn't realize the param was also written into the index, so I fixed that.

[hubbe, 2017-06-30 19:00:51]

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

[commit-bot: I haz the power, 2017-06-30 19:01:09]

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, 2017-06-30 20:50:38]

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

[commit-bot: I haz the power, 2017-06-30 20:50:41]

Dry run: This issue passed the CQ dry run.

[pasko, 2017-07-06 12:30:17]

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:346: // std::sort() to avoid N-squared
worst case scenario.
On 2017/06/29 18:55:27, hubbe wrote:
> On 2017/06/29 15:27:25, pasko (OOO) wrote:
> > If it is only marginally faster than std::sort, I would prefer the latter.
> 
> Depends on what you mean by marginal.
> The introselect algorithm is linear on average, so the more entries that are
in
> the cache, the more we gain from it. Since evicting by size will keep more
> entries in the cache, I felt that this might be important.
> 

I'd say a custom introselect is worth if it provides more than 2ms improvement
over std::sort on M53 core for 99th %-ile of cache sizes (16K entries). Welcome
to my world of arbitrary numbers :)

[pasko, 2017-07-06 12:33:51]

Also: please enhance the commit description (roughly: what, why and how). If you
reference the document, please make it publicly readable.

[jkarlin, 2017-07-06 13:58:18]

https://codereview.chromium.org/2918893002/diff/180001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/2918893002/diff/180001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:325: bool use_size =
base::FeatureList::IsEnabled(kSimpleCacheEvictionWithSize);
We shouldn't use base::FeatureList::IsEnabled for SimpleCache experiment
detection, we should be using GetSimpleExperiment which takes the cache_type
into account. We don't want to enable experimental features on non-DISK_CACHE
types. Once you're using GetSimpleExperiment, it's trivial to use the param to
determine if you're in a control or experiment group of the experiment.

[hubbe, 2017-07-10 18:21:37]

On 2017/07/06 12:30:17, pasko wrote:
>
https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
> File net/disk_cache/simple/simple_index.cc (right):
> 
>
https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
> net/disk_cache/simple/simple_index.cc:346: // std::sort() to avoid N-squared
> worst case scenario.
> On 2017/06/29 18:55:27, hubbe wrote:
> > On 2017/06/29 15:27:25, pasko (OOO) wrote:
> > > If it is only marginally faster than std::sort, I would prefer the latter.
> > 
> > Depends on what you mean by marginal.
> > The introselect algorithm is linear on average, so the more entries that are
> in
> > the cache, the more we gain from it. Since evicting by size will keep more
> > entries in the cache, I felt that this might be important.
> > 
> 
> I'd say a custom introselect is worth if it provides more than 2ms improvement
> over std::sort on M53 core for 99th %-ile of cache sizes (16K entries).
Welcome
> to my world of arbitrary numbers :)

I don't have an M53 device to benchmark on.

[hubbe, 2017-07-10 18:22:51]

Description was changed from

==========
evict larger entries first

Design doc:
https://docs.google.com/document/d/1N07uwDiOipRcMG3SanxR8X8flvxrh9Axi1SLpfRNK...

BUG=700102, 736437
==========

to

==========
Changes the eviction algorithm to take the size of the entry into account.
Weighting by size means that large entries will get thrown out of the cache
sooner and the end result is that the cache will have more, but smaller entries.

Design doc:
https://docs.google.com/document/d/1N07uwDiOipRcMG3SanxR8X8flvxrh9Axi1SLpfRNK...

BUG=700102, 736437
==========

[hubbe, 2017-07-10 18:23:46]

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

[hubbe, 2017-07-10 18:23:53]

https://codereview.chromium.org/2918893002/diff/180001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/2918893002/diff/180001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:325: bool use_size =
base::FeatureList::IsEnabled(kSimpleCacheEvictionWithSize);
On 2017/07/06 13:58:18, jkarlin_slow wrote:
> We shouldn't use base::FeatureList::IsEnabled for SimpleCache experiment
> detection, we should be using GetSimpleExperiment which takes the cache_type
> into account. We don't want to enable experimental features on non-DISK_CACHE
> types. Once you're using GetSimpleExperiment, it's trivial to use the param to
> determine if you're in a control or experiment group of the experiment.
> 

Done.
Also changed the experiment enabling code to work exactly like the size
experiment.

[commit-bot: I haz the power, 2017-07-10 18:24:05]

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, 2017-07-10 19:00:27]

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

[commit-bot: I haz the power, 2017-07-10 19:00:28]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[hubbe, 2017-07-10 19:07:00]

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

[commit-bot: I haz the power, 2017-07-10 19:07:24]

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, 2017-07-10 20:39:39]

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

[commit-bot: I haz the power, 2017-07-10 20:39:40]

Dry run: This issue passed the CQ dry run.

[pasko, 2017-07-11 13:27:33]

On 2017/07/10 18:21:37, hubbe wrote:
> On 2017/07/06 12:30:17, pasko wrote:
> >
>
https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
> > File net/disk_cache/simple/simple_index.cc (right):
> > 
> >
>
https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
> > net/disk_cache/simple/simple_index.cc:346: // std::sort() to avoid N-squared
> > worst case scenario.
> > On 2017/06/29 18:55:27, hubbe wrote:
> > > On 2017/06/29 15:27:25, pasko (OOO) wrote:
> > > > If it is only marginally faster than std::sort, I would prefer the
latter.
> > > 
> > > Depends on what you mean by marginal.
> > > The introselect algorithm is linear on average, so the more entries that
are
> > in
> > > the cache, the more we gain from it. Since evicting by size will keep more
> > > entries in the cache, I felt that this might be important.
> > > 
> > 
> > I'd say a custom introselect is worth if it provides more than 2ms
improvement
> > over std::sort on M53 core for 99th %-ile of cache sizes (16K entries).
> Welcome
> > to my world of arbitrary numbers :)
> 
> I don't have an M53 device to benchmark on.

then just keep it simple?

[hubbe, 2017-07-11 17:58:14]

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

[hubbe, 2017-07-11 17:58:28]

On 2017/07/11 13:27:33, pasko wrote:
> On 2017/07/10 18:21:37, hubbe wrote:
> > On 2017/07/06 12:30:17, pasko wrote:
> > >
> >
>
https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
> > > File net/disk_cache/simple/simple_index.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
> > > net/disk_cache/simple/simple_index.cc:346: // std::sort() to avoid
N-squared
> > > worst case scenario.
> > > On 2017/06/29 18:55:27, hubbe wrote:
> > > > On 2017/06/29 15:27:25, pasko (OOO) wrote:
> > > > > If it is only marginally faster than std::sort, I would prefer the
> latter.
> > > > 
> > > > Depends on what you mean by marginal.
> > > > The introselect algorithm is linear on average, so the more entries that
> are
> > > in
> > > > the cache, the more we gain from it. Since evicting by size will keep
more
> > > > entries in the cache, I felt that this might be important.
> > > > 
> > > 
> > > I'd say a custom introselect is worth if it provides more than 2ms
> improvement
> > > over std::sort on M53 core for 99th %-ile of cache sizes (16K entries).
> > Welcome
> > > to my world of arbitrary numbers :)
> > 
> > I don't have an M53 device to benchmark on.
> 
> then just keep it simple?

Done.

[hubbe, 2017-07-11 17:58:32]

On 2017/07/11 13:27:33, pasko wrote:
> On 2017/07/10 18:21:37, hubbe wrote:
> > On 2017/07/06 12:30:17, pasko wrote:
> > >
> >
>
https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
> > > File net/disk_cache/simple/simple_index.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2918893002/diff/140001/net/disk_cache/simple/...
> > > net/disk_cache/simple/simple_index.cc:346: // std::sort() to avoid
N-squared
> > > worst case scenario.
> > > On 2017/06/29 18:55:27, hubbe wrote:
> > > > On 2017/06/29 15:27:25, pasko (OOO) wrote:
> > > > > If it is only marginally faster than std::sort, I would prefer the
> latter.
> > > > 
> > > > Depends on what you mean by marginal.
> > > > The introselect algorithm is linear on average, so the more entries that
> are
> > > in
> > > > the cache, the more we gain from it. Since evicting by size will keep
more
> > > > entries in the cache, I felt that this might be important.
> > > > 
> > > 
> > > I'd say a custom introselect is worth if it provides more than 2ms
> improvement
> > > over std::sort on M53 core for 99th %-ile of cache sizes (16K entries).
> > Welcome
> > > to my world of arbitrary numbers :)
> > 
> > I don't have an M53 device to benchmark on.
> 
> then just keep it simple?

Done.

[commit-bot: I haz the power, 2017-07-11 17:58:33]

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, 2017-07-11 18:57:00]

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

[commit-bot: I haz the power, 2017-07-11 18:57:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Maks Orlovich, 2017-07-12 13:47:21]

Benchmarked new revision on my MotoG3, and it performs fine --- ~2.2ms instead
of existing code's 2ms, which seems fine.

(IIRC the fancy algorithm did ~1ms)

[jkarlin, 2017-07-12 15:51:41]

https://codereview.chromium.org/2918893002/diff/240001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_experiment.cc (right):

https://codereview.chromium.org/2918893002/diff/240001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:29: if
(disk_cache::SimpleExperimentType::NONE != experiment->type)
After returning early in GetSimpleExperiment you can turn this into a DCHECK

https://codereview.chromium.org/2918893002/diff/240001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:51: experiment->type =
disk_cache::SimpleExperimentType::SIZE;
I believe you want EVICT_WITH_SIZE here ;) That would have been caught with a
unittest. Please add relevant tests to simple_experiment_unittest.cc

https://codereview.chromium.org/2918893002/diff/240001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:94:
CheckForSimpleSizeExperiment(&experiment);
Can you have each check return early if they return true to give us clear
semantics that the first matching experiment wins.

https://codereview.chromium.org/2918893002/diff/240001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/2918893002/diff/240001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:321: typedef std::pair<uint64_t, const
EntrySet::value_type*> SortHelper;
using SortHelper = std::pair<uint64_t, const EntrySet::value_type*>;

https://codereview.chromium.org/2918893002/diff/240001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:324: uint32_t now = (base::Time::Now() -
base::Time::UnixEpoch()).InSeconds();
Hmm, can we instead use base::Time here instead of uint32_t? In fact, we should
be using base::Time and TimeDelta throughout.

https://codereview.chromium.org/2918893002/diff/240001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:333: uint64_t sort_value = now -
i->second.RawTimeForSorting();
The new eviction strategy needs tests in simple_index_unittest.cc

[hubbe, 2017-07-12 18:22:11]

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

[commit-bot: I haz the power, 2017-07-12 18:22:27]

Dry run: CQ is trying da patch.

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

[hubbe, 2017-07-12 18:22:50]

https://codereview.chromium.org/2918893002/diff/240001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_experiment.cc (right):

https://codereview.chromium.org/2918893002/diff/240001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:29: if
(disk_cache::SimpleExperimentType::NONE != experiment->type)
On 2017/07/12 15:51:40, jkarlin wrote:
> After returning early in GetSimpleExperiment you can turn this into a DCHECK

Done.

https://codereview.chromium.org/2918893002/diff/240001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:51: experiment->type =
disk_cache::SimpleExperimentType::SIZE;
On 2017/07/12 15:51:41, jkarlin wrote:
> I believe you want EVICT_WITH_SIZE here ;) That would have been caught with a
> unittest. Please add relevant tests to simple_experiment_unittest.cc

Done.

https://codereview.chromium.org/2918893002/diff/240001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:94:
CheckForSimpleSizeExperiment(&experiment);
On 2017/07/12 15:51:41, jkarlin wrote:
> Can you have each check return early if they return true to give us clear
> semantics that the first matching experiment wins.

Done.

https://codereview.chromium.org/2918893002/diff/240001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/2918893002/diff/240001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:321: typedef std::pair<uint64_t, const
EntrySet::value_type*> SortHelper;
On 2017/07/12 15:51:41, jkarlin wrote:
> using SortHelper = std::pair<uint64_t, const EntrySet::value_type*>;

Done.

https://codereview.chromium.org/2918893002/diff/240001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:324: uint32_t now = (base::Time::Now() -
base::Time::UnixEpoch()).InSeconds();
On 2017/07/12 15:51:41, jkarlin wrote:
> Hmm, can we instead use base::Time here instead of uint32_t? In fact, we
should
> be using base::Time and TimeDelta throughout.

We can, but it slows things down, so I would prefer not to.

https://codereview.chromium.org/2918893002/diff/240001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:333: uint64_t sort_value = now -
i->second.RawTimeForSorting();
On 2017/07/12 15:51:41, jkarlin wrote:
> The new eviction strategy needs tests in simple_index_unittest.cc

Done.

[commit-bot: I haz the power, 2017-07-12 21:08:20]

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

[commit-bot: I haz the power, 2017-07-12 21:08:21]

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_...)

[jkarlin, 2017-07-13 13:24:26]

https://codereview.chromium.org/2918893002/diff/240001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/2918893002/diff/240001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:324: uint32_t now = (base::Time::Now() -
base::Time::UnixEpoch()).InSeconds();
On 2017/07/12 18:22:50, hubbe wrote:
> On 2017/07/12 15:51:41, jkarlin wrote:
> > Hmm, can we instead use base::Time here instead of uint32_t? In fact, we
> should
> > be using base::Time and TimeDelta throughout.
> 
> We can, but it slows things down, so I would prefer not to.

I'd like to see some sort of benchmark to show that the slowdown is significant.
We should be using base::Time where feasible.

[hubbe, 2017-07-13 17:34:13]

On 2017/07/13 13:24:26, jkarlin wrote:
>
https://codereview.chromium.org/2918893002/diff/240001/net/disk_cache/simple/...
> File net/disk_cache/simple/simple_index.cc (right):
> 
>
https://codereview.chromium.org/2918893002/diff/240001/net/disk_cache/simple/...
> net/disk_cache/simple/simple_index.cc:324: uint32_t now = (base::Time::Now() -
> base::Time::UnixEpoch()).InSeconds();
> On 2017/07/12 18:22:50, hubbe wrote:
> > On 2017/07/12 15:51:41, jkarlin wrote:
> > > Hmm, can we instead use base::Time here instead of uint32_t? In fact, we
> > should
> > > be using base::Time and TimeDelta throughout.
> > 
> > We can, but it slows things down, so I would prefer not to.
> 
> I'd like to see some sort of benchmark to show that the slowdown is
significant.
> We should be using base::Time where feasible.

On what platform under what circumstances?
My 32-core xeon is hardly representative.

[jkarlin, 2017-07-13 17:40:38]

On 2017/07/13 17:34:13, hubbe wrote:
> On 2017/07/13 13:24:26, jkarlin wrote:
> >
>
https://codereview.chromium.org/2918893002/diff/240001/net/disk_cache/simple/...
> > File net/disk_cache/simple/simple_index.cc (right):
> > 
> >
>
https://codereview.chromium.org/2918893002/diff/240001/net/disk_cache/simple/...
> > net/disk_cache/simple/simple_index.cc:324: uint32_t now = (base::Time::Now()
-
> > base::Time::UnixEpoch()).InSeconds();
> > On 2017/07/12 18:22:50, hubbe wrote:
> > > On 2017/07/12 15:51:41, jkarlin wrote:
> > > > Hmm, can we instead use base::Time here instead of uint32_t? In fact, we
> > > should
> > > > be using base::Time and TimeDelta throughout.
> > > 
> > > We can, but it slows things down, so I would prefer not to.
> > 
> > I'd like to see some sort of benchmark to show that the slowdown is
> significant.
> > We should be using base::Time where feasible.
> 
> On what platform under what circumstances?
> My 32-core xeon is hardly representative.

You're only using one core here. Not sure which benchmark, perhaps Maks has a
suggestion?

[Maks Orlovich, 2017-07-13 17:48:18]

> You're only using one core here. Not sure which benchmark, perhaps Maks has a
> suggestion?

SimpleIndexPerfTest.EvictionPerformance, and I can run it on Android.

[hubbe, 2017-07-13 17:54:49]

On 2017/07/13 17:48:18, Maks Orlovich wrote:
> > You're only using one core here. Not sure which benchmark, perhaps Maks has
a
> > suggestion?
> 
> SimpleIndexPerfTest.EvictionPerformance, and I can run it on Android.

I did this:

--- a/net/disk_cache/simple/simple_index.cc
+++ b/net/disk_cache/simple/simple_index.cc
@@ -320,7 +320,7 @@ void SimpleIndex::StartEvictionIfNeeded() {
   using SortHelper = std::pair<uint64_t, const EntrySet::value_type*>;
   std::vector<SortHelper> entries;
   entries.reserve(entries_set_.size());
-  uint32_t now = (base::Time::Now() - base::Time::UnixEpoch()).InSeconds();
+  base::Time now = base::Time::Now();
   bool use_size = false;
   SimpleExperiment experiment = GetSimpleExperiment(cache_type_);
   if (experiment.type == SimpleExperimentType::EVICT_WITH_SIZE &&
@@ -329,7 +329,7 @@ void SimpleIndex::StartEvictionIfNeeded() {
   }
   for (EntrySet::const_iterator i = entries_set_.begin();
        i != entries_set_.end(); ++i) {
-    uint64_t sort_value = now - i->second.RawTimeForSorting();
+    uint64_t sort_value = (now - i->second.GetLastUsedTime()).InSeconds();
     if (use_size) {
       // Will not overflow since we're multiplying two 32-bit values and
storing
       // them in a 64-bit variable.


The benchmark on my machine goes from 0.271059ms to 0.294052ms
So, it's an 8.5% slowdown on my machine. Which implementation do you prefer?

[chromium-reviews, 2017-07-13 18:10:02]

Running the phone version right now...

On Thu, Jul 13, 2017 at 1:54 PM, <hubbe@chromium.org> wrote:

> On 2017/07/13 17:48:18, Maks Orlovich wrote:
> > > You're only using one core here. Not sure which benchmark, perhaps
> Maks has
> a
> > > suggestion?
> >
> > SimpleIndexPerfTest.EvictionPerformance, and I can run it on Android.
>
> I did this:
>
> --- a/net/disk_cache/simple/simple_index.cc
> +++ b/net/disk_cache/simple/simple_index.cc
> @@ -320,7 +320,7 @@ void SimpleIndex::StartEvictionIfNeeded() {
> using SortHelper = std::pair<uint64_t, const EntrySet::value_type*>;
> std::vector<SortHelper> entries;
> entries.reserve(entries_set_.size());
> - uint32_t now = (base::Time::Now() - base::Time::UnixEpoch()).
> InSeconds();
> + base::Time now = base::Time::Now();
> bool use_size = false;
> SimpleExperiment experiment = GetSimpleExperiment(cache_type_);
> if (experiment.type == SimpleExperimentType::EVICT_WITH_SIZE &&
> @@ -329,7 +329,7 @@ void SimpleIndex::StartEvictionIfNeeded() {
> }
> for (EntrySet::const_iterator i = entries_set_.begin();
> i != entries_set_.end(); ++i) {
> - uint64_t sort_value = now - i->second.RawTimeForSorting();
> + uint64_t sort_value = (now - i->second.GetLastUsedTime()).InSeconds();
> if (use_size) {
> // Will not overflow since we're multiplying two 32-bit values and
> storing
> // them in a 64-bit variable.
>
>
> The benchmark on my machine goes from 0.271059ms to 0.294052ms
> So, it's an 8.5% slowdown on my machine. Which implementation do you
> prefer?
>
>
> https://codereview.chromium.org/2918893002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Maks Orlovich, 2017-07-13 18:20:23]

Using base::Time makes it about 2x slower on my MotoG3.
(4.4ms, with previous patch revision being 2.2ms, and before being 2ms)

[jkarlin, 2017-07-13 18:26:42]

On 2017/07/13 17:54:49, hubbe wrote:
> On 2017/07/13 17:48:18, Maks Orlovich wrote:
> > > You're only using one core here. Not sure which benchmark, perhaps Maks
has
> a
> > > suggestion?
> > 
> > SimpleIndexPerfTest.EvictionPerformance, and I can run it on Android.
> 
> I did this:
> 
> --- a/net/disk_cache/simple/simple_index.cc
> +++ b/net/disk_cache/simple/simple_index.cc
> @@ -320,7 +320,7 @@ void SimpleIndex::StartEvictionIfNeeded() {
>    using SortHelper = std::pair<uint64_t, const EntrySet::value_type*>;
>    std::vector<SortHelper> entries;
>    entries.reserve(entries_set_.size());
> -  uint32_t now = (base::Time::Now() - base::Time::UnixEpoch()).InSeconds();
> +  base::Time now = base::Time::Now();
>    bool use_size = false;
>    SimpleExperiment experiment = GetSimpleExperiment(cache_type_);
>    if (experiment.type == SimpleExperimentType::EVICT_WITH_SIZE &&
> @@ -329,7 +329,7 @@ void SimpleIndex::StartEvictionIfNeeded() {
>    }
>    for (EntrySet::const_iterator i = entries_set_.begin();
>         i != entries_set_.end(); ++i) {
> -    uint64_t sort_value = now - i->second.RawTimeForSorting();
> +    uint64_t sort_value = (now - i->second.GetLastUsedTime()).InSeconds();
>      if (use_size) {
>        // Will not overflow since we're multiplying two 32-bit values and
> storing
>        // them in a 64-bit variable.
> 
> 
> The benchmark on my machine goes from 0.271059ms to 0.294052ms
> So, it's an 8.5% slowdown on my machine. Which implementation do you prefer?

8.5% seemed strong to me. I ran them locally 3 times each:

uint64_t: 0.284283, 0.283569, 0.27482
base::Time: 0.285466, 0.284601, 0.278868

I'm inclined to say the difference, if any, is in the noise.

[jkarlin, 2017-07-13 18:39:52]

On 2017/07/13 18:20:23, Maks Orlovich wrote:
> Using base::Time makes it about 2x slower on my MotoG3.
> (4.4ms, with previous patch revision being 2.2ms, and before being 2ms)

That must be noise. I have a hard time believing that the linear application of
base::Time (which is finely tuned and essentially just base::Time anyway) is
going to have any real effect on an nlogn function. Certainly not > 2X.

[jkarlin, 2017-07-13 18:40:19]

On 2017/07/13 18:39:52, jkarlin wrote:
> On 2017/07/13 18:20:23, Maks Orlovich wrote:
> > Using base::Time makes it about 2x slower on my MotoG3.
> > (4.4ms, with previous patch revision being 2.2ms, and before being 2ms)
> 
> That must be noise. I have a hard time believing that the linear application
of
> base::Time (which is finely tuned and essentially just base::Time anyway) is
> going to have any real effect on an nlogn function. Certainly not > 2X.

Sorry, essentially just uint64_t

[Maks Orlovich, 2017-07-13 18:47:05]

> > That must be noise. I have a hard time believing that the linear application
> of
> > base::Time (which is finely tuned and essentially just base::Time anyway) is
> > going to have any real effect on an nlogn function. Certainly not > 2X.


OK, doing 20 runs to check.
Might take a bit.

[hubbe, 2017-07-13 18:59:36]

On 2017/07/13 18:47:05, Maks Orlovich wrote:
> > > That must be noise. I have a hard time believing that the linear
application
> > of
> > > base::Time (which is finely tuned and essentially just base::Time anyway)
is
> > > going to have any real effect on an nlogn function. Certainly not > 2X.
> 
> 
> OK, doing 20 runs to check.
> Might take a bit.

I don't think it's noise. 
The slowdown in the base::Time() version is probably not caused by base::Time(),
but by calling a non-inlined function. (GetLastUsedTime)
This is tight-loop-code, so calling non-inlined functions is actually a really
slow operation by comparision.
I hit similar problems with using lambda functions as sort predicates in earlier
revisions.

[Maks Orlovich, 2017-07-13 20:02:56]

Sure doesn't look random to me (though baseline is unpatched...) --- this one is
from a Samsung Note test device..

https://docs.google.com/spreadsheets/d/1TGQ58wgiVrzDa0vZZSlqjQLgcVfnzAjyZvYiK...
(public spreadsheet)

[jkarlin, 2017-07-14 15:00:01]

On 2017/07/13 20:02:56, Maks Orlovich wrote:
> Sure doesn't look random to me (though baseline is unpatched...) --- this one
is
> from a Samsung Note test device..
> 
>
https://docs.google.com/spreadsheets/d/1TGQ58wgiVrzDa0vZZSlqjQLgcVfnzAjyZvYiK...
> (public spreadsheet)

Thanks for doing that Maks. I'm surprised. Part of the cost could come from
GetLastUsedTime being more than a simple getter as well. Alright, uint it is.

[jkarlin, 2017-07-14 15:19:57]

lgtm!

[hubbe, 2017-07-14 16:55:44]

On 2017/07/14 15:19:57, jkarlin wrote:
> lgtm!

pasko, please take another look?

[hubbe, 2017-07-17 23:12:43]

On 2017/07/14 16:55:44, hubbe wrote:
> On 2017/07/14 15:19:57, jkarlin wrote:
> > lgtm!
> 
> pasko, please take another look?

ping?

[pasko, 2017-07-18 15:30:01]

overall looks a lot cleaner, thank you!

My suggestions below are relatively minor stylistic and testing nitpicks.

On 2017/07/17 23:12:43, hubbe wrote:
> On 2017/07/14 16:55:44, hubbe wrote:
> > On 2017/07/14 15:19:57, jkarlin wrote:
> > > lgtm!
> > 
> > pasko, please take another look?
> 
> ping?

Oh, sorry about blocking you, vacations are to blame. Bad, bad.

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_experiment.cc (right):

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:26: // Updates experiment if the
experiment is active.
nit: s/experiment/|experiment|/

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:27: bool
CheckForEvictionWithSizeExperiment(
The difference between this function and the function below seems to be only in
the use of two constants. Can this avoid repetitions?

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_experiment.h (right):

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.h:27: EVICT_WITH_SIZE = 2,
Please note in the comment how the param is used (I think it is not used?). With
the SIZE experiment it was quite obvious, but for EVICT_WITH_SIZE one could
imagine it being something meaningful.

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:320: using SortHelper =
std::pair<uint64_t, const EntrySet::value_type*>;
It would be clearer with more concrete types and in one line:

std::vector<std::pair<uint64_t, const EntryMetadata*>> entries;

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:327: experiment.param) {
why do we need to check that param is non-zero? I thought the param is only for
being able to clear the cache when changing it ..

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:346: auto begin = entries.begin();
something slightly shorter:

std::sort(std::begin(entries), std::end(entries));
for (const auto& score_metadata_pair : entries) {
  ...
}

Wow, I see that std::pair comparators are since C++14 [1], is it OK to use them?
Will it cause problems when building with GCC? I'm no expert here.

[1] http://en.cppreference.com/w/cpp/utility/pair/operator_cmp

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index_unittest.cc (right):

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index_unittest.cc:605: 40000u);
Perhaps make it depend on the implementation in a tighter way? like make it just
1 byte bigger than the value that prevents this eviction? At least this would
add some more confidence to me, but maybe it's all too obvious for you :)

Hmm, now thinking more about it, one byte off may be lengthy to explain, in
which case I would lean on the side of ease of explaining why the choice of
sizes is such, maybe even without changing the actual test.

Non-mandatory suggestion, up to you, this is just a test, and I almost left
shopping, etc.

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index_unittest.cc:618: // TODO(rdsmith): This is
dependent on the innards of the implementation
rdsmith: what do you mean? Surely eviction triggering depends on how sizes of
entries evolve. Did you want to expose something like RunEvictionNowForTesting?
Otherwise I cannot imagine this TODO being addressed or removed in 5 years from
now.

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index_unittest.cc:625:
EXPECT_FALSE(index()->Has(hashes_.at<2>()));
maybe also test the case when the smaller and older entry is evicted first? Up
to you.

[hubbe, 2017-07-18 18:24:59]

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

[commit-bot: I haz the power, 2017-07-18 18:25:14]

Dry run: CQ is trying da patch.

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

[hubbe, 2017-07-18 18:27:40]

Not too worried about the vacation.
More worried about you guys tag-teaming this into the longest code review I've
ever seen.

PTAL

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_experiment.cc (right):

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:26: // Updates experiment if the
experiment is active.
On 2017/07/18 15:30:01, pasko wrote:
> nit: s/experiment/|experiment|/

Gone.

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.cc:27: bool
CheckForEvictionWithSizeExperiment(
On 2017/07/18 15:30:01, pasko wrote:
> The difference between this function and the function below seems to be only
in
> the use of two constants. Can this avoid repetitions?

Done.

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_experiment.h (right):

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
net/disk_cache/simple/simple_experiment.h:27: EVICT_WITH_SIZE = 2,
On 2017/07/18 15:30:01, pasko wrote:
> Please note in the comment how the param is used (I think it is not used?).
With
> the SIZE experiment it was quite obvious, but for EVICT_WITH_SIZE one could
> imagine it being something meaningful.

Done.

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:320: using SortHelper =
std::pair<uint64_t, const EntrySet::value_type*>;
On 2017/07/18 15:30:01, pasko wrote:
> It would be clearer with more concrete types and in one line:
> 
> std::vector<std::pair<uint64_t, const EntryMetadata*>> entries;

I disagree, but will change it if you insist.

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:327: experiment.param) {
On 2017/07/18 15:30:01, pasko wrote:
> why do we need to check that param is non-zero? I thought the param is only
for
> being able to clear the cache when changing it ..

If we don't check the param, how do we know if we're in the control group or
not?

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:346: auto begin = entries.begin();
On 2017/07/18 15:30:01, pasko wrote:
> something slightly shorter:
> 
> std::sort(std::begin(entries), std::end(entries));
> for (const auto& score_metadata_pair : entries) {
>   ...
> }
> 

Done

> Wow, I see that std::pair comparators are since C++14 [1], is it OK to use
them?
> Will it cause problems when building with GCC? I'm no expert here.
> 
> [1] http://en.cppreference.com/w/cpp/utility/pair/operator_cmp

No, it's been around for a little longer:
http://www.sgi.com/tech/stl/pair.html

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index_unittest.cc (right):

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index_unittest.cc:605: 40000u);
On 2017/07/18 15:30:01, pasko wrote:
> Perhaps make it depend on the implementation in a tighter way? like make it
just
> 1 byte bigger than the value that prevents this eviction? At least this would
> add some more confidence to me, but maybe it's all too obvious for you :)
> 
> Hmm, now thinking more about it, one byte off may be lengthy to explain, in
> which case I would lean on the side of ease of explaining why the choice of
> sizes is such, maybe even without changing the actual test.
> 
> Non-mandatory suggestion, up to you, this is just a test, and I almost left
> shopping, etc.

Making the test tightly coupled to the implementation seems like a bad idea to
me.
A few more tests might be helpful, but it's not like the previous implementation
had a lot of tests either...

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index_unittest.cc:618: // TODO(rdsmith): This is
dependent on the innards of the implementation
On 2017/07/18 15:30:01, pasko wrote:
> rdsmith: what do you mean? Surely eviction triggering depends on how sizes of
> entries evolve. Did you want to expose something like
RunEvictionNowForTesting?
> Otherwise I cannot imagine this TODO being addressed or removed in 5 years
from
> now. 

Sure, but what if we PostTask the eviction? Or do it in a separate thread? Or
something else...

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index_unittest.cc:625:
EXPECT_FALSE(index()->Has(hashes_.at<2>()));
On 2017/07/18 15:30:01, pasko wrote:
> maybe also test the case when the smaller and older entry is evicted first? Up
> to you.

Done.

[commit-bot: I haz the power, 2017-07-18 20:46:52]

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

[commit-bot: I haz the power, 2017-07-18 20:46:53]

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_...)

[pasko, 2017-07-19 14:42:28]

lgtm, thank you

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:320: using SortHelper =
std::pair<uint64_t, const EntrySet::value_type*>;
On 2017/07/18 18:27:39, hubbe wrote:
> On 2017/07/18 15:30:01, pasko wrote:
> > It would be clearer with more concrete types and in one line:
> > 
> > std::vector<std::pair<uint64_t, const EntryMetadata*>> entries;
> 
> I disagree, but will change it if you insist.

OK, after noting that the type I suggested was wrong, it indeed makes sense to
reference the value_type to hint that we are pointing at innards of the
entries_set_. Perhaps a short comment about that would clarify. Up to you.

However, still seeing 'SortHelper' as not useful, is it some common pattern that
makes things instantly more obvious for people?

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:327: experiment.param) {
On 2017/07/18 18:27:40, hubbe wrote:
> On 2017/07/18 15:30:01, pasko wrote:
> > why do we need to check that param is non-zero? I thought the param is only
> for
> > being able to clear the cache when changing it ..
> 
> If we don't check the param, how do we know if we're in the control group or
> not?

Oh, I forgot, sorry, thank you for clarification. Clever.

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:346: auto begin = entries.begin();
On 2017/07/18 15:30:01, pasko wrote:
> something slightly shorter:
> 
> std::sort(std::begin(entries), std::end(entries));
> for (const auto& score_metadata_pair : entries) {
>   ...
> }
> 
> Wow, I see that std::pair comparators are since C++14 [1], is it OK to use
them?
> Will it cause problems when building with GCC? I'm no expert here.
> 
> [1] http://en.cppreference.com/w/cpp/utility/pair/operator_cmp

Acknowledged. Thanks.

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index_unittest.cc (right):

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index_unittest.cc:605: 40000u);
On 2017/07/18 18:27:40, hubbe wrote:
> Making the test tightly coupled to the implementation seems like a bad idea to
> me.
> A few more tests might be helpful, but it's not like the previous
implementation
> had a lot of tests either...

OK

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index_unittest.cc:618: // TODO(rdsmith): This is
dependent on the innards of the implementation
On 2017/07/18 18:27:40, hubbe wrote:
> On 2017/07/18 15:30:01, pasko wrote:
> > rdsmith: what do you mean? Surely eviction triggering depends on how sizes
of
> > entries evolve. Did you want to expose something like
> RunEvictionNowForTesting?
> > Otherwise I cannot imagine this TODO being addressed or removed in 5 years
> from
> > now. 
> 
> Sure, but what if we PostTask the eviction? Or do it in a separate thread? Or
> something else...

Ah, so you mean in the proper design the test would need to use an official API
for waiting for eviction to complete? Makes sense. Still not sure there is
enough wood behind this arrow to make us motivated to fix this TODO. Would not
actively object to keeping it, thanks for clarification.

[pasko, 2017-07-19 14:50:17]

I was wondering if your long commit message lines are intentional. And I do not
want to sound too picky, so feel free to leave it as is.

Also TIL: the recommended way to reference bugs in commit messages changed:
https://www.chromium.org/developers/contributing-code

[hubbe, 2017-07-19 17:42:06]

Description was changed from

==========
Changes the eviction algorithm to take the size of the entry into account.
Weighting by size means that large entries will get thrown out of the cache
sooner and the end result is that the cache will have more, but smaller entries.

Design doc:
https://docs.google.com/document/d/1N07uwDiOipRcMG3SanxR8X8flvxrh9Axi1SLpfRNK...

BUG=700102, 736437
==========

to

==========
Changes the eviction algorithm to take the size of the entry into
account. Weighting by size means that large entries will get thrown out
of the cache sooner and the end result is that the cache will have more,
but smaller entries.

Design doc:
https://docs.google.com/document/d/1N07uwDiOipRcMG3SanxR8X8flvxrh9Axi1SLpfRNK...

BUG=700102, 736437
==========

[hubbe, 2017-07-19 17:50:33]

> I was wondering if your long commit message lines are >intentional. And I do
not
> want to sound too picky, so feel free to leave it as is.

No, not intentional; fixed.

> Also TIL: the recommended way to reference bugs in commit messages changed:
> https://www.chromium.org/developers/contributing-code

This CL is older than that recommendation I think. :)
(git cl upload --rietveld still generates templates with BUG= though for some
reason.)

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:320: using SortHelper =
std::pair<uint64_t, const EntrySet::value_type*>;
On 2017/07/19 14:42:28, pasko wrote:
> On 2017/07/18 18:27:39, hubbe wrote:
> > On 2017/07/18 15:30:01, pasko wrote:
> > > It would be clearer with more concrete types and in one line:
> > > 
> > > std::vector<std::pair<uint64_t, const EntryMetadata*>> entries;
> > 
> > I disagree, but will change it if you insist.
> 
> OK, after noting that the type I suggested was wrong, it indeed makes sense to
> reference the value_type to hint that we are pointing at innards of the
> entries_set_. Perhaps a short comment about that would clarify. Up to you.
> 
> However, still seeing 'SortHelper' as not useful, is it some common pattern
that
> makes things instantly more obvious for people?

SortHelper was useful when the code was a lot more complicated.
It's not anymore, so I removed it.

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index_unittest.cc (right):

https://codereview.chromium.org/2918893002/diff/260001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index_unittest.cc:618: // TODO(rdsmith): This is
dependent on the innards of the implementation
On 2017/07/19 14:42:28, pasko wrote:
> On 2017/07/18 18:27:40, hubbe wrote:
> > On 2017/07/18 15:30:01, pasko wrote:
> > > rdsmith: what do you mean? Surely eviction triggering depends on how sizes
> of
> > > entries evolve. Did you want to expose something like
> > RunEvictionNowForTesting?
> > > Otherwise I cannot imagine this TODO being addressed or removed in 5 years
> > from
> > > now. 
> > 
> > Sure, but what if we PostTask the eviction? Or do it in a separate thread?
Or
> > something else...
> 
> Ah, so you mean in the proper design the test would need to use an official
API
> for waiting for eviction to complete? Makes sense. Still not sure there is
> enough wood behind this arrow to make us motivated to fix this TODO. Would not
> actively object to keeping it, thanks for clarification.

I consider it more of an FYI than a TODO to make sure that whoever ends up
changing when eviction occurs isn't mystified as to why the tests fail... I
didn't write this TODO, but I think keeping it makes sense, even if it never
actually gets fixed.

[hubbe, 2017-07-19 17:52:14]

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

[commit-bot: I haz the power, 2017-07-19 17:52:35]

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, 2017-07-19 18:57:26]

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

[commit-bot: I haz the power, 2017-07-19 18:57:28]

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...)

[hubbe, 2017-07-19 20:47:53]

The CQ bit was checked by hubbe@chromium.org

[hubbe, 2017-07-19 20:47:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
morlovich@chromium.org, jkarlin@chromium.org, pasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2918893002/#ps300001
(title: "-SortHelper")

[commit-bot: I haz the power, 2017-07-19 20:48:17]

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-07-19 22:47:47]

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

[commit-bot: I haz the power, 2017-07-19 22:47:50]

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...)

[hubbe, 2017-07-19 23:22:54]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2017-07-19 23:23:09]

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-07-20 00:13:02]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1500506574056070,
"parent_rev": "f892df540686f7464ed4f042c763c3770be0e3ae", "commit_rev":
"b89b69cd1174e15198c274b6b662ea7ae85f6a87"}

[commit-bot: I haz the power, 2017-07-20 00:13:23]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1500506574056070,
"parent_rev": "0dd5fcf6628e2232a0e100b130ab2ea688443c4c", "commit_rev":
"a9eeb01aaf3e555b843f78298de19b2f1ac29fa8"}

[commit-bot: I haz the power, 2017-07-20 00:13:41]

Description was changed from

==========
Changes the eviction algorithm to take the size of the entry into
account. Weighting by size means that large entries will get thrown out
of the cache sooner and the end result is that the cache will have more,
but smaller entries.

Design doc:
https://docs.google.com/document/d/1N07uwDiOipRcMG3SanxR8X8flvxrh9Axi1SLpfRNK...

BUG=700102, 736437
==========

to

==========
Changes the eviction algorithm to take the size of the entry into
account. Weighting by size means that large entries will get thrown out
of the cache sooner and the end result is that the cache will have more,
but smaller entries.

Design doc:
https://docs.google.com/document/d/1N07uwDiOipRcMG3SanxR8X8flvxrh9Axi1SLpfRNK...

BUG=700102, 736437

Review-Url: https://codereview.chromium.org/2918893002
Cr-Commit-Position: refs/heads/master@{#488058}
Committed:
https://chromium.googlesource.com/chromium/src/+/a9eeb01aaf3e555b843f78298de1...
==========

[commit-bot: I haz the power, 2017-07-20 00:13:43]

Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/a9eeb01aaf3e555b843f78298de1...