New disk cache eviction algorithm.

New disk cache eviction algorithm (partial implementation).

Disabled by a #def. When enabled, file format 2.1 is used,
with a possible update from ver 2.0. If a version with this
code disabled is run after the upgrade, it will just
discard the file and go back to 2.0.

We now put some of those extra list to use! Entries are
separated into various lists depending on how often are
used, and we keep track of previously evicted entries.

If the new algorithm is not enabled, most of the code just
goes through a different path (with the old code instead of
the new one). One notable exception is OpenFollowingEntry,
used to enumerate the entries on the cache; the code changed
significantly to support the new version of the "cache
iterator", but functionally should do the same as the current
code.



Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=11145

[Nicolas Sylvain, 2009-03-05 02:24:33]

not done with the review, but i have a couple of questions.

http://codereview.chromium.org/27345/diff/2001/3009
File net/disk_cache/backend_impl.cc (right):

http://codereview.chromium.org/27345/diff/2001/3009#newcode324
Line 324: return ResurrectEntry(old_entry, entry);
nit: # of spaces

http://codereview.chromium.org/27345/diff/2001/3009#newcode1033
Line 1033: if (!ret)
so it needs to work only once? why else would it fail?

http://codereview.chromium.org/27345/diff/2001/3009#newcode1041
Line 1041: OpenFollowingEntryFromList(forward, iterator->list,
no need to check for the return value?

http://codereview.chromium.org/27345/diff/2001/3009#newcode1047
Line 1047: entries[i].swap(&temp);  // The entry was already addref'd.
maybe DCHECK for temp == null?

http://codereview.chromium.org/27345/diff/2001/3009#newcode1060
Line 1060: oldest = i;
i'm not sure how often this function is called, but you could optimize it by
adding a "continue;" under this line

http://codereview.chromium.org/27345/diff/2001/3009#newcode1120
Line 1120: bool dirty;
purify wont complain here? Should you set it to something?

http://codereview.chromium.org/27345/diff/2001/3009#newcode1142
Line 1142: bool BackendImpl::ResurrectEntry(EntryImpl* deleted_entry, Entry**
entry) {
looks like "entry" here should be used for something.

[rvargas (doing something else), 2009-03-05 04:16:47]

http://codereview.chromium.org/27345/diff/2001/3009
File net/disk_cache/backend_impl.cc (right):

http://codereview.chromium.org/27345/diff/2001/3009#newcode1033
Line 1033: if (!ret)
On 2009/03/05 02:24:33, Nicolas Sylvain wrote:
> so it needs to work only once? why else would it fail?

If ret is false all lists are empty so there is nothing to do.
It is possible that, for instance, there is only one list, and the first element
is dirty (the browser crashed last time) so OpenFollowingEntryFromList will
delete the entry and return fail (because it doesn't iterate), so we end up
failing the enumeration when there are entries to enumerate, but such "under
recovery" scenarios are not really covered 100% due to the increased code
complexity if we do it. As long as we recover gracefully I think it is ok (the
next call will enumerate everything... and the current code makes the same
tradeoff).

http://codereview.chromium.org/27345/diff/2001/3009#newcode1041
Line 1041: OpenFollowingEntryFromList(forward, iterator->list,
On 2009/03/05 02:24:33, Nicolas Sylvain wrote:
> no need to check for the return value?

We could. I'm just relying in that a failure will return temp on null.

http://codereview.chromium.org/27345/diff/2001/3009#newcode1047
Line 1047: entries[i].swap(&temp);  // The entry was already addref'd.
On 2009/03/05 02:24:33, Nicolas Sylvain wrote:
> maybe DCHECK for temp == null?

Do you mean after the swap?

http://codereview.chromium.org/27345/diff/2001/3009#newcode1120
Line 1120: bool dirty;
On 2009/03/05 02:24:33, Nicolas Sylvain wrote:
> purify wont complain here? Should you set it to something?

no complains. It is an output of NewEntry.

http://codereview.chromium.org/27345/diff/2001/3009#newcode1142
Line 1142: bool BackendImpl::ResurrectEntry(EntryImpl* deleted_entry, Entry**
entry) {
On 2009/03/05 02:24:33, Nicolas Sylvain wrote:
> looks like "entry" here should be used for something.

line 1154? the output was already set to null when the user argument was
validated (line 312)

[Nicolas Sylvain, 2009-03-06 02:06:58]

LGTM