Run recursive file operations sequentially.

Parallel copy can have huge performance drop in some cases. (See issue 578923 for the detail)
For safety and simplicity, this CL makes the recursive file operations sequential.

BUG=578923
TEST=manually confirm the performance of copying a directory to a SD card.

Review URL: https://codereview.chromium.org/1619733004

Cr-Commit-Position: refs/heads/master@{#371467}

2 files changed, 8 insertions, 30 deletions

diff --git a/storage/browser/fileapi/recursive_operation_delegate.cc b/storage/browser/fileapi/recursive_operation_delegate.cc
index a1e13fe..415953c 100644
--- a/storage/browser/fileapi/recursive_operation_delegate.cc
+++ b/storage/browser/fileapi/recursive_operation_delegate.cc
@@ -14,15 +14,9 @@
 
 namespace storage {
 
-namespace {
-// Don't start too many inflight operations.
-const int kMaxInflightOperations = 5;
-}
-
 RecursiveOperationDelegate::RecursiveOperationDelegate(
     FileSystemContext* file_system_context)
     : file_system_context_(file_system_context),
-      inflight_operations_(0),
       canceled_(false),
       error_behavior_(FileSystemOperation::ERROR_BEHAVIOR_ABORT),
       failed_some_operations_(false) {
@@ -42,7 +36,6 @@ void RecursiveOperationDelegate::StartRecursiveOperation(
     const StatusCallback& callback) {
   DCHECK(pending_directory_stack_.empty());
   DCHECK(pending_files_.empty());
-  DCHECK_EQ(0, inflight_operations_);
 
   error_behavior_ = error_behavior;
   callback_ = callback;
@@ -51,7 +44,6 @@ void RecursiveOperationDelegate::StartRecursiveOperation(
 }
 
 void RecursiveOperationDelegate::TryProcessFile(const FileSystemURL& root) {
-  ++inflight_operations_;
   ProcessFile(root, base::Bind(&RecursiveOperationDelegate::DidTryProcessFile,
                                AsWeakPtr(), root));
 }
@@ -68,9 +60,7 @@ void RecursiveOperationDelegate::DidTryProcessFile(
     base::File::Error error) {
   DCHECK(pending_directory_stack_.empty());
   DCHECK(pending_files_.empty());
-  DCHECK_EQ(1, inflight_operations_);
 
-  --inflight_operations_;
   if (canceled_ || error != base::File::FILE_ERROR_NOT_A_FILE) {
     Done(error);
     return;
@@ -85,11 +75,9 @@ void RecursiveOperationDelegate::ProcessNextDirectory() {
   DCHECK(pending_files_.empty());
   DCHECK(!pending_directory_stack_.empty());
   DCHECK(!pending_directory_stack_.top().empty());
-  DCHECK_EQ(0, inflight_operations_);
 
   const FileSystemURL& url = pending_directory_stack_.top().front();
 
-  ++inflight_operations_;
   ProcessDirectory(
       url,
       base::Bind(
@@ -101,9 +89,7 @@ void RecursiveOperationDelegate::DidProcessDirectory(
   DCHECK(pending_files_.empty());
   DCHECK(!pending_directory_stack_.empty());
   DCHECK(!pending_directory_stack_.top().empty());
-  DCHECK_EQ(1, inflight_operations_);
 
-  --inflight_operations_;
   if (canceled_ || error != base::File::FILE_OK) {
     Done(error);
     return;
@@ -123,7 +109,6 @@ void RecursiveOperationDelegate::DidReadDirectory(
     const FileEntryList& entries,
     bool has_more) {
   DCHECK(!pending_directory_stack_.empty());
-  DCHECK_EQ(0, inflight_operations_);
 
   if (canceled_ || error != base::File::FILE_OK) {
     Done(error);
@@ -151,7 +136,7 @@ void RecursiveOperationDelegate::DidReadDirectory(
 void RecursiveOperationDelegate::ProcessPendingFiles() {
   DCHECK(!pending_directory_stack_.empty());
 
-  if ((pending_files_.empty() || canceled_) && inflight_operations_ == 0) {
+  if (pending_files_.empty() || canceled_) {
     ProcessSubDirectory();
     return;
   }
@@ -160,12 +145,10 @@ void RecursiveOperationDelegate::ProcessPendingFiles() {
   if (canceled_)
     return;
 
-  // Run ProcessFile in parallel (upto kMaxInflightOperations).
+  // Run ProcessFile.
   scoped_refptr<base::SingleThreadTaskRunner> current_task_runner =
       base::ThreadTaskRunnerHandle::Get();
-  while (!pending_files_.empty() &&
-         inflight_operations_ < kMaxInflightOperations) {
-    ++inflight_operations_;
+  if (!pending_files_.empty()) {
     current_task_runner->PostTask(
         FROM_HERE,
         base::Bind(&RecursiveOperationDelegate::ProcessFile, AsWeakPtr(),
@@ -178,8 +161,6 @@ void RecursiveOperationDelegate::ProcessPendingFiles() {
 
 void RecursiveOperationDelegate::DidProcessFile(const FileSystemURL& url,
                                                 base::File::Error error) {
-  --inflight_operations_;
-
   if (error != base::File::FILE_OK) {
     if (error_behavior_ == FileSystemOperation::ERROR_BEHAVIOR_ABORT) {
       // If an error occurs, invoke Done immediately (even if there remain
@@ -198,7 +179,6 @@ void RecursiveOperationDelegate::DidProcessFile(const FileSystemURL& url,
 void RecursiveOperationDelegate::ProcessSubDirectory() {
   DCHECK(pending_files_.empty());
   DCHECK(!pending_directory_stack_.empty());
-  DCHECK_EQ(0, inflight_operations_);
 
   if (canceled_) {
     Done(base::File::FILE_ERROR_ABORT);
@@ -220,7 +200,6 @@ void RecursiveOperationDelegate::ProcessSubDirectory() {
   }
 
   DCHECK(!pending_directory_stack_.top().empty());
-  ++inflight_operations_;
   PostProcessDirectory(
       pending_directory_stack_.top().front(),
       base::Bind(&RecursiveOperationDelegate::DidPostProcessDirectory,
@@ -232,9 +211,7 @@ void RecursiveOperationDelegate::DidPostProcessDirectory(
   DCHECK(pending_files_.empty());
   DCHECK(!pending_directory_stack_.empty());
   DCHECK(!pending_directory_stack_.top().empty());
-  DCHECK_EQ(1, inflight_operations_);
 
-  --inflight_operations_;
   pending_directory_stack_.top().pop();
   if (canceled_ || error != base::File::FILE_OK) {
     Done(error);
diff --git a/storage/browser/fileapi/recursive_operation_delegate.h b/storage/browser/fileapi/recursive_operation_delegate.h
index 0a3efbf..dc4362e 100644
--- a/storage/browser/fileapi/recursive_operation_delegate.h
+++ b/storage/browser/fileapi/recursive_operation_delegate.h
@@ -74,7 +74,7 @@ class STORAGE_EXPORT RecursiveOperationDelegate
   // ProcessDirectory is called first for the directory.
   // Then the directory contents are read (to obtain its sub directories and
   // files in it).
-  // ProcessFile is called for found files. This may run in parallel.
+  // ProcessFile is called for found files.
   // The same step is recursively applied to each subdirectory.
   // After all files and subdirectories in a directory are processed,
   // PostProcessDirectory is called for the directory.
@@ -91,11 +91,13 @@ class STORAGE_EXPORT RecursiveOperationDelegate
   // Then traverse order is:
   // ProcessFile(a_dir) (This should return File::FILE_NOT_A_FILE).
   // ProcessDirectory(a_dir).
-  // ProcessFile(b3_file), ProcessFile(b4_file). (in parallel).
+  // ProcessFile(b3_file).
+  // ProcessFile(b4_file).
   // ProcessDirectory(b1_dir).
   // ProcessFile(c2_file)
   // ProcessDirectory(c1_dir).
-  // ProcessFile(d1_file), ProcessFile(d2_file). (in parallel).
+  // ProcessFile(d1_file).
+  // ProcessFile(d2_file).
   // PostProcessDirectory(c1_dir)
   // PostProcessDirectory(b1_dir).
   // ProcessDirectory(b2_dir)
@@ -146,7 +148,6 @@ class STORAGE_EXPORT RecursiveOperationDelegate
   std::stack<FileSystemURL> pending_directories_;
   std::stack<std::queue<FileSystemURL> > pending_directory_stack_;
   std::queue<FileSystemURL> pending_files_;
-  int inflight_operations_;
   bool canceled_;
   ErrorBehavior error_behavior_;
   bool failed_some_operations_;