**STILLBAKING** Switch SimpleCache to SequencedWorkerPool.

**STILLBAKING** Switch SimpleCache to SequencedWorkerPool.

By using a SequencedWorkerPool for tasks rather than the WorkerPool, flakiness
should be reduced, and the SimpleEntryImpl ownership model is greatly simplified.

The SimpleBackendImpl creates a sequenced task runner for each active entry as it
opens, providing a proxy task runner during the wait for that initialization.

BUG=None

[gavinp, 2013-05-21 12:30:33]

This is very early, but it's passing most tests, and I'd like your opinion of
the general approach.

What do you think of this approach? I think the SimpleEntryImpl is simplified
greatly, and with a bit of work in SimpleBackendImpl we should be flake free on
Doom(), Create() and Close() etc...

[gavinp, 2013-05-21 13:18:00]

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

https://codereview.chromium.org/15563005/diff/9002/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_entry_impl.cc:129: return
OpenEntryInternal(callback, out_entry);
Really this should be rolled in here, but the idea was to make this upload
easier to read I would minimize cutting and pasting of code.

[pasko, 2013-05-21 16:00:01]

Generally looks lovely! Keep on good hacking!

https://codereview.chromium.org/15563005/diff/9002/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/15563005/diff/9002/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_backend_impl.cc:325: class
SimpleBackendImpl::ProxyTaskRunner : public TaskRunner {
A more self-descriptive name would be:
BarrierTaskRunner or AccumulatingTaskRunner

https://codereview.chromium.org/15563005/diff/9002/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_backend_impl.cc:440:
inactive_entry_task_runner_->PostTaskAndReply(
Why do we have to serialize token creation via a worker thread without doing IO?

I thought we'd just create tokens as soon as we need them, not asynchronously.
If we need the lifetime of active entry to be different from the lifetime of
active token, I don't see a big problem in that.

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

https://codereview.chromium.org/15563005/diff/9002/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_entry_impl.cc:129: return
OpenEntryInternal(callback, out_entry);
On 2013/05/21 13:18:00, gavinp wrote:
> Really this should be rolled in here, but the idea was to make this upload
> easier to read I would minimize cutting and pasting of code.

Thanks.

https://codereview.chromium.org/15563005/diff/9002/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_entry_impl.cc:524: MarkAsDoomed();
synchronous_entry_ = NULL?
synchronous_entry_ seems to be the new state, which is luckily a boolean. By the
way, can this be further simplified? (maybe later?)

[gavinp, 2013-05-21 16:55:02]

https://codereview.chromium.org/15563005/diff/9002/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/15563005/diff/9002/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_backend_impl.cc:440:
inactive_entry_task_runner_->PostTaskAndReply(
On 2013/05/21 16:00:01, pasko wrote:
> Why do we have to serialize token creation via a worker thread without doing
IO?
> 
> I thought we'd just create tokens as soon as we need them, not asynchronously.
> If we need the lifetime of active entry to be different from the lifetime of
> active token, I don't see a big problem in that.

What if a mass doom operation is running right now which affects the new
ActiveEntry?

[pasko-google - do not use, 2013-05-21 17:25:47]

https://codereview.chromium.org/15563005/diff/9002/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/15563005/diff/9002/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_backend_impl.cc:440:
inactive_entry_task_runner_->PostTaskAndReply(
On 2013/05/21 16:55:02, gavinp wrote:
> On 2013/05/21 16:00:01, pasko wrote:
> > Why do we have to serialize token creation via a worker thread without doing
> IO?
> > 
> > I thought we'd just create tokens as soon as we need them, not
asynchronously.
> > If we need the lifetime of active entry to be different from the lifetime of
> > active token, I don't see a big problem in that.
> 
> What if a mass doom operation is running right now which affects the new
> ActiveEntry?

something around the lines of SimpleEntryImpl::OpenEntry():

if (backend_->index()->MassDoomInProgress()) {
  backend_->index()->CallWhenMassDoomDone(...);
  return net::ERR_IO_PENDING;
}

yeah, need to distinguish when we created the entry and when took something
already active. But that seems better than posting extra tasks on the fast path.

[gavinp, 2013-05-21 18:23:43]

On 2013/05/21 17:25:47, pasko-google - do not use wrote:
>
https://codereview.chromium.org/15563005/diff/9002/net/disk_cache/simple/simp...
> File net/disk_cache/simple/simple_backend_impl.cc (right):
> 
>
https://codereview.chromium.org/15563005/diff/9002/net/disk_cache/simple/simp...
> net/disk_cache/simple/simple_backend_impl.cc:440:
> inactive_entry_task_runner_->PostTaskAndReply(
> On 2013/05/21 16:55:02, gavinp wrote:
> > On 2013/05/21 16:00:01, pasko wrote:
> > > Why do we have to serialize token creation via a worker thread without
doing
> > IO?
> > > 
> > > I thought we'd just create tokens as soon as we need them, not
> asynchronously.
> > > If we need the lifetime of active entry to be different from the lifetime
of
> > > active token, I don't see a big problem in that.
> > 
> > What if a mass doom operation is running right now which affects the new
> > ActiveEntry?
> 
> something around the lines of SimpleEntryImpl::OpenEntry():
> 
> if (backend_->index()->MassDoomInProgress()) {
>   backend_->index()->CallWhenMassDoomDone(...);
>   return net::ERR_IO_PENDING;
> }
> 
> yeah, need to distinguish when we created the entry and when took something
> already active. But that seems better than posting extra tasks on the fast
path.

Aha, yes, we can definitely make the normal case NOT cross thread for speed
here, but I think it's minor (it's a pretty quick two thread hop). I'll add that
though in my next upload.

[Randy Smith (Not in Mondays), 2013-05-21 21:15:30]

We might want to consider PostNamedSequenceWorkerTask.  I suggest that because
if we use the filename, that will automatically handle the close->create race (I
didn't spot how you were handling that race, but I'm sure it was in there
somewhere).

As noted, I'm wondering if we can get rid of the mass doom concept and save
ourselves some pain around racing.

https://codereview.chromium.org/15563005/diff/9002/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/15563005/diff/9002/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_backend_impl.cc:37: const int kMaxThreads = 50;
Yikes.  There's a real, and reasonably high memory cost for each thread (at
least, on Mac, I see 244MB virtual memory dedicated to the stack for a not-very
loaded (6-8 tabs?) chrome instance.  I hope we don't need anything like this
many to get reasonable parallelism.

https://codereview.chromium.org/15563005/diff/9002/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_backend_impl.cc:325: class
SimpleBackendImpl::ProxyTaskRunner : public TaskRunner {
nit: I think helper classes are supposed to be at the top of the file.

https://codereview.chromium.org/15563005/diff/9002/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_backend_impl.cc:440:
inactive_entry_task_runner_->PostTaskAndReply(
On 2013/05/21 16:55:02, gavinp wrote:
> On 2013/05/21 16:00:01, pasko wrote:
> > Why do we have to serialize token creation via a worker thread without doing
> IO?
> > 
> > I thought we'd just create tokens as soon as we need them, not
asynchronously.
> > If we need the lifetime of active entry to be different from the lifetime of
> > active token, I don't see a big problem in that.
> 
> What if a mass doom operation is running right now which affects the new
> ActiveEntry?

What are the performance costs of doing a mass doom operation piecemeal through
the entries?  It seems like we keep running into tricky race conditions around
trying to do a mass doom as an end run.

https://codereview.chromium.org/15563005/diff/9002/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_backend_impl.h (right):

https://codereview.chromium.org/15563005/diff/9002/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_backend_impl.h:105: typedef std::map<uint64,
ActiveEntry> EntryMap;
My understanding of how stl containers work is that the tings in them can be
copied at any time, which I think would play merry h**l with the weak pointers
generated for them.  My guess is that you should keep pointers to ActiveEntrys
instead and explicitly delete them when you're done.

[gavinp, 2013-06-03 13:54:01]

Given the timeline, I'm going to say this shouldn't land until after branch
(june 24th).

[pasko, 2013-07-22 18:25:31]

should this one be closed now?

[gavinp, 2013-07-22 18:28:49]

On 2013/07/22 18:25:31, pasko wrote:
> should this one be closed now?

Yessir