Disk Cache: Replace the worker pool with a sequenced worker pool (posix).

Disk Cache: Replace the worker pool with a sequenced worker pool (posix).

On posix systems the cache implements asynchronous IO with a worker pool.
This CL replaces the deprecated WorkerPool with SequencedWorkerPool, and
removes the InFlightIO controller that keeps track of all pending operations.

Now there's no way to wait for operations to complete on production code, but
the cache switched a while ago to only wait on unit tests.

BUG=251774, 268646
TEST=net_unittests
R=gavinp@chromium.org

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

[gavinp, 2013-09-09 21:18:16]

Whoops, just saw I hadn't sent these out.

https://codereview.chromium.org/23752005/diff/1/net/disk_cache/backend_unitte...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/23752005/diff/1/net/disk_cache/backend_unitte...
net/disk_cache/backend_unittest.cc:65: // This function requires the use of a
single thread for the cache.
I think grammar is confusing me a bit here. Does this mean "This function
requires that the cache is running on a single thread" ?

As well, could we be asserting on the mode the cache is running in?

https://codereview.chromium.org/23752005/diff/1/net/disk_cache/file_posix.cc
File net/disk_cache/file_posix.cc (right):

https://codereview.chromium.org/23752005/diff/1/net/disk_cache/file_posix.cc#...
net/disk_cache/file_posix.cc:68: bool File::Read(void* buffer, size_t
buffer_len, size_t offset) {
The references to init_ and platform_file_ from these methods sort of scare me,
since they look like they could race. Reading the code it looks safe, since the
platform_file_ is set with init and then left, and then only ever cleared in the
destructor.

What about making these methods static, or putting them in an anonymous
namespace. Pass in the platform_file explicitly, and then the DCHECK can compare
against kInvalidPlatformFileValue, which should be equivalent to checking init?

Of course, this means that the File can't destruct while the operation is
pending (since the file would close before it's used). But that's already
guarded by the use of PostTaskAndReply. So I dunno.

So I guess it's OK as it is, but I just wanted to share that idea in case you
think it's clearer/safer for future coders?

https://codereview.chromium.org/23752005/diff/1/net/disk_cache/file_posix.cc#...
net/disk_cache/file_posix.cc:125: if (buffer_len >
static_cast<size_t>(kint32max) ||
I'd have written this as std::numeric_limits<int32>::max(). But kint32max is in
basictypes. I don't know if either of them should be preferable.

[rvargas (doing something else), 2013-09-09 22:26:57]

https://codereview.chromium.org/23752005/diff/1/net/disk_cache/backend_unitte...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/23752005/diff/1/net/disk_cache/backend_unitte...
net/disk_cache/backend_unittest.cc:65: // This function requires the use of a
single thread for the cache.
On 2013/09/09 21:18:16, gavinp wrote:
> I think grammar is confusing me a bit here. Does this mean "This function
> requires that the cache is running on a single thread" ?
> 
> As well, could we be asserting on the mode the cache is running in?

Done.

https://codereview.chromium.org/23752005/diff/1/net/disk_cache/file_posix.cc
File net/disk_cache/file_posix.cc (right):

https://codereview.chromium.org/23752005/diff/1/net/disk_cache/file_posix.cc#...
net/disk_cache/file_posix.cc:68: bool File::Read(void* buffer, size_t
buffer_len, size_t offset) {
On 2013/09/09 21:18:16, gavinp wrote:
> The references to init_ and platform_file_ from these methods sort of scare
me,
> since they look like they could race. Reading the code it looks safe, since
the
> platform_file_ is set with init and then left, and then only ever cleared in
the
> destructor.
> 
> What about making these methods static, or putting them in an anonymous
> namespace. Pass in the platform_file explicitly, and then the DCHECK can
compare
> against kInvalidPlatformFileValue, which should be equivalent to checking
init?
> 
> Of course, this means that the File can't destruct while the operation is
> pending (since the file would close before it's used). But that's already
> guarded by the use of PostTaskAndReply. So I dunno.
> 
> So I guess it's OK as it is, but I just wanted to share that idea in case you
> think it's clearer/safer for future coders?

I don't think init_ is a problem as it is guarding file open which should be
single thread.

As for the use of platform_file_, I don't really see static methods helping
readability. Object use semantics are not precluding 'races' in the sense of
interactions between multiple operations that the caller wishes to perform
simultaneously, but the object itself should protect about conventional races of
access of its members. Neither one of those properties are affected by jumping
through static methods (I think).

Please correct me if I'm wrong, but the OS should handle simultaneous access to
the same fd from multiple threads without any issues.

https://codereview.chromium.org/23752005/diff/1/net/disk_cache/file_posix.cc#...
net/disk_cache/file_posix.cc:125: if (buffer_len >
static_cast<size_t>(kint32max) ||
On 2013/09/09 21:18:16, gavinp wrote:
> I'd have written this as std::numeric_limits<int32>::max(). But kint32max is
in
> basictypes. I don't know if either of them should be preferable.

Given that both are valid today I have a strong preference for the shorter one.

[gavinp, 2013-09-10 17:50:15]

Moving the helper into the test class was a good change.

LGTM.

https://codereview.chromium.org/23752005/diff/1/net/disk_cache/file_posix.cc
File net/disk_cache/file_posix.cc (right):

https://codereview.chromium.org/23752005/diff/1/net/disk_cache/file_posix.cc#...
net/disk_cache/file_posix.cc:68: bool File::Read(void* buffer, size_t
buffer_len, size_t offset) {
On 2013/09/09 22:26:57, rvargas wrote:
> On 2013/09/09 21:18:16, gavinp wrote:
> > The references to init_ and platform_file_ from these methods sort of scare
> me,
> > since they look like they could race. Reading the code it looks safe, since
> the
> > platform_file_ is set with init and then left, and then only ever cleared in
> the
> > destructor.
> > 
> > What about making these methods static, or putting them in an anonymous
> > namespace. Pass in the platform_file explicitly, and then the DCHECK can
> compare
> > against kInvalidPlatformFileValue, which should be equivalent to checking
> init?
> > 
> > Of course, this means that the File can't destruct while the operation is
> > pending (since the file would close before it's used). But that's already
> > guarded by the use of PostTaskAndReply. So I dunno.
> > 
> > So I guess it's OK as it is, but I just wanted to share that idea in case
you
> > think it's clearer/safer for future coders?
> 
> I don't think init_ is a problem as it is guarding file open which should be
> single thread.

I agree.

> 
> As for the use of platform_file_, I don't really see static methods helping
> readability. Object use semantics are not precluding 'races' in the sense of
> interactions between multiple operations that the caller wishes to perform
> simultaneously, but the object itself should protect about conventional races
of
> access of its members. Neither one of those properties are affected by jumping
> through static methods (I think).

You're right; I sometimes find the pattern of never, ever accessing members of a
class from another thread can make readability better; when you have that
invariant, it's easier to then read through the accesses on the one thread that
does do it, and figure out if there's a conventional race there. However, in the
case of this class, the use was safe and guarded well enough by well defined
barriers that I think it's fine without my style nit.

> Please correct me if I'm wrong, but the OS should handle simultaneous access
to
> the same fd from multiple threads without any issues.

The OS should be totally fine on that. Things get tricky when the file is
getting closed, but this code carefully guards against this by closing in a
destructor that runs on the cache thread, and all running worker pools hold a
reference, so it's perfect.

https://codereview.chromium.org/23752005/diff/1/net/disk_cache/file_posix.cc#...
net/disk_cache/file_posix.cc:125: if (buffer_len >
static_cast<size_t>(kint32max) ||
On 2013/09/09 22:26:57, rvargas wrote:
> On 2013/09/09 21:18:16, gavinp wrote:
> > I'd have written this as std::numeric_limits<int32>::max(). But kint32max is
> in
> > basictypes. I don't know if either of them should be preferable.
> 
> Given that both are valid today I have a strong preference for the shorter
one.

SGTM. I would probably use numeric_limits<>(), but I have a hard time saying
that's anything other than preference.

[commit-bot: I haz the power, 2013-09-10 20:40:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rvargas@chromium.org/23752005/29001

[commit-bot: I haz the power, 2013-09-10 23:59:42]

Change committed as 222408