Add SimpleCache index file heuristics

Add SimpleCache index file heuristics

We restore the index from enumerating the entries on disk when the dir mtime doesnt match the index file mtime.
Using the mtime we can reliably detect the stale index on every new entry that has been added or an old entry deleted on disk, since the index file must always the last file created (renamed) in the directory.

BUG=233536
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195197

[gavinp, 2013-04-19 09:30:17]

Should we add UMA in this CL as well?

https://codereview.chromium.org/13933029/diff/3002/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://codereview.chromium.org/13933029/diff/3002/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_entry_impl.cc:52: index->Insert(key);
This is dead code. Either index is null, or index->UseIfExists() returned true.

https://codereview.chromium.org/13933029/diff/3002/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index_file.cc (right):

https://codereview.chromium.org/13933029/diff/3002/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.cc:178: return index_info.last_modified
!= dir_info.last_modified;
I think index_info.last_modified >= dir_info.last_modified is better. Right now
we do an atomic rename to put the index in place so equality should be right,
but other operations can update file mtime without changing directory mtime (for
instance, mutating a file).

https://codereview.chromium.org/13933029/diff/3002/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index_file.h (right):

https://codereview.chromium.org/13933029/diff/3002/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:73: static bool IndexFileIsStale(const
base::FilePath& index_filename);
IsIndexFileStale is a better name. I did a double take when I first saw this.

[pasko-google - do not use, 2013-04-19 10:07:55]

Top level, did not look into the code yet:

Please send out CLs by mail as soon as you want them to be reviewed.
Also please fill in the BUG=.

[gavinp, 2013-04-19 10:09:26]

On 2013/04/19 10:07:55, pasko wrote:
> Top level, did not look into the code yet:
> 
> Please send out CLs by mail as soon as you want them to be reviewed.
> Also please fill in the BUG=.

As long as we're nitting the description: Make the summary and first line both
sentences. I suggest "Add SimpleCache index file heuristics"

Wrap lines of description at 72 chars, but on no account more than 76 chars.

And yeah, bug #.

[felipeg, 2013-04-19 10:14:05]

UMA will come in a separate CL.

https://chromiumcodereview.appspot.com/13933029/diff/3002/net/disk_cache/simp...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://chromiumcodereview.appspot.com/13933029/diff/3002/net/disk_cache/simp...
net/disk_cache/simple/simple_entry_impl.cc:52: index->Insert(key);
On 2013/04/19 09:30:17, gavinp wrote:
> This is dead code. Either index is null, or index->UseIfExists() returned
true.

Done.

https://chromiumcodereview.appspot.com/13933029/diff/3002/net/disk_cache/simp...
File net/disk_cache/simple/simple_index_file.cc (right):

https://chromiumcodereview.appspot.com/13933029/diff/3002/net/disk_cache/simp...
net/disk_cache/simple/simple_index_file.cc:178: return index_info.last_modified
!= dir_info.last_modified;
On 2013/04/19 09:30:17, gavinp wrote:
> I think index_info.last_modified >= dir_info.last_modified is better. Right
now
> we do an atomic rename to put the index in place so equality should be right,
> but other operations can update file mtime without changing directory mtime
(for
> instance, mutating a file).

Done.

https://chromiumcodereview.appspot.com/13933029/diff/3002/net/disk_cache/simp...
File net/disk_cache/simple/simple_index_file.h (right):

https://chromiumcodereview.appspot.com/13933029/diff/3002/net/disk_cache/simp...
net/disk_cache/simple/simple_index_file.h:73: static bool IndexFileIsStale(const
base::FilePath& index_filename);
On 2013/04/19 09:30:17, gavinp wrote:
> IsIndexFileStale is a better name. I did a double take when I first saw this.

that comes from my bad english mixed up with french and portuguese

[gavinp, 2013-04-19 11:30:17]

lgtm

