FileAPI: Split recursive remove into multiple async tasks

FileAPI: Split recursive remove into multiple async tasks

While this change makes each recursive delete run slower, concurrent jobs can run without waiting for the entire recursive job to finish.

Recursive remove end-to-end (200 files, 120 dirs, 4-level tree, total 100MB):
Before this change: Ave: 72.94 msec, Stddev:  3.571 msec
After this change:  Ave:129.54 msec, Stddev: 25.368 msec

It takes 1.76 times slower than before by average (we can probably do more optimization as this implementation doesn't care much about it), while another concurrent file task called immediately after the delete could finish in 1-5 msec (while back then it needed to wait for 70-80 msec).

BUG=146215
TEST=content_unittests:.*File.*,content_browsertests:FileSystemLayoutTests.OpRemove

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

[kinuko, 2013-01-21 04:07:35]

PTL thanks,

[tzik, 2013-01-23 03:44:41]

lgtm

https://codereview.chromium.org/12018017/diff/13010/webkit/fileapi/local_file...
File webkit/fileapi/local_file_system_operation.h (right):

https://codereview.chromium.org/12018017/diff/13010/webkit/fileapi/local_file...
webkit/fileapi/local_file_system_operation.h:101: 
nit: add empty comment line?

[ericu, 2013-01-23 17:59:46]

I haven't checked this in detail, but I wanted to comment on the fact that
unless I've missed something, this is now the first filesystem operation that
allows reentrancy.  Up until this point, any filesystem operation could assume
that it was the only operation running at a given time.  This change seems safe
to me, as it's always been the case that any operation could fail, and it's
still the case that only one thread will access e.g. the sandbox's database at a
time.  However, from now on we will have to keep our eyes open for an entire new
class of bad interactions as sub-operations are interleaved.

[kinuko, 2013-01-24 01:12:10]

On 2013/01/23 17:59:46, ericu wrote:
> I haven't checked this in detail, but I wanted to comment on the fact that
> unless I've missed something, this is now the first filesystem operation that
> allows reentrancy. Up until this point, any filesystem operation could assume
> that it was the only operation running at a given time.  This change seems
safe
> to me, as it's always been the case that any operation could fail, and it's
> still the case that only one thread will access e.g. the sandbox's database at
a
> time.  However, from now on we will have to keep our eyes open for an entire
new
> class of bad interactions as sub-operations are interleaved.

It's not really re-entering, it's just delegating the task to other operations,
and it's not the first one either as we already do something similar in
WriterDelegate to get metadata for the file.  So creating a new operation inside
an operation is not very special but I agree that we should keep our eyes open.

[ericu, 2013-01-24 01:16:29]

Sorry, reentrancy was the wrong word.  It allows overlapping with
other operations.  So now a recursive copy can get interleaved with a
recursive move in the same origin, with undefined results.  I believe
it's safe--we'll just fail partway through if we get confused, leaving
some subset of the operations completed--but we'll have to keep our
eyes open.

Previously, even if you did the move from a worker and the copy from
the document, each would complete and only see a self-consistent
filesystem, as they were serialized.

[kinuko, 2013-01-24 01:33:54]

That's true, they are no longer atomic. Apps will have a higher possibility to
have nasty errors like the ones we tend to get when we perform 'rm -rf' or 'mv
<huge dir in a drive> <another drive>' while doing something in the directory.

[kinuko, 2013-01-24 02:44:45]

https://codereview.chromium.org/12018017/diff/13010/webkit/fileapi/local_file...
File webkit/fileapi/local_file_system_operation.h (right):

https://codereview.chromium.org/12018017/diff/13010/webkit/fileapi/local_file...
webkit/fileapi/local_file_system_operation.h:101: 
On 2013/01/23 03:44:41, tzik wrote:
> nit: add empty comment line?

Done.

[commit-bot: I haz the power, 2013-01-24 04:21:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/12018017/14017

[commit-bot: I haz the power, 2013-01-24 07:20:47]

Change committed as 178542