Track entries pending Doom in SimpleCache backend.

Track entries pending Doom in SimpleCache backend.

Building on Philippe's recent work with doom operations on entries,
this tracks entries which don't exist in the backend.

The new SimpleBackendImpl::entries_pending_doom_ tracks entries that
are in the process of being doomed. While entries are in this state,
it is not legal to create a new active entry, instead the backend
should wait until the doom is complete.

There's still an outstanding issue of mass dooms (evictions, doombetween...)
not using this new mechanism, which is to be fixed in a followup CL.

R=rdsmith,pliard,ttuttle,pasko
BUG=289542
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222695

[gavinp, 2013-08-28 22:18:39]

This is very early, and doesn't even lint yet, but it's a nice companion to
https://codereview.chromium.org/22859060/ (which it is downstream of).

[Philippe, 2013-08-29 10:16:16]

Thanks Gavin! The general approach looks good to me. I will let you polish and
think about the unit tests before reviewing in detail.

Speaking of unit tests, I have the two unit tests that you had mentioned in the
other CL in my stash (and in /home/pliard/doom_unit_tests.patch). It might be
the right time to add them :) I added them in a completely random place and did
several dirty iterations on them without really knowing where I ended up so you
might have to make a few changes to them.

https://codereview.chromium.org/23486006/diff/1/net/disk_cache/simple/simple_...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/23486006/diff/1/net/disk_cache/simple/simple_...
net/disk_cache/simple/simple_backend_impl.cc:299: base::hash_map<uint64,
Closure>::value_type(entry_hash, Closure()));
The closure that you create here will be the last one that will be run, right?
Should we make it remove the entry hash from |entries_pending_doom_|? We don't
seem to ever remove from |entries_pending_doom_| right now.

[Philippe, 2013-08-29 11:46:14]

https://codereview.chromium.org/23486006/diff/1/net/disk_cache/simple/simple_...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/23486006/diff/1/net/disk_cache/simple/simple_...
net/disk_cache/simple/simple_backend_impl.cc:299: base::hash_map<uint64,
Closure>::value_type(entry_hash, Closure()));
On 2013/08/29 10:16:16, Philippe wrote:
> The closure that you create here will be the last one that will be run, right?
> Should we make it remove the entry hash from |entries_pending_doom_|? We don't
> seem to ever remove from |entries_pending_doom_| right now.

Or maybe instead do an erase() on the iterator returned by find() line 304
(after the closure was copied).

[gavinp, 2013-08-30 13:09:35]

rdsmith, pliard, ttuttle: PTAL.
clamy, pasko: FYI.

This is now polished enough to review. It's downstream of
https://codereview.chromium.org/22859060/ by pliard, which fixes some
problematic races on the SimpleEntryImpl.

The unit test passes with this patch, and fails about 1/70 runs on my machine
without this patch.

ttuttle: I think you'll find this patch is a good basis to rebase the third
stream code on, as it introduces doomed_, a somewhat useful member on
SimpleEntryImpl.

There's some bug fixes I'll upload downstream of this, together with putting the
mass doom operations through this mechanism.

[Philippe, 2013-08-30 13:33:14]

Thanks Gavin! LGTM modulo the tiny nits and the question about the backend
lifetime.

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

https://codereview.chromium.org/23486006/diff/32001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_backend_impl.cc:206: if (backend_weak_ptr) {
I might be wrong but I believe that we have the guarantee here that the backend
will systematically outlive the callback chain since it owns the callback chain.

You would need the weak pointer though if you were pushing tasks to a message
loop (outliving the backend) rather than the |entries_pending_doom_| queue owned
by the backend.

Wdyt?

https://codereview.chromium.org/23486006/diff/32001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_backend_impl.cc:299: void
SimpleBackendImpl::OnDoomStart(const uint64 entry_hash) {
Nit: As much as I like const, this one (and the one line 305) might be slight
overkill :) You can move the one on line 305 to line 307 if you want to keep one
:)

https://codereview.chromium.org/23486006/diff/32001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_backend_impl.cc:302: base::hash_map<uint64,
Closure>::value_type(entry_hash, Closure()));
Nit: any reason not to use the shorter make_pair() here?