https://codereview.chromium.org/13933029/diff/12001/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://codereview.chromium.org/13933029/diff/12001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:69: // way we never leak files. 
CreationOperationComplete will remove the entry
nit: one space after .

https://codereview.chromium.org/13933029/diff/12001/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/13933029/diff/12001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index.cc:12: #include "base/file_util.h"
Once is probably enough.

https://codereview.chromium.org/13933029/diff/12001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index.cc:204: index_file_entries =
SimpleIndexFile::LoadFromDisk(index_filename);
Add a TODO commenting that we probably could load a stale index and use it for
something.

As well, in this code path, an empty index is "always" "invalid". Why not have a
bool out_error returned by LoadFromDisk, so we can sucessfully load empty
indexes?

For that matter, it's a bit outside of this CL, but why is it an error to create
an index in a brand new cache?

https://codereview.chromium.org/13933029/diff/12001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index.cc:207: if (!index_file_entries.get()) {
While we're here, no need for this .get().

https://codereview.chromium.org/13933029/diff/12001/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_file.h (right):

https://codereview.chromium.org/13933029/diff/12001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.h:71: // Using mtime of the file and the
path, it detects if the index file is
// Using the mtime of the file and its mtime, detects if the index file is
stale.

[pasko-google - do not use, 2013-04-19 11:43:24]

https://chromiumcodereview.appspot.com/13933029/diff/12001/net/disk_cache/sim...
File net/disk_cache/simple/simple_index.cc (right):

https://chromiumcodereview.appspot.com/13933029/diff/12001/net/disk_cache/sim...
net/disk_cache/simple/simple_index.cc:204: index_file_entries =
SimpleIndexFile::LoadFromDisk(index_filename);
It is a little hard to follow the logic of deciding when to load from disk. When
the index file is not present, we try to look at it in two different places.
Simpler would be to teach SimpleIndexFile::LoadFromDisk to return the state of
the index (not present, stale) and also provide the entries.

[felipeg, 2013-04-19 12:20:29]

https://chromiumcodereview.appspot.com/13933029/diff/12001/net/disk_cache/sim...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://chromiumcodereview.appspot.com/13933029/diff/12001/net/disk_cache/sim...
net/disk_cache/simple/simple_entry_impl.cc:69: // way we never leak files. 
CreationOperationComplete will remove the entry
On 2013/04/19 11:30:17, gavinp wrote:
> nit: one space after .

Done.

https://chromiumcodereview.appspot.com/13933029/diff/12001/net/disk_cache/sim...
File net/disk_cache/simple/simple_index.cc (right):

https://chromiumcodereview.appspot.com/13933029/diff/12001/net/disk_cache/sim...
net/disk_cache/simple/simple_index.cc:12: #include "base/file_util.h"
On 2013/04/19 11:30:17, gavinp wrote:
> Once is probably enough.

Done.

https://chromiumcodereview.appspot.com/13933029/diff/12001/net/disk_cache/sim...
net/disk_cache/simple/simple_index.cc:204: index_file_entries =
SimpleIndexFile::LoadFromDisk(index_filename);
On 2013/04/19 11:30:17, gavinp wrote:
> Add a TODO commenting that we probably could load a stale index and use it for
> something.
> 
> As well, in this code path, an empty index is "always" "invalid". Why not have
a
> bool out_error returned by LoadFromDisk, so we can sucessfully load empty
> indexes?
> 
> For that matter, it's a bit outside of this CL, but why is it an error to
create
> an index in a brand new cache?
> 

Done.

https://chromiumcodereview.appspot.com/13933029/diff/12001/net/disk_cache/sim...
net/disk_cache/simple/simple_index.cc:204: index_file_entries =
SimpleIndexFile::LoadFromDisk(index_filename);
On 2013/04/19 11:43:24, pasko wrote:
> It is a little hard to follow the logic of deciding when to load from disk.
When
> the index file is not present, we try to look at it in two different places.
> Simpler would be to teach SimpleIndexFile::LoadFromDisk to return the state of
> the index (not present, stale) and also provide the entries.

Done.

https://chromiumcodereview.appspot.com/13933029/diff/12001/net/disk_cache/sim...
net/disk_cache/simple/simple_index.cc:207: if (!index_file_entries.get()) {
On 2013/04/19 11:30:17, gavinp wrote:
> While we're here, no need for this .get().

Done.

https://chromiumcodereview.appspot.com/13933029/diff/12001/net/disk_cache/sim...
File net/disk_cache/simple/simple_index_file.h (right):

https://chromiumcodereview.appspot.com/13933029/diff/12001/net/disk_cache/sim...
net/disk_cache/simple/simple_index_file.h:71: // Using mtime of the file and the
path, it detects if the index file is
On 2013/04/19 11:30:17, gavinp wrote:
> // Using the mtime of the file and its mtime, detects if the index file is
> stale.

Done.

[gavinp, 2013-04-19 12:23:13]

wait for Egor's LGTM, you have mine if you do that and update the description as
discussed above, and address my one nit below.

https://chromiumcodereview.appspot.com/13933029/diff/11002/net/disk_cache/sim...
File net/disk_cache/simple/simple_index.cc (right):

https://chromiumcodereview.appspot.com/13933029/diff/11002/net/disk_cache/sim...
net/disk_cache/simple/simple_index.cc:203: bool force_index_flush = false;
This whole function is a lot easier to read. Thanks.

https://chromiumcodereview.appspot.com/13933029/diff/11002/net/disk_cache/sim...
File net/disk_cache/simple/simple_index_file.h (right):

https://chromiumcodereview.appspot.com/13933029/diff/11002/net/disk_cache/sim...
net/disk_cache/simple/simple_index_file.h:73: static bool IsIndexFileStale(const
base::FilePath& index_filename);
This can be private now, right?

[felipeg, 2013-04-19 12:55:58]

https://chromiumcodereview.appspot.com/13933029/diff/11002/net/disk_cache/sim...
File net/disk_cache/simple/simple_index.cc (right):

https://chromiumcodereview.appspot.com/13933029/diff/11002/net/disk_cache/sim...
net/disk_cache/simple/simple_index.cc:203: bool force_index_flush = false;
On 2013/04/19 12:23:13, gavinp wrote:
> This whole function is a lot easier to read. Thanks.

Done.

https://chromiumcodereview.appspot.com/13933029/diff/11002/net/disk_cache/sim...
File net/disk_cache/simple/simple_index_file.h (right):

https://chromiumcodereview.appspot.com/13933029/diff/11002/net/disk_cache/sim...
net/disk_cache/simple/simple_index_file.h:73: static bool IsIndexFileStale(const
base::FilePath& index_filename);
On 2013/04/19 12:23:13, gavinp wrote:
> This can be private now, right?

Done.

[commit-bot: I haz the power, 2013-04-19 12:56:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/felipeg@chromium.org/13933029/4007

[felipeg_google, 2013-04-19 12:58:17]

ptal again



On Fri, Apr 19, 2013 at 2:56 PM, <commit-bot@chromium.org> wrote:

> CQ is trying da patch. Follow status at
> https://chromium-status.**appspot.com/cq/felipeg@**
>
chromium.org/13933029/4007<https://chromium-status.appspot.com/cq/felipeg@chromium.org/13933029/4007>
>
>
>
https://chromiumcodereview.**appspot.com/13933029/<https://chromiumcodereview...
>

[pasko-google - do not use, 2013-04-19 13:05:56]

lgtm

Thanks, now easier to read, at least for me

[commit-bot: I haz the power, 2013-04-19 13:08:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/felipeg@chromium.org/13933029/4007

[commit-bot: I haz the power, 2013-04-19 15:05:50]

Change committed as 195197