Introduce parent and child entries for MemEntryImpl...

Introduce parent and child entries for MemEntryImpl
Defines enums for kParentEntry and kChildEntry in
MemEntryImpl. Also has code in MemBackendImpl to
create a slave entry. Parent entries are non-sparse
entries until sparse API are called on them, and
they would start to keep a list of child entries.
Child entries hold partial content and are not
susposed to be accessible from the public and
are managed by the parent entry that created it.
Child entries are registered in the backend's
ranking list to allow individual eviction. More
details about how child entries are to be used
are in the comments.

TEST=DiskCacheEntryTest.MemoryOnlyEnumerationWithSlaveEntries


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

[Alpha Left Google, 2009-06-10 22:40:54]

The first bit of change that defines two types of memory cache entries, master
and slave. Also have TODOs in the code to illustrate how I'm going to manage
these two types of entries.

[rvargas (doing something else), 2009-06-11 20:23:05]

The approach seems fine to me.

I guess there are a few places that may change significantly once you start
worrying about actual sparse IO, for instance, how ReadData will behave
depending of the entry type.

http://codereview.chromium.org/120004/diff/12/1010
File net/disk_cache/backend_unittest.cc (right):

http://codereview.chromium.org/120004/diff/12/1010#newcode1279
Line 1279: // Test that slave entries in a memory cache backend is not visible
from public.
nit: are not visible from enumerations.