https://codereview.chromium.org/23486006/diff/32001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_backend_impl.cc:331: it =
entries_pending_doom_.find(entry_hash);
Nit: wouldn't it be more common to break after '=' instead as you do on line
333? This comment would also apply to line 341, 359, 369 and 386.

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

https://codereview.chromium.org/23486006/diff/32001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.h:267: bool doomed_;
Good change :) I like the fact that the validity of backend_ now only reflects
whether the entry is outliving the backend.

[gavinp, 2013-08-30 18:03:10]

Thanks for the review, Philippe!

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

https://codereview.chromium.org/23486006/diff/32001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_backend_impl.cc:206: if (backend_weak_ptr) {
On 2013/08/30 13:33:14, Philippe wrote:
> I might be wrong but I believe that we have the guarantee here that the
backend
> will systematically outlive the callback chain since it owns the callback
chain.
> 
> You would need the weak pointer though if you were pushing tasks to a message
> loop (outliving the backend) rather than the |entries_pending_doom_| queue
owned
> by the backend.
> 
> Wdyt?

I think you're right. Done.

https://codereview.chromium.org/23486006/diff/32001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_backend_impl.cc:299: void
SimpleBackendImpl::OnDoomStart(const uint64 entry_hash) {
On 2013/08/30 13:33:14, Philippe wrote:
> Nit: As much as I like const, this one (and the one line 305) might be slight
> overkill :) You can move the one on line 305 to line 307 if you want to keep
one
> :)

Done.

https://codereview.chromium.org/23486006/diff/32001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_backend_impl.cc:302: base::hash_map<uint64,
Closure>::value_type(entry_hash, Closure()));
On 2013/08/30 13:33:14, Philippe wrote:
> Nit: any reason not to use the shorter make_pair() here?

No, none. Done.

https://codereview.chromium.org/23486006/diff/32001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_backend_impl.cc:331: it =
entries_pending_doom_.find(entry_hash);
On 2013/08/30 13:33:14, Philippe wrote:
> Nit: wouldn't it be more common to break after '=' instead as you do on line
> 333? This comment would also apply to line 341, 359, 369 and 386.

Done.

[Deprecated (see juliatuttle), 2013-09-03 16:15:10]

More comments later, but since I just noticed this:

https://chromiumcodereview.appspot.com/23486006/diff/69001/net/disk_cache/sim...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://chromiumcodereview.appspot.com/23486006/diff/69001/net/disk_cache/sim...
net/disk_cache/simple/simple_backend_impl.cc:328: it->second =
ChainOperationIntoClosure(operation, callback, it->second);
This is really clever, but I think it's less clear and uses more overhead (an
extra closure for every callback past the first) than making
entries_pending_doom_ a map<uint64, vector<Closure> >.

[Philippe, 2013-09-03 16:24:33]

https://chromiumcodereview.appspot.com/23486006/diff/69001/net/disk_cache/sim...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://chromiumcodereview.appspot.com/23486006/diff/69001/net/disk_cache/sim...
net/disk_cache/simple/simple_backend_impl.cc:328: it->second =
ChainOperationIntoClosure(operation, callback, it->second);
On 2013/09/03 16:15:11, ttuttle wrote:
> This is really clever, but I think it's less clear and uses more overhead (an
> extra closure for every callback past the first) than making
> entries_pending_doom_ a map<uint64, vector<Closure> >.

I agree at a glance that a map/multimap would look clearer. I'm not sure how
this would look like in practice though given the fact that the callbacks can be
executed asynchronously (i.e. it seems that we can't just iterate over the
callbacks vector and run each callback as we would do synchronously). What do
you think?

[Philippe, 2013-09-03 16:25:58]

https://chromiumcodereview.appspot.com/23486006/diff/69001/net/disk_cache/sim...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://chromiumcodereview.appspot.com/23486006/diff/69001/net/disk_cache/sim...
net/disk_cache/simple/simple_backend_impl.cc:328: it->second =
ChainOperationIntoClosure(operation, callback, it->second);
On 2013/09/03 16:24:33, Philippe wrote:
> On 2013/09/03 16:15:11, ttuttle wrote:
> > This is really clever, but I think it's less clear and uses more overhead
(an
> > extra closure for every callback past the first) than making
> > entries_pending_doom_ a map<uint64, vector<Closure> >.
> 
> I agree at a glance that a map/multimap would look clearer. I'm not sure how
> this would look like in practice though given the fact that the callbacks can
be
> executed asynchronously (i.e. it seems that we can't just iterate over the
> callbacks vector and run each callback as we would do synchronously). What do
> you think?

s/how this/what this :)

[Randy Smith (Not in Mondays), 2013-09-03 19:04:18]

It looks to me as if this doesn't address the race in the case of the mass doom
of entries not in the active pool.  If that's right, would you be willing to
call it out in the CL description?

I'd love to have something (bug?) where we tracked our overall progress towards
completely eliminating this race.  

With the above and the below, this LGTM.

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

https://codereview.chromium.org/23486006/diff/69001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_backend_impl.cc:328: it->second =
ChainOperationIntoClosure(operation, callback, it->second);
On 2013/09/03 16:25:58, Philippe wrote:
> On 2013/09/03 16:24:33, Philippe wrote:
> > On 2013/09/03 16:15:11, ttuttle wrote:
> > > This is really clever, but I think it's less clear and uses more overhead
> (an
> > > extra closure for every callback past the first) than making
> > > entries_pending_doom_ a map<uint64, vector<Closure> >.
> > 
> > I agree at a glance that a map/multimap would look clearer. I'm not sure how
> > this would look like in practice though given the fact that the callbacks
can
> be
> > executed asynchronously (i.e. it seems that we can't just iterate over the
> > callbacks vector and run each callback as we would do synchronously). What
do
> > you think?
> 
> s/how this/what this :)

I think I'd prefer a map to a vector as Thomas suggests over the current code,
though I reserve the right to change my mind if the new code looks horrid :-}. 
But that's because I don't see what's stopping us from iterating over the
callbacks and calling each one.  The ones for which that will produce an async
post of some sort will have mechanisms in place (pending_operations_) to
serialize what needs to be serialized.  Can you give me a clearer idea of what
problems you see coming from the naive for () { callback.Run(); } loop?

https://codereview.chromium.org/23486006/diff/69001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_backend_impl.cc:500: const std::string& key) {
Suggestion: DCHECK(entry_hash, simple_util::GetEntryHashKey(key))?

[Philippe, 2013-09-04 08:55:48]

https://chromiumcodereview.appspot.com/23486006/diff/69001/net/disk_cache/sim...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://chromiumcodereview.appspot.com/23486006/diff/69001/net/disk_cache/sim...
net/disk_cache/simple/simple_backend_impl.cc:328: it->second =
ChainOperationIntoClosure(operation, callback, it->second);
On 2013/09/03 19:04:19, rdsmith wrote:
> On 2013/09/03 16:25:58, Philippe wrote:
> > On 2013/09/03 16:24:33, Philippe wrote:
> > > On 2013/09/03 16:15:11, ttuttle wrote:
> > > > This is really clever, but I think it's less clear and uses more
overhead
> > (an
> > > > extra closure for every callback past the first) than making
> > > > entries_pending_doom_ a map<uint64, vector<Closure> >.
> > > 
> > > I agree at a glance that a map/multimap would look clearer. I'm not sure
how
> > > this would look like in practice though given the fact that the callbacks
> can
> > be
> > > executed asynchronously (i.e. it seems that we can't just iterate over the
> > > callbacks vector and run each callback as we would do synchronously). What
> do
> > > you think?
> > 
> > s/how this/what this :)
> 
> I think I'd prefer a map to a vector as Thomas suggests over the current code,
> though I reserve the right to change my mind if the new code looks horrid :-}.

> But that's because I don't see what's stopping us from iterating over the
> callbacks and calling each one.  The ones for which that will produce an async
> post of some sort will have mechanisms in place (pending_operations_) to
> serialize what needs to be serialized.  Can you give me a clearer idea of what
> problems you see coming from the naive for () { callback.Run(); } loop?

Right, this should work actually thanks to the operations serialization
mechanism in SimpleEntryImpl.

[Philippe, 2013-09-11 11:46:10]

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

https://codereview.chromium.org/23486006/diff/57001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.h:250: const
base::WeakPtr<SimpleBackendImpl> backend_;
FYI, I'm also doing this change and the one below (as well as the one in the .cc
file) to fix the user-after-free (crbug.com/288963).

[gavinp, 2013-09-11 15:25:57]

ttuttle: PTAL.

Remediated, rebased, and description updated per rdsmith.

What do y'all think? I have two LGTMs right now. I'll let try jobs bake, and
send this down the road to CQ after ttuttle's gotten back to us.

https://chromiumcodereview.appspot.com/23486006/diff/69001/net/disk_cache/sim...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://chromiumcodereview.appspot.com/23486006/diff/69001/net/disk_cache/sim...
net/disk_cache/simple/simple_backend_impl.cc:328: it->second =
ChainOperationIntoClosure(operation, callback, it->second);
On 2013/09/04 08:55:49, Philippe wrote:
> On 2013/09/03 19:04:19, rdsmith wrote:
> > On 2013/09/03 16:25:58, Philippe wrote:
> > > On 2013/09/03 16:24:33, Philippe wrote:
> > > > On 2013/09/03 16:15:11, ttuttle wrote:
> > > > > This is really clever, but I think it's less clear and uses more
> overhead
> > > (an
> > > > > extra closure for every callback past the first) than making
> > > > > entries_pending_doom_ a map<uint64, vector<Closure> >.
> > > > 
> > > > I agree at a glance that a map/multimap would look clearer. I'm not sure
> how
> > > > this would look like in practice though given the fact that the
callbacks
> > can
> > > be
> > > > executed asynchronously (i.e. it seems that we can't just iterate over
the
> > > > callbacks vector and run each callback as we would do synchronously).
What
> > do
> > > > you think?
> > > 
> > > s/how this/what this :)
> > 
> > I think I'd prefer a map to a vector as Thomas suggests over the current
code,
> > though I reserve the right to change my mind if the new code looks horrid
:-}.
> 
> > But that's because I don't see what's stopping us from iterating over the
> > callbacks and calling each one.  The ones for which that will produce an
async
> > post of some sort will have mechanisms in place (pending_operations_) to
> > serialize what needs to be serialized.  Can you give me a clearer idea of
what
> > problems you see coming from the naive for () { callback.Run(); } loop?
> 
> Right, this should work actually thanks to the operations serialization
> mechanism in SimpleEntryImpl.

The latest upload uses a vector, and the code is very readable. Take a look.

https://chromiumcodereview.appspot.com/23486006/diff/69001/net/disk_cache/sim...
net/disk_cache/simple/simple_backend_impl.cc:500: const std::string& key) {
On 2013/09/03 19:04:19, rdsmith wrote:
> Suggestion: DCHECK(entry_hash, simple_util::GetEntryHashKey(key))?

Done.

[Philippe, 2013-09-11 15:41:31]

Still LGTM

https://chromiumcodereview.appspot.com/23486006/diff/110001/net/disk_cache/si...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://chromiumcodereview.appspot.com/23486006/diff/110001/net/disk_cache/si...
net/disk_cache/simple/simple_backend_impl.cc:320: base::hash_map<uint64,
std::vector<Closure> >::iterator it =
I wonder if it would make sense to factor out this part from here to line 329?
The immediate downside I see is the fact that you would have to call the
factored out function by passing it a callback that would not be lazily created
as it is done right now (only when the hash was found in
|entries_pending_doom_|).

Wdyt?

[Randy Smith (Not in Mondays), 2013-09-11 16:11:19]

Still/the changes LGTM.

[pasko, 2013-09-11 16:21:33]

fly-by note without reviewing:

I was just being nice to everyone and created a bug for tracing work on
Doom/Create race. I was also nice by not dooming the bug, but that's another
story. Gavin, Philippe, please add BUG=289542 to all doom-related changes in
flight (and following) including this one.

[pliard, 2013-09-11 16:24:10]

Thanks Egor for creating the bug and apologies for not doing it!


On Wed, Sep 11, 2013 at 6:21 PM, <pasko@chromium.org> wrote:

> fly-by note without reviewing:
>
> I was just being nice to everyone and created a bug for tracing work on
> Doom/Create race. I was also nice by not dooming the bug, but that's
> another
> story. Gavin, Philippe, please add BUG=289542 to all doom-related changes
> in
> flight (and following) including this one.
>
>
https://codereview.chromium.**org/23486006/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[pliard, 2013-09-11 18:08:44]

Just FYI Gavin, please proceed with the commit when you are ready without
waiting for my CL. I will do the rebase on my side.


On Wed, Sep 11, 2013 at 6:24 PM, Philippe Liard <pliard@google.com> wrote:

> Thanks Egor for creating the bug and apologies for not doing it!
>
>
> On Wed, Sep 11, 2013 at 6:21 PM, <pasko@chromium.org> wrote:
>
>> fly-by note without reviewing:
>>
>> I was just being nice to everyone and created a bug for tracing work on
>> Doom/Create race. I was also nice by not dooming the bug, but that's
>> another
>> story. Gavin, Philippe, please add BUG=289542 to all doom-related changes
>> in
>> flight (and following) including this one.
>>
>>
https://codereview.chromium.**org/23486006/<https://codereview.chromium.org/2...
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Deprecated (see juliatuttle), 2013-09-11 19:03:20]

lgtm.

https://codereview.chromium.org/23486006/diff/110001/net/disk_cache/entry_uni...
File net/disk_cache/entry_unittest.cc (right):

https://codereview.chromium.org/23486006/diff/110001/net/disk_cache/entry_uni...
net/disk_cache/entry_unittest.cc:3032: disk_cache::Entry* null = NULL;
Is there a reason you're not just comparing directly to NULL?

https://codereview.chromium.org/23486006/diff/110001/net/disk_cache/simple/si...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/23486006/diff/110001/net/disk_cache/simple/si...
net/disk_cache/simple/simple_backend_impl.cc:320: base::hash_map<uint64,
std::vector<Closure> >::iterator it =
On 2013/09/11 15:41:31, Philippe wrote:
> I wonder if it would make sense to factor out this part from here to line 329?
> The immediate downside I see is the fact that you would have to call the
> factored out function by passing it a callback that would not be lazily
created
> as it is done right now (only when the hash was found in
> |entries_pending_doom_|).
> 
> Wdyt?

I think it's a good idea.

https://codereview.chromium.org/23486006/diff/110001/net/disk_cache/simple/si...
File net/disk_cache/simple/simple_entry_impl.h (right):

https://codereview.chromium.org/23486006/diff/110001/net/disk_cache/simple/si...
net/disk_cache/simple/simple_entry_impl.h:270: bool doomed_;
Hooray!

[gavinp, 2013-09-11 19:20:52]

Thanks everyone! Should go into CQ shortly.

https://codereview.chromium.org/23486006/diff/110001/net/disk_cache/simple/si...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/23486006/diff/110001/net/disk_cache/simple/si...
net/disk_cache/simple/simple_backend_impl.cc:320: base::hash_map<uint64,
std::vector<Closure> >::iterator it =
On 2013/09/11 19:03:21, ttuttle wrote:
> On 2013/09/11 15:41:31, Philippe wrote:
> > I wonder if it would make sense to factor out this part from here to line
329?
> > The immediate downside I see is the fact that you would have to call the
> > factored out function by passing it a callback that would not be lazily
> created
> > as it is done right now (only when the hash was found in
> > |entries_pending_doom_|).
> > 
> > Wdyt?
> 
> I think it's a good idea.

I agree, but doing it is subtle enough that I didn't want to stop right now and
do it; so I've added a TODO(). Creating a redundant closure each time is too
expensive, but the methods that are ultimately called vary, too.

[commit-bot: I haz the power, 2013-09-11 19:22:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/23486006/57002

[commit-bot: I haz the power, 2013-09-12 01:50:50]

Change committed as 222695