Make SimpleEntryImpl ref counted.

Make SimpleEntryImpl ref counted.

Upcoming work for multiple operations, CRC calculation, etc... have made the lifetime of the SimpleEntryImpl more tricky. By adding ref counting, asynchronous operations are guaranteed access to the entry after execution.

R=pasko@chromium.org,felipeg@chromium.org,rvargas@chromium.org
BUG=None

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

[gavinp, 2013-04-11 18:17:53]

PTAL.

I think the time has come.

[pasko-google - do not use, 2013-04-12 14:21:43]

Looks nice!
Is it possible to add a test for counting the number of refs to a simple entry
during an operation and after it?

Also please find a place to put a TODO on investigating what backend ops are
called when HttpCache encounters a truncated entry, we would need to implement
those to avoid growing numbers of truncated entries caused by shutdown.

[felipeg, 2013-04-12 15:52:19]

LGTM
only nits

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

https://codereview.chromium.org/13880016/diff/8002/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_entry_impl.cc:111: self_ = NULL;
should we DCHECK the sanity of self_ ?
something like DCHECK(self_.get());
I dont know

https://codereview.chromium.org/13880016/diff/8002/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_entry_impl.h (right):

https://codereview.chromium.org/13880016/diff/8002/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_entry_impl.h:121: // Close(), we call
|self_.reset()| so the Entry will self release after
in the actual code you do
self_ = NULL;
change one or the other

[gavinp, 2013-04-13 08:54:35]

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

https://codereview.chromium.org/13880016/diff/8002/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_entry_impl.cc:111: self_ = NULL;
On 2013/04/12 15:52:19, felipeg1 wrote:
> should we DCHECK the sanity of self_ ?
> something like DCHECK(self_.get());
> I dont know

Done.

https://codereview.chromium.org/13880016/diff/8002/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_entry_impl.h (right):

https://codereview.chromium.org/13880016/diff/8002/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_entry_impl.h:121: // Close(), we call
|self_.reset()| so the Entry will self release after
On 2013/04/12 15:52:19, felipeg1 wrote:
> in the actual code you do
> self_ = NULL;
> change one or the other

Done.

[gavinp, 2013-04-13 08:55:17]

On 2013/04/12 14:21:43, pasko wrote:
> Looks nice!
> Is it possible to add a test for counting the number of refs to a simple entry
> during an operation and after it?

I added a lot of DCHECK to Close().

Were you thinking about a unit test? That could be written, but it would be a
very internals-specific unit test.

> Also please find a place to put a TODO on investigating what backend ops are
> called when HttpCache encounters a truncated entry, we would need to implement
> those to avoid growing numbers of truncated entries caused by shutdown.

[gavinp, 2013-04-13 09:02:57]

+rvargas.

Can you please take a look?

[felipeg, 2013-04-13 12:20:43]

LGTM

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

https://codereview.chromium.org/13880016/diff/22001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:115: DCHECK(!in_use || (in_use &&
HasOneRef()));
would it be possible to be in_use with multiple operations on the fly, and thus,
have more than one ref ?

In any case add a comment here explaining that it is possible or not possible at
all.

[gavinp, 2013-04-13 12:28:09]

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

https://codereview.chromium.org/13880016/diff/22001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:115: DCHECK(!in_use || (in_use &&
HasOneRef()));
On 2013/04/13 12:20:43, felipeg1 wrote:
> would it be possible to be in_use with multiple operations on the fly, and
thus,
> have more than one ref ?
> 
> In any case add a comment here explaining that it is possible or not possible
at
> all.

Done.

[gavinp, 2013-04-15 19:44:04]

Hi Ricardo,

Can you please take a look at this CL?

[rvargas (doing something else), 2013-04-15 22:07:27]

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

https://codereview.chromium.org/13880016/diff/31001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:47: *entry = new_entry.get();
Although strictly not required by anything, it is better to avoid returning an
invalid object pointer in case of failure.

https://codereview.chromium.org/13880016/diff/31001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:118: DCHECK(!in_use || (in_use &&
HasOneRef()));
if (in_use)
  DCHECK();

https://codereview.chromium.org/13880016/diff/31001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:274: Initialize(sync_entry);
Why do you need a self_ reference? It is created at this point, and released at
Close time, so it is active throughout the whole lifetime of the object.

Is it just so that you don't addref/release this explicitly? In that case,
addref/release is better because it is explicit :) (and simpler).

[rvargas (doing something else), 2013-04-15 22:09:44]

And remove references to future CLs from the description... just describe what
this CL does.

[gavinp, 2013-04-16 08:41:55]

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

https://codereview.chromium.org/13880016/diff/31001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:47: *entry = new_entry.get();
On 2013/04/15 22:07:27, rvargas wrote:
> Although strictly not required by anything, it is better to avoid returning an
> invalid object pointer in case of failure.

Done.

https://codereview.chromium.org/13880016/diff/31001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:118: DCHECK(!in_use || (in_use &&
HasOneRef()));
On 2013/04/15 22:07:27, rvargas wrote:
> if (in_use)
>   DCHECK();

Done.

https://codereview.chromium.org/13880016/diff/31001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:274: Initialize(sync_entry);
On 2013/04/15 22:07:27, rvargas wrote:
> Why do you need a self_ reference? It is created at this point, and released
at
> Close time, so it is active throughout the whole lifetime of the object.
> 
> Is it just so that you don't addref/release this explicitly? In that case,
> addref/release is better because it is explicit :) (and simpler).

At least self_ advertises that it doesn't leak references.

We'll probably need to switch to the fragile system eventually anyway, and now
is as good as anytime.

Done.

[gavinp, 2013-04-16 08:43:50]

+pliard

[gavinp, 2013-04-16 09:37:45]

+digit

Some context for newcomers: this CL changes the way we meet the promises of the
disk cache backend API. It's documented in disk_cache.h, the main point of it is
that Entry::Close() is implicitly a release; in some backends, like the
in-memory backend, it will just delete this.

The ref counting is used here to make asynchronous operations that need to refer
to the data stored on the entry easier to write. So right now, since the entry
can have at most one operation in flight, the max refcount is 2 (1 consumer of
the Entry, 1 read/write in flight).

Once we support asynchronous operations, we'll be up to any number of
references.

[digit, 2013-04-16 10:21:32]

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

https://codereview.chromium.org/13880016/diff/55003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:51:
base::Bind(&SimpleSynchronousEntry::OpenEntry, path,
This callback will be destroyed in the WorkerPool thread after its Run() method
is called (see WorkerThread::ThreadMain() in
base/threading/worker_pool_posix.cc)

This will end up destroying the callback created on line 47, which will end up
derefencing 'new_entry' in the worker pool thread.

As such, it looks like this class needs to be RefCountedThreadSafe.

https://codereview.chromium.org/13880016/diff/55003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:72:
base::Bind(&SimpleSynchronousEntry::CreateEntry, path,
exact same issue here.

https://codereview.chromium.org/13880016/diff/55003/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_entry_impl.h (right):

https://codereview.chromium.org/13880016/diff/55003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.h:94: // can return the SimpleEntryImpl
to |entry|. Runs |completion_callback|.
I believe the style guide wants us to place all input parameters before output
ones. Also, should the comment above say "can return the SimpleEntryImpl to
|*entry|" instead?

[gavinp, 2013-04-17 09:52:46]

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

https://codereview.chromium.org/13880016/diff/55003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:51:
base::Bind(&SimpleSynchronousEntry::OpenEntry, path,
On 2013/04/16 10:21:32, digit wrote:
> This callback will be destroyed in the WorkerPool thread after its Run()
method
> is called (see WorkerThread::ThreadMain() in
> base/threading/worker_pool_posix.cc)
> 
> This will end up destroying the callback created on line 47, which will end up
> derefencing 'new_entry' in the worker pool thread.
> 
> As such, it looks like this class needs to be RefCountedThreadSafe.

Great catch.

For moving forward, I went with RefCountedThread safe, as that's the fastest
path to correctness. In practice, we should probably be passing opaque handles
to the entry into the worker pool, as it would be faster (and lock free).

[gavinp, 2013-04-17 09:52:59]

digit, pliard: PTAL now.

[Philippe, 2013-04-17 10:19:22]

Thanks Gavin! This mostly looks good to me. See my comment about scoped_refptr.

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

https://codereview.chromium.org/13880016/diff/55003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:10: #include "base/location.h"
Nit: "base/logging.h" is missing.

https://codereview.chromium.org/13880016/diff/55003/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_entry_impl.h (right):

https://codereview.chromium.org/13880016/diff/55003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.h:89: 
Nit: extra blank line.

https://codereview.chromium.org/13880016/diff/69002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_entry_impl.h (right):

https://codereview.chromium.org/13880016/diff/69002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.h:33: static int
OpenEntry(scoped_refptr<SimpleIndex> index,
I guess passing ref-counted objects around by copying scoped_refptr's is
probably the general consensus in simple cache backend. This brings extra safety
but causes unnecessary AddRef/Release (translating to atomicops).

I believe the "recommended" practice in Chromium is to pass ref-counted objects
around through raw pointers and store them (in long-lived data structures) using
scoped_refptr.

What do you think? Should this be const scoped_refptr<>& at least?

https://codereview.chromium.org/13880016/diff/69002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.h:88: 
Nit: extra blank line.

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

https://codereview.chromium.org/13880016/diff/73001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:108: DCHECK((in_use && !HasOneRef())
|| (!in_use && HasOneRef()));
It is probably ok now but this DCHECK() might be a little brittle in the long
term when passing the ref-counted object around by copying scoped_refptr's (i.e.
increasing the ref-count without actually really increasing the number of
clients).

https://codereview.chromium.org/13880016/diff/73001/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_entry_impl.h (right):

https://codereview.chromium.org/13880016/diff/73001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.h:88: 
Nit: extra blank line.

[gavinp, 2013-04-17 10:56:28]

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

https://codereview.chromium.org/13880016/diff/55003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:10: #include "base/location.h"
On 2013/04/17 10:19:22, Philippe wrote:
> Nit: "base/logging.h" is missing.

Done.

https://codereview.chromium.org/13880016/diff/55003/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_entry_impl.h (right):

https://codereview.chromium.org/13880016/diff/55003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.h:89: 
On 2013/04/17 10:19:22, Philippe wrote:
> Nit: extra blank line.

Done.

https://codereview.chromium.org/13880016/diff/55003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.h:94: // can return the SimpleEntryImpl
to |entry|. Runs |completion_callback|.
On 2013/04/16 10:21:32, digit wrote:
> I believe the style guide wants us to place all input parameters before output
> ones. Also, should the comment above say "can return the SimpleEntryImpl to
> |*entry|" instead?

You're right about the style guide. The challenge is that it's a partially bound
callback, which has to bind from right to left.

https://codereview.chromium.org/13880016/diff/69002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_entry_impl.h (right):

https://codereview.chromium.org/13880016/diff/69002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.h:33: static int
OpenEntry(scoped_refptr<SimpleIndex> index,
On 2013/04/17 10:19:22, Philippe wrote:
> I guess passing ref-counted objects around by copying scoped_refptr's is
> probably the general consensus in simple cache backend. This brings extra
safety
> but causes unnecessary AddRef/Release (translating to atomicops).
> 
> I believe the "recommended" practice in Chromium is to pass ref-counted
objects
> around through raw pointers and store them (in long-lived data structures)
using
> scoped_refptr.
> 
> What do you think? Should this be const scoped_refptr<>& at least?

const scoped_refptr<> seems the right way to go. This came about because of
copy-replace of the WeakPtrs, (which also should perhaps have been references).

Your point about raw pointers is well said though; but we are IO blocked and
have massive correctness concerns because of the racing of IO latency; I like
having the safe passage.

https://codereview.chromium.org/13880016/diff/69002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.h:88: 
On 2013/04/17 10:19:22, Philippe wrote:
> Nit: extra blank line.

Done.

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

https://codereview.chromium.org/13880016/diff/73001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:108: DCHECK((in_use && !HasOneRef())
|| (!in_use && HasOneRef()));
On 2013/04/17 10:19:22, Philippe wrote:
> It is probably ok now but this DCHECK() might be a little brittle in the long
> term when passing the ref-counted object around by copying scoped_refptr's
(i.e.
> increasing the ref-count without actually really increasing the number of
> clients).

Good point. I've added a comment noting that this might be flakey, so future
generations don't lose this observation.

[gavinp, 2013-04-17 11:02:20]

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

https://codereview.chromium.org/13880016/diff/2003/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_entry_impl.cc:256: WorkerPool::PostTask(FROM_HERE,
Wait, worker pool can't post to worker pool.

[felipeg, 2013-04-17 11:48:25]

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

https://codereview.chromium.org/13880016/diff/2003/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_entry_impl.cc:108: bool in_use =
synchronous_entry_in_use_by_worker_;
can you add a comment explaining why you need to save this member variable in a
local variable before calling Release() ?

[Philippe, 2013-04-17 12:04:45]

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

https://codereview.chromium.org/13880016/diff/2003/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_entry_impl.cc:256: WorkerPool::PostTask(FROM_HERE,
On 2013/04/17 11:02:21, gavinp wrote:
> Wait, worker pool can't post to worker pool.

I don't think you will be able to use a RefCountedThreadSafeTrait actually to
specify which thread the ref-counted object needs to be deleted on. The trait is
used by RefCountedThreadSafe through its static methods which doesn't allow you
to inject the IO thread message loop pointer.

You might be able to do something as in:

SimpleEntryImpl::~SimpleEntryImpl() {
  if (io_thread_->BelongsToCurrentThread()) {
    CloseSynchronousEntry(synchronous_entry_);  // Posts the task to the worker
pool.
  } else {
    io_thread_->PostTask(
        FROM_HERE,
        base::Bind(&SimpleEntryImpl::CloseSynchronousEntry,
                   base::Unretained(synchronous_entry_)));
  }
}

Note that CloseSynchronousEntry() would be a static method.

[gavinp, 2013-04-17 13:04:59]

pliard, felipeg: I think this addresses your concerns. What do you think?

[Philippe, 2013-04-17 13:30:50]

lgtm, thanks Gavin!

[gavinp, 2013-04-17 14:55:13]

Committed patchset #21 manually as r194591 (presubmit successful).

[rvargas (doing something else), 2013-04-17 19:56:38]

I don't think index_ should be refcounted. We should keep the weak pointer model
there.

And we should not pass "const scoped_refptr<>&", we should pass a raw pointer,
and only whoever wants to store that for later should use a scoped variable...
in fact, that should also be the behavior for weak pointers.

Why? because it is more convoluted and buries things :(. For example, I know
that is what it says, (so there is no bug or violation of the letter), but the
entry stores the index_ passed with a const and then calls methods that modify
the index_.

https://codereview.chromium.org/13880016/diff/69002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_entry_impl.h (right):

https://codereview.chromium.org/13880016/diff/69002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.h:33: static int
OpenEntry(scoped_refptr<SimpleIndex> index,
On 2013/04/17 10:56:28, gavinp wrote:
> On 2013/04/17 10:19:22, Philippe wrote:
> > I guess passing ref-counted objects around by copying scoped_refptr's is
> > probably the general consensus in simple cache backend. This brings extra
> safety
> > but causes unnecessary AddRef/Release (translating to atomicops).
> > 
> > I believe the "recommended" practice in Chromium is to pass ref-counted
> objects
> > around through raw pointers and store them (in long-lived data structures)
> using
> > scoped_refptr.
> > 
> > What do you think? Should this be const scoped_refptr<>& at least?
> 
> const scoped_refptr<> seems the right way to go. This came about because of
> copy-replace of the WeakPtrs, (which also should perhaps have been
references).
> 
> Your point about raw pointers is well said though; but we are IO blocked and
> have massive correctness concerns because of the racing of IO latency; I like
> having the safe passage.

Note that the recommended practice (Google C++ premier) is to use raw pointers
on arguments (as much as possible), and it is not so much about performance
improvements. So my preference is also to pass a pointer to the object and not a
smart pointer, unless there is a very strong reason to do otherwise. This is a
general comment, without looking at the specifics of this particular method.

https://codereview.chromium.org/13880016/diff/78012/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/13880016/diff/78012/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_backend_impl.cc:81: return
SimpleEntryImpl::OpenEntry(index_, path_, key, entry, callback);
the weak pointer seems like a better approach.

https://codereview.chromium.org/13880016/diff/78012/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_backend_impl.h (right):

https://codereview.chromium.org/13880016/diff/78012/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_backend_impl.h:82: scoped_refptr<SimpleIndex>
index_;
Isn't another way to handle the index? refcounting muddles the ownership and
lifetime of objects so it would be better if the  index has a single and clearly
defined owner (and lifetime determined by the backend).

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

https://codereview.chromium.org/13880016/diff/78012/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:272: // Adding a reference to self
will keep |this| alive after the scope of our
nit: "our callback calling us" -> the callback

https://codereview.chromium.org/13880016/diff/78012/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:278: *out_entry = this;
nit: I believe the "real" reason for Addref is that this line returns "this" to
the caller. While your comment is correct, I think it is more clear to point
this out ('cause you are not saying _why_ we need to extend the life of this
object)

https://codereview.chromium.org/13880016/diff/78012/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_synchronous_entry.h (right):

https://codereview.chromium.org/13880016/diff/78012/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_synchronous_entry.h:63: const
scoped_refptr<base::TaskRunner>& callback_runner,
this should be a plain pointer

[gavinp, 2013-04-18 08:26:38]

On 2013/04/17 19:56:38, rvargas wrote:
> I don't think index_ should be refcounted. We should keep the weak pointer
model
> there.

I was sad to remove the weak pointer as well. I'd believed that since the
SimpleEntryImpl could be destructed on the worker pool that it was unsafe to use
a weakptr in it. That wasn't true; WeakPtr are destructible on any thread. Once
Egor & Phillipe reminded me of that, I knew we're good. So thanks.

> And we should not pass "const scoped_refptr<>&", we should pass a raw pointer,
> and only whoever wants to store that for later should use a scoped variable...
> in fact, that should also be the behavior for weak pointers.
>
> Why? because it is more convoluted and buries things :(. For example, I know
> that is what it says, (so there is no bug or violation of the letter), but the
> entry stores the index_ passed with a const and then calls methods that modify
> the index_.

The placement of const in C++ is confusing. There's no great way to make it
right. And our standard is to use raw pointers. Fixing that in this issue would
put me in pure merge hell; I have a downstream set of patches that are already
very difficult to keep alive.

For the raw pointers, I've created an issue I'll do after I've landed the crc32
change:

crbug.com/233103 ref counted objects should be passed as raw pointers in Simple
Cache code

For the style comments and WeakPtr, I'll upload a patch shortly.

> 
>
https://codereview.chromium.org/13880016/diff/69002/net/disk_cache/simple/sim...
> File net/disk_cache/simple/simple_entry_impl.h (right):
> 
>
https://codereview.chromium.org/13880016/diff/69002/net/disk_cache/simple/sim...
> net/disk_cache/simple/simple_entry_impl.h:33: static int
> OpenEntry(scoped_refptr<SimpleIndex> index,
> On 2013/04/17 10:56:28, gavinp wrote:
> > On 2013/04/17 10:19:22, Philippe wrote:
> > > I guess passing ref-counted objects around by copying scoped_refptr's is
> > > probably the general consensus in simple cache backend. This brings extra
> > safety
> > > but causes unnecessary AddRef/Release (translating to atomicops).
> > > 
> > > I believe the "recommended" practice in Chromium is to pass ref-counted
> > objects
> > > around through raw pointers and store them (in long-lived data structures)
> > using
> > > scoped_refptr.
> > > 
> > > What do you think? Should this be const scoped_refptr<>& at least?
> > 
> > const scoped_refptr<> seems the right way to go. This came about because of
> > copy-replace of the WeakPtrs, (which also should perhaps have been
> references).
> > 
> > Your point about raw pointers is well said though; but we are IO blocked and
> > have massive correctness concerns because of the racing of IO latency; I
like
> > having the safe passage.
> 
> Note that the recommended practice (Google C++ premier) is to use raw pointers
> on arguments (as much as possible), and it is not so much about performance
> improvements. So my preference is also to pass a pointer to the object and not
a
> smart pointer, unless there is a very strong reason to do otherwise. This is a
> general comment, without looking at the specifics of this particular method.
> 
>
https://codereview.chromium.org/13880016/diff/78012/net/disk_cache/simple/sim...
> File net/disk_cache/simple/simple_backend_impl.cc (right):
> 
>
https://codereview.chromium.org/13880016/diff/78012/net/disk_cache/simple/sim...
> net/disk_cache/simple/simple_backend_impl.cc:81: return
> SimpleEntryImpl::OpenEntry(index_, path_, key, entry, callback);
> the weak pointer seems like a better approach.
> 
>
https://codereview.chromium.org/13880016/diff/78012/net/disk_cache/simple/sim...
> File net/disk_cache/simple/simple_backend_impl.h (right):
> 
>
https://codereview.chromium.org/13880016/diff/78012/net/disk_cache/simple/sim...
> net/disk_cache/simple/simple_backend_impl.h:82: scoped_refptr<SimpleIndex>
> index_;
> Isn't another way to handle the index? refcounting muddles the ownership and
> lifetime of objects so it would be better if the  index has a single and
clearly
> defined owner (and lifetime determined by the backend).
> 
>
https://codereview.chromium.org/13880016/diff/78012/net/disk_cache/simple/sim...
> File net/disk_cache/simple/simple_entry_impl.cc (right):
> 
>
https://codereview.chromium.org/13880016/diff/78012/net/disk_cache/simple/sim...
> net/disk_cache/simple/simple_entry_impl.cc:272: // Adding a reference to self
> will keep |this| alive after the scope of our
> nit: "our callback calling us" -> the callback
> 
>
https://codereview.chromium.org/13880016/diff/78012/net/disk_cache/simple/sim...
> net/disk_cache/simple/simple_entry_impl.cc:278: *out_entry = this;
> nit: I believe the "real" reason for Addref is that this line returns "this"
to
> the caller. While your comment is correct, I think it is more clear to point
> this out ('cause you are not saying _why_ we need to extend the life of this
> object)
> 
>
https://codereview.chromium.org/13880016/diff/78012/net/disk_cache/simple/sim...
> File net/disk_cache/simple/simple_synchronous_entry.h (right):
> 
>
https://codereview.chromium.org/13880016/diff/78012/net/disk_cache/simple/sim...
> net/disk_cache/simple/simple_synchronous_entry.h:63: const
> scoped_refptr<base::TaskRunner>& callback_runner,
> this should be a plain pointer

[gavinp, 2013-04-18 09:12:41]

See https://codereview.chromium.org/14022012/

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

https://codereview.chromium.org/13880016/diff/78012/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:272: // Adding a reference to self
will keep |this| alive after the scope of our
On 2013/04/17 19:56:38, rvargas wrote:
> nit: "our callback calling us" -> the callback

Done.

https://codereview.chromium.org/13880016/diff/78012/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:278: *out_entry = this;
On 2013/04/17 19:56:38, rvargas wrote:
> nit: I believe the "real" reason for Addref is that this line returns "this"
to
> the caller. While your comment is correct, I think it is more clear to point
> this out ('cause you are not saying _why_ we need to extend the life of this
> object)

Done.

[felipeg, 2013-04-19 18:12:08]

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

https://codereview.chromium.org/13880016/diff/55003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:51:
base::Bind(&SimpleSynchronousEntry::OpenEntry, path,
On 2013/04/17 09:52:46, gavinp wrote:
> On 2013/04/16 10:21:32, digit wrote:
> > This callback will be destroyed in the WorkerPool thread after its Run()
> method
> > is called (see WorkerThread::ThreadMain() in
> > base/threading/worker_pool_posix.cc)
> > 
> > This will end up destroying the callback created on line 47, which will end
up
> > derefencing 'new_entry' in the worker pool thread.
> > 
> > As such, it looks like this class needs to be RefCountedThreadSafe.
> 
> Great catch.
> 
> For moving forward, I went with RefCountedThread safe, as that's the fastest
> path to correctness. In practice, we should probably be passing opaque handles
> to the entry into the worker pool, as it would be faster (and lock free).

I think this is not true
sync_creation_callback on line 47 will run on io_thread and  the pointer to the
SimpleEntryImpl object must be alive when it calls the
CreationOperationComplete.
So the SimpleEntryImpl object will be destroyed in the io_thread, AFAICT

[gavinp, 2013-04-20 07:28:33]

I think the thread safety is required. Does this make sense?

I have a fix coming for this anyway that will let us go back to standard
refcounting on the SimpleEntryImpl. I'll upload it a bit later today.

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

https://codereview.chromium.org/13880016/diff/78012/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:106: void SimpleEntryImpl::Close() {
***THREE*** In response to the callback from ***TWO***, the client calls
SimpleEntryImpl::Close().

https://codereview.chromium.org/13880016/diff/78012/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:108: Release();  // Balanced in
CreationOperationCompleted().
***FOUR*** Close releases one ref on SimpleEntryImpl.

https://codereview.chromium.org/13880016/diff/78012/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:296: completion_callback.Run(result);
***TWO*** The IO thread runs the task posted just before ***ONE***, and control
flow reaches this callback.

https://codereview.chromium.org/13880016/diff/78012/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_synchronous_entry.cc (right):

https://codereview.chromium.org/13880016/diff/78012/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_synchronous_entry.cc:123:
callback_runner_->PostTask(FROM_HERE, base::Bind(callback, result));
***ONE*** A context switch occurs precisely after running this statement,
leaving the program counter inside of SimpleSynchronousEntry::ReadData(). Note
it's called from a Closure(), and that closure has a ref to |callback|, which
has a |scoped_refptr<SimpleEntryImpl>| as part of it.

https://codereview.chromium.org/13880016/diff/78012/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_synchronous_entry.cc:124: }
***FIVE*** Control flow continues at this point. The SimpleEntryImpl object is
deleted on the worker pool.