http://codereview.chromium.org/120004/diff/12/1010#newcode1280
Line 1280: TEST_F(DiskCacheBackendTest, MemoryOnlySlaveEntry) {
This test should not be in this file. Use entry_unittest instead. (This is not
part of the contract of a backend)

http://codereview.chromium.org/120004/diff/12/1010#newcode1309
Line 1309: BackendDoomAll();
nit: No need to doom all entries here.

http://codereview.chromium.org/120004/diff/12/1006
File net/disk_cache/mem_backend_impl.cc (right):

http://codereview.chromium.org/120004/diff/12/1006#newcode258
Line 258: MemEntryImpl* cache_entry = new MemEntryImpl(this);
Maybe it is better to expose a separate method to insert an entry on the
rankings list so that you don't have to go from entry to backed and to entry
again just to create the slave, and we can keep most of the code on the side of
the entry.

http://codereview.chromium.org/120004/diff/12/1007
File net/disk_cache/mem_entry_impl.h (right):

http://codereview.chromium.org/120004/diff/12/1007#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
nit: upgrade to 2006-2009

http://codereview.chromium.org/120004/diff/12/1007#newcode6
Line 6: #define NET_DISK_CACHE_MEM_ENTRY_IMPL_H__
nit: upgrade to a single trailing underscore.

http://codereview.chromium.org/120004/diff/12/1007#newcode16
Line 16: // types of entries, Master and Slave. The system of having two types
of entries
nit: Could we use some other names? Maybe parent and child?

http://codereview.chromium.org/120004/diff/12/1007#newcode17
Line 17: // is to handle sparse caching.
nit: Reword a little. Maybe "We use two... in order to support..."

http://codereview.chromium.org/120004/diff/12/1007#newcode18
Line 18: // A master entry is a regular entry until sparse API is invoked (i.e.
nit: What is a regular entry? How are you going to differentiate an entry that
already points to a list of entries from an entry that cannot be used with
sparse methods?

http://codereview.chromium.org/120004/diff/12/1007#newcode23
Line 23: // A slave entry is used to carry partial cache content, non-sparse API
like
nit: "content; non-sparse methods"

http://codereview.chromium.org/120004/diff/12/1007#newcode24
Line 24: // ReadData and WriteData cannot be applied on them. The lifetime of a
slave
nit: on -> to them.

http://codereview.chromium.org/120004/diff/12/1007#newcode25
Line 25: // entry is managed by the master entry that created it. A slave entry
does not
nit: except that the entry can be evicted.

http://codereview.chromium.org/120004/diff/12/1007#newcode32
Line 32: MASTER_ENTRY,
nit: You can use kXxxYyy now

http://codereview.chromium.org/120004/diff/12/1007#newcode85
Line 85: EntryType type() { return type_; }
nit: use another line (all other accessors are doing so).

[Alpha Left Google, 2009-06-12 22:16:15]

Sorry for the delay. I addressed all the problems you mentioned.

http://codereview.chromium.org/120004/diff/12/1010
File net/disk_cache/backend_unittest.cc (right):

http://codereview.chromium.org/120004/diff/12/1010#newcode1279
Line 1279: // Test that slave entries in a memory cache backend is not visible
from public.
On 2009/06/11 20:23:05, rvargas wrote:
> nit: are not visible from enumerations.

Done.

http://codereview.chromium.org/120004/diff/12/1010#newcode1280
Line 1280: TEST_F(DiskCacheBackendTest, MemoryOnlySlaveEntry) {
On 2009/06/11 20:23:05, rvargas wrote:
> This test should not be in this file. Use entry_unittest instead. (This is not
> part of the contract of a backend)

Done.

http://codereview.chromium.org/120004/diff/12/1010#newcode1309
Line 1309: BackendDoomAll();
On 2009/06/11 20:23:05, rvargas wrote:
> nit: No need to doom all entries here.

Done.

http://codereview.chromium.org/120004/diff/12/1006
File net/disk_cache/mem_backend_impl.cc (right):

http://codereview.chromium.org/120004/diff/12/1006#newcode258
Line 258: MemEntryImpl* cache_entry = new MemEntryImpl(this);
On 2009/06/11 20:23:05, rvargas wrote:
> Maybe it is better to expose a separate method to insert an entry on the
> rankings list so that you don't have to go from entry to backed and to entry
> again just to create the slave, and we can keep most of the code on the side
of
> the entry.

Sounds good.

http://codereview.chromium.org/120004/diff/12/1007
File net/disk_cache/mem_entry_impl.h (right):

http://codereview.chromium.org/120004/diff/12/1007#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/06/11 20:23:05, rvargas wrote:
> nit: upgrade to 2006-2009

Done.

http://codereview.chromium.org/120004/diff/12/1007#newcode6
Line 6: #define NET_DISK_CACHE_MEM_ENTRY_IMPL_H__
On 2009/06/11 20:23:05, rvargas wrote:
> nit: upgrade to a single trailing underscore.

Done.

http://codereview.chromium.org/120004/diff/12/1007#newcode16
Line 16: // types of entries, Master and Slave. The system of having two types
of entries
On 2009/06/11 20:23:05, rvargas wrote:
> nit: Could we use some other names? Maybe parent and child?

Done.

http://codereview.chromium.org/120004/diff/12/1007#newcode17
Line 17: // is to handle sparse caching.
On 2009/06/11 20:23:05, rvargas wrote:
> nit: Reword a little. Maybe "We use two... in order to support..."

Done.

http://codereview.chromium.org/120004/diff/12/1007#newcode18
Line 18: // A master entry is a regular entry until sparse API is invoked (i.e.
On 2009/06/11 20:23:05, rvargas wrote:
> nit: What is a regular entry? How are you going to differentiate an entry that
> already points to a list of entries from an entry that cannot be used with
> sparse methods?

Done.

http://codereview.chromium.org/120004/diff/12/1007#newcode23
Line 23: // A slave entry is used to carry partial cache content, non-sparse API
like
On 2009/06/11 20:23:05, rvargas wrote:
> nit: "content; non-sparse methods"

Done.

http://codereview.chromium.org/120004/diff/12/1007#newcode24
Line 24: // ReadData and WriteData cannot be applied on them. The lifetime of a
slave
On 2009/06/11 20:23:05, rvargas wrote:
> nit: on -> to them.

Done.

http://codereview.chromium.org/120004/diff/12/1007#newcode25
Line 25: // entry is managed by the master entry that created it. A slave entry
does not
On 2009/06/11 20:23:05, rvargas wrote:
> nit: except that the entry can be evicted.

Done.

http://codereview.chromium.org/120004/diff/12/1007#newcode32
Line 32: MASTER_ENTRY,
On 2009/06/11 20:23:05, rvargas wrote:
> nit: You can use kXxxYyy now

Done.

http://codereview.chromium.org/120004/diff/12/1007#newcode85
Line 85: EntryType type() { return type_; }
On 2009/06/11 20:23:05, rvargas wrote:
> nit: use another line (all other accessors are doing so).

Done.

[rvargas (doing something else), 2009-06-13 22:07:48]

Thanks. Looks good.

http://codereview.chromium.org/120004/diff/3021/3026
File net/disk_cache/entry_unittest.cc (right):

http://codereview.chromium.org/120004/diff/3021/3026#newcode14
Line 14: #include "net/disk_cache/mem_backend_impl.h"
nit: I don't think you need this header.

http://codereview.chromium.org/120004/diff/3021/3026#newcode825
Line 825: // Test that child entries in a memory cache backend is not visible
from
nit: s/is/are

http://codereview.chromium.org/120004/diff/3021/3026#newcode827
Line 827: TEST_F(DiskCacheEntryTest, MemoryOnlySlaveEntry) {
nit:l fix the name.

http://codereview.chromium.org/120004/diff/3021/3026#newcode836
Line 836: ASSERT_TRUE(mem_cache_->CreateEntry("parent", &entry));
nit: you can use cache_ here.

http://codereview.chromium.org/120004/diff/3021/3026#newcode843
Line 843: EXPECT_TRUE(child_entry->CreateChildEntry(parent_entry));
I'd prefer testing this through the public interface of the class (with an
action that triggers creating a child). If you really want to test this method
implementation maybe it is better to make the test class a friend of
MemEntryImpl.

http://codereview.chromium.org/120004/diff/3021/3026#newcode859
Line 859: EXPECT_TRUE(cache_->DoomAllEntries());
There is no need to doom all entries here. If you want to test the behavior of
doom with a mix of entries I'd suggest writing a specific test for that.

http://codereview.chromium.org/120004/diff/3021/3022
File net/disk_cache/mem_backend_impl.cc (right):

http://codereview.chromium.org/120004/diff/3021/3022#newcode167
Line 167: }
nit: instead of adding this, remove them from line 171... to be consistent with
this file.

http://codereview.chromium.org/120004/diff/3021/3022#newcode182
Line 182: // Break the loop until there is no more entry or the entry is too
old.
nit: "Break the loop when there are no more entries or"

http://codereview.chromium.org/120004/diff/3021/3025
File net/disk_cache/mem_entry_impl.cc (right):

http://codereview.chromium.org/120004/diff/3021/3025#newcode76
Line 76: // used (i.e. opened).
I'm not so sure about this, but we need IO code to be sure.

http://codereview.chromium.org/120004/diff/3021/3023
File net/disk_cache/mem_entry_impl.h (right):

http://codereview.chromium.org/120004/diff/3021/3023#newcode17
Line 17: // A parent entry is non-sparse until sparse API is invoked (i.e.
nit: "until a sparse method"... or "one of the methods of the sparse API" or
something like that :)

