Disk cache: Don't evict anything for the first five

Disk cache: Don't evict anything for the first five
minutes. Basically, we don't want to do anything
that may slow down startup.

BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99059

[rvargas (doing something else), 2011-08-30 22:28:12]

ping

[gavinp, 2011-08-31 02:20:32]

LGTM, with a small nit that you can ignore or not.

http://codereview.chromium.org/7765006/diff/1/net/disk_cache/backend_impl.cc
File net/disk_cache/backend_impl.cc (right):

http://codereview.chromium.org/7765006/diff/1/net/disk_cache/backend_impl.cc#...
net/disk_cache/backend_impl.cc:1197: if (up_ticks_ < kTrimDelay * 2)  // No need
to count forever.
Nit: Why not count forever?  Seems easier and less confusing.

[rvargas (doing something else), 2011-08-31 02:33:19]

Thanks

http://codereview.chromium.org/7765006/diff/1/net/disk_cache/backend_impl.cc
File net/disk_cache/backend_impl.cc (right):

http://codereview.chromium.org/7765006/diff/1/net/disk_cache/backend_impl.cc#...
net/disk_cache/backend_impl.cc:1197: if (up_ticks_ < kTrimDelay * 2)  // No need
to count forever.
On 2011/08/31 02:20:32, gavinp wrote:
> Nit: Why not count forever?  Seems easier and less confusing.

Not easier because then I would have to either clamp the value or care about
integer overflows. To be fair, this is also a clamp, just at a lower value (so
few instructions saved). I could as easily change the constant to be kint32max
if you prefer it that way.

[gavinp, 2011-08-31 03:01:20]

I'd say leave it the same rather than clamp it with kint32max.  However, I argue
for no clamp at all again below...

http://codereview.chromium.org/7765006/diff/1/net/disk_cache/backend_impl.cc
File net/disk_cache/backend_impl.cc (right):

http://codereview.chromium.org/7765006/diff/1/net/disk_cache/backend_impl.cc#...
net/disk_cache/backend_impl.cc:51: const int kTrimDelay = 10;
I'm also made a bit grumpy that these are in multiples of 30 seconds, without
being calculated.  I'd rather this was 5 * kTicksPerMinute or 300 /
kSecondsPerTick.  I haven't looked to see if either constant is available
though.

http://codereview.chromium.org/7765006/diff/1/net/disk_cache/backend_impl.cc#...
net/disk_cache/backend_impl.cc:1197: if (up_ticks_ < kTrimDelay * 2)  // No need
to count forever.
On 2011/08/31 02:33:19, rvargas wrote:
> On 2011/08/31 02:20:32, gavinp wrote:
> > Nit: Why not count forever?  Seems easier and less confusing.
> 
> Not easier because then I would have to either clamp the value or care about
> integer overflows. To be fair, this is also a clamp, just at a lower value (so
> few instructions saved). I could as easily change the constant to be kint32max
> if you prefer it that way.

I'm not convinced a clamp is necessary, because if I'm not missing something,
the overflow is about 2043 years, right?  Admittedly, since we use a signed int,
so after that we then would not evict cache entries for 2043 years + 5 minutes,
which is a long time, and so the disk would probably get full...

[rvargas (doing something else), 2011-08-31 18:06:34]

Thanks

http://codereview.chromium.org/7765006/diff/1/net/disk_cache/backend_impl.cc
File net/disk_cache/backend_impl.cc (right):

http://codereview.chromium.org/7765006/diff/1/net/disk_cache/backend_impl.cc#...
net/disk_cache/backend_impl.cc:51: const int kTrimDelay = 10;
I see your point, but right now 30 seconds is kind of embedded in the code
(mostly relevant for UMA) and I don't want to give the impression that changing
the timer interval is as easy as changing a constant at the top of the file :(

http://codereview.chromium.org/7765006/diff/1/net/disk_cache/backend_impl.cc#...
net/disk_cache/backend_impl.cc:1197: if (up_ticks_ < kTrimDelay * 2)  // No need
to count forever.
On 2011/08/31 03:01:20, gavinp wrote:
> On 2011/08/31 02:33:19, rvargas wrote:
> > On 2011/08/31 02:20:32, gavinp wrote:
> > > Nit: Why not count forever?  Seems easier and less confusing.
> > 
> > Not easier because then I would have to either clamp the value or care about
> > integer overflows. To be fair, this is also a clamp, just at a lower value
(so
> > few instructions saved). I could as easily change the constant to be
kint32max
> > if you prefer it that way.
> 
> I'm not convinced a clamp is necessary, because if I'm not missing something,
> the overflow is about 2043 years, right?  Admittedly, since we use a signed
int,
> so after that we then would not evict cache entries for 2043 years + 5
minutes,
> which is a long time, and so the disk would probably get full...

Convinced. Clamp gone.