http://codereview.chromium.org/120004/diff/3021/3023#newcode60
Line 60: bool CreateChildEntry(MemEntryImpl* parent);
nit: why is this public?

http://codereview.chromium.org/120004/diff/3021/3023#newcode84
Line 84: EntryType type() {
nit: this can be const.

[darin (slow to review), 2009-06-14 05:17:12]

please remember to update the description of this CL since master/slave appear
to have been renamed parent/child.

[Alpha Left Google, 2009-06-15 18:13:29]

To access the private method we need to keep the whole DiskCacheEntryTest in
disk_cache namespace..

http://codereview.chromium.org/120004/diff/3021/3026
File net/disk_cache/entry_unittest.cc (right):

http://codereview.chromium.org/120004/diff/3021/3026#newcode14
Line 14: #include "net/disk_cache/mem_backend_impl.h"
On 2009/06/13 22:07:48, rvargas wrote:
> nit: I don't think you need this header.

Done.

http://codereview.chromium.org/120004/diff/3021/3026#newcode825
Line 825: // Test that child entries in a memory cache backend is not visible
from
On 2009/06/13 22:07:48, rvargas wrote:
> nit: s/is/are

Done.

http://codereview.chromium.org/120004/diff/3021/3026#newcode827
Line 827: TEST_F(DiskCacheEntryTest, MemoryOnlySlaveEntry) {
On 2009/06/13 22:07:48, rvargas wrote:
> nit:l fix the name.

Done.

http://codereview.chromium.org/120004/diff/3021/3026#newcode836
Line 836: ASSERT_TRUE(mem_cache_->CreateEntry("parent", &entry));
On 2009/06/13 22:07:48, rvargas wrote:
> nit: you can use cache_ here.

Done.

http://codereview.chromium.org/120004/diff/3021/3026#newcode843
Line 843: EXPECT_TRUE(child_entry->CreateChildEntry(parent_entry));
On 2009/06/13 22:07:48, rvargas wrote:
> I'd prefer testing this through the public interface of the class (with an
> action that triggers creating a child). If you really want to test this method
> implementation maybe it is better to make the test class a friend of
> MemEntryImpl.

Through a public method that triggers the creation of child would probably be
outside of the scope of this test.

http://codereview.chromium.org/120004/diff/3021/3026#newcode859
Line 859: EXPECT_TRUE(cache_->DoomAllEntries());
On 2009/06/13 22:07:48, rvargas wrote:
> There is no need to doom all entries here. If you want to test the behavior of
> doom with a mix of entries I'd suggest writing a specific test for that.

Done.

http://codereview.chromium.org/120004/diff/3021/3022
File net/disk_cache/mem_backend_impl.cc (right):

http://codereview.chromium.org/120004/diff/3021/3022#newcode167
Line 167: }
On 2009/06/13 22:07:48, rvargas wrote:
> nit: instead of adding this, remove them from line 171... to be consistent
with
> this file.

Done.

http://codereview.chromium.org/120004/diff/3021/3022#newcode182
Line 182: // Break the loop until there is no more entry or the entry is too
old.
On 2009/06/13 22:07:48, rvargas wrote:
> nit: "Break the loop when there are no more entries or"

Done.

http://codereview.chromium.org/120004/diff/3021/3023
File net/disk_cache/mem_entry_impl.h (right):

http://codereview.chromium.org/120004/diff/3021/3023#newcode17
Line 17: // A parent entry is non-sparse until sparse API is invoked (i.e.
On 2009/06/13 22:07:48, rvargas wrote:
> nit: "until a sparse method"... or "one of the methods of the sparse API" or
> something like that :)

Done.

http://codereview.chromium.org/120004/diff/3021/3023#newcode60
Line 60: bool CreateChildEntry(MemEntryImpl* parent);
On 2009/06/13 22:07:48, rvargas wrote:
> nit: why is this public?

This should be private. :)

http://codereview.chromium.org/120004/diff/3021/3023#newcode84
Line 84: EntryType type() {
On 2009/06/13 22:07:48, rvargas wrote:
> nit: this can be const.

Done.

[rvargas (doing something else), 2009-06-15 18:48:48]

OK.

http://codereview.chromium.org/120004/diff/3021/3026
File net/disk_cache/entry_unittest.cc (right):

http://codereview.chromium.org/120004/diff/3021/3026#newcode843
Line 843: EXPECT_TRUE(child_entry->CreateChildEntry(parent_entry));
sure, but does it compile without being a friend?

On 2009/06/15 18:13:30, Alpha wrote:
> On 2009/06/13 22:07:48, rvargas wrote:
> > I'd prefer testing this through the public interface of the class (with an
> > action that triggers creating a child). If you really want to test this
method
> > implementation maybe it is better to make the test class a friend of
> > MemEntryImpl.
> 
> Through a public method that triggers the creation of child would probably be
> outside of the scope of this test.

http://codereview.chromium.org/120004/diff/3030/2014#newcode22
Line 22: 
Don't add this namespace. Unit tests should run outside the namespace they are
testing.

http://codereview.chromium.org/120004/diff/3030/2014#newcode862
Line 862: 
I'm confused about this. Why do you have to doom the child at all? (or the
parent).

[Alpha Left Google, 2009-06-15 20:05:51]

http://codereview.chromium.org/120004/diff/3021/3026
File net/disk_cache/entry_unittest.cc (right):

http://codereview.chromium.org/120004/diff/3021/3026#newcode843
Line 843: EXPECT_TRUE(child_entry->CreateChildEntry(parent_entry));
Let's keep CreateChildEntry public so we can access this method from the test
until this method is actually used to create child entries from parent entry.
Now we need the FRIEND_TEST AND DiskCacheEntryTest be in namespace disk_cache to
be able to access this private method. I'll update MemCacheEntry shortly to
trigger the creation of child entries.

http://codereview.chromium.org/120004/diff/3030/2014#newcode862
Line 862: 
On 2009/06/15 18:48:48, rvargas wrote:
> I'm confused about this. Why do you have to doom the child at all? (or the
> parent).

TearDown dooms master entries only. If we don't doom this child entry manually
there's be a leak.

[Alpha Left Google, 2009-06-15 20:07:32]

http://codereview.chromium.org/120004/diff/3030/2014
File net/disk_cache/entry_unittest.cc (right):

http://codereview.chromium.org/120004/diff/3030/2014#newcode862
Line 862: // internally. Now we have to doom this child entry manually.
On 2009/06/15 20:05:51, Alpha wrote:
> On 2009/06/15 18:48:48, rvargas wrote:
> > I'm confused about this. Why do you have to doom the child at all? (or the
> > parent).
> 
> TearDown dooms master entries only. If we don't doom this child entry manually
> there's be a leak.

We don't need this when a parent entry knows to doom it's children when it is
doomed. So we can remove this change shortly in next CL.

[rvargas (doing something else), 2009-06-15 20:37:47]

OK