Avoid copying the data string when posting ImportantFileWriter write tasks

When binding the data string to a callback, the existing code copies the data which can be 1MB+ sometimes.
Use scoped_ptr to avoid unnecessary data copy.

BUG=None

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

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

4 files changed, 26 insertions, 22 deletions

diff --git a/base/files/important_file_writer.cc b/base/files/important_file_writer.cc
index 47b0b09..f49790c 100644
--- a/base/files/important_file_writer.cc
+++ b/base/files/important_file_writer.cc
@@ -51,6 +51,12 @@ void LogFailure(const FilePath& path, TempFileFailure failure_code,
                  << " : " << message;
 }
 
+// Helper function to call WriteFileAtomically() with a scoped_ptr<std::string>.
+bool WriteScopedStringToFileAtomically(const FilePath& path,
+                                       scoped_ptr<std::string> data) {
+  return ImportantFileWriter::WriteFileAtomically(path, *data);
+}
+
 }  // namespace
 
 // static
@@ -139,9 +145,9 @@ bool ImportantFileWriter::HasPendingWrite() const {
   return timer_.IsRunning();
 }
 
-void ImportantFileWriter::WriteNow(const std::string& data) {
+void ImportantFileWriter::WriteNow(scoped_ptr<std::string> data) {
   DCHECK(CalledOnValidThread());
-  if (data.length() > static_cast<size_t>(kint32max)) {
+  if (data->length() > static_cast<size_t>(kint32max)) {
     NOTREACHED();
     return;
   }
@@ -149,13 +155,14 @@ void ImportantFileWriter::WriteNow(const std::string& data) {
   if (HasPendingWrite())
     timer_.Stop();
 
-  if (!PostWriteTask(data)) {
+  auto task = Bind(&WriteScopedStringToFileAtomically, path_, Passed(&data));
+  if (!PostWriteTask(task)) {
     // Posting the task to background message loop is not expected
     // to fail, but if it does, avoid losing data and just hit the disk
     // on the current thread.
     NOTREACHED();
 
-    WriteFileAtomically(path_, data);
+    task.Run();
   }
 }
 
@@ -173,9 +180,9 @@ void ImportantFileWriter::ScheduleWrite(DataSerializer* serializer) {
 
 void ImportantFileWriter::DoScheduledWrite() {
   DCHECK(serializer_);
-  std::string data;
-  if (serializer_->SerializeData(&data)) {
-    WriteNow(data);
+  scoped_ptr<std::string> data(new std::string);
+  if (serializer_->SerializeData(data.get())) {
+    WriteNow(data.Pass());
   } else {
     DLOG(WARNING) << "failed to serialize data to be saved in "
                   << path_.value().c_str();
@@ -189,7 +196,7 @@ void ImportantFileWriter::RegisterOnNextSuccessfulWriteCallback(
   on_next_successful_write_ = on_next_successful_write;
 }
 
-bool ImportantFileWriter::PostWriteTask(const std::string& data) {
+bool ImportantFileWriter::PostWriteTask(const Callback<bool()>& task) {
   // TODO(gab): This code could always use PostTaskAndReplyWithResult and let
   // ForwardSuccessfulWrite() no-op if |on_next_successful_write_| is null, but
   // PostTaskAndReply causes memory leaks in tests (crbug.com/371974) and
@@ -199,16 +206,13 @@ bool ImportantFileWriter::PostWriteTask(const std::string& data) {
     return base::PostTaskAndReplyWithResult(
         task_runner_.get(),
         FROM_HERE,
-        MakeCriticalClosure(
-            Bind(&ImportantFileWriter::WriteFileAtomically, path_, data)),
+        MakeCriticalClosure(task),
         Bind(&ImportantFileWriter::ForwardSuccessfulWrite,
              weak_factory_.GetWeakPtr()));
   }
   return task_runner_->PostTask(
       FROM_HERE,
-      MakeCriticalClosure(
-          Bind(IgnoreResult(&ImportantFileWriter::WriteFileAtomically),
-               path_, data)));
+      MakeCriticalClosure(base::Bind(IgnoreResult(task))));
 }
 
 void ImportantFileWriter::ForwardSuccessfulWrite(bool result) {
diff --git a/base/files/important_file_writer.h b/base/files/important_file_writer.h
index 9ead9c1..99f1a7c 100644
--- a/base/files/important_file_writer.h
+++ b/base/files/important_file_writer.h
@@ -78,7 +78,7 @@ class BASE_EXPORT ImportantFileWriter : public NonThreadSafe {
 
   // Save |data| to target filename. Does not block. If there is a pending write
   // scheduled by ScheduleWrite, it is cancelled.
-  void WriteNow(const std::string& data);
+  void WriteNow(scoped_ptr<std::string> data);
 
   // Schedule a save to target filename. Data will be serialized and saved
   // to disk after the commit interval. If another ScheduleWrite is issued
@@ -106,7 +106,7 @@ class BASE_EXPORT ImportantFileWriter : public NonThreadSafe {
 
  private:
   // Helper method for WriteNow().
-  bool PostWriteTask(const std::string& data);
+  bool PostWriteTask(const Callback<bool()>& task);
 
   // If |result| is true and |on_next_successful_write_| is set, invokes
   // |on_successful_write_| and then resets it; no-ops otherwise.
diff --git a/base/files/important_file_writer_unittest.cc b/base/files/important_file_writer_unittest.cc
index 98eaee8..170c2b2 100644
--- a/base/files/important_file_writer_unittest.cc
+++ b/base/files/important_file_writer_unittest.cc
@@ -100,7 +100,7 @@ TEST_F(ImportantFileWriterTest, Basic) {
   ImportantFileWriter writer(file_, MessageLoopProxy::current().get());
   EXPECT_FALSE(PathExists(writer.path()));
   EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
-  writer.WriteNow("foo");
+  writer.WriteNow(make_scoped_ptr(new std::string("foo")));
   RunLoop().RunUntilIdle();
 
   EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
@@ -113,7 +113,7 @@ TEST_F(ImportantFileWriterTest, BasicWithSuccessfulWriteObserver) {
   EXPECT_FALSE(PathExists(writer.path()));
   EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
   successful_write_observer_.ObserveNextSuccessfulWrite(&writer);
-  writer.WriteNow("foo");
+  writer.WriteNow(make_scoped_ptr(new std::string("foo")));
   RunLoop().RunUntilIdle();
 
   // Confirm that the observer is invoked.
@@ -124,7 +124,7 @@ TEST_F(ImportantFileWriterTest, BasicWithSuccessfulWriteObserver) {
   // Confirm that re-installing the observer works for another write.
   EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
   successful_write_observer_.ObserveNextSuccessfulWrite(&writer);
-  writer.WriteNow("bar");
+  writer.WriteNow(make_scoped_ptr(new std::string("bar")));
   RunLoop().RunUntilIdle();
 
   EXPECT_TRUE(successful_write_observer_.GetAndResetObservationState());
@@ -134,7 +134,7 @@ TEST_F(ImportantFileWriterTest, BasicWithSuccessfulWriteObserver) {
   // Confirm that writing again without re-installing the observer doesn't
   // result in a notification.
   EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
-  writer.WriteNow("baz");
+  writer.WriteNow(make_scoped_ptr(new std::string("baz")));
   RunLoop().RunUntilIdle();
 
   EXPECT_FALSE(successful_write_observer_.GetAndResetObservationState());
diff --git a/components/bookmarks/browser/bookmark_storage.cc b/components/bookmarks/browser/bookmark_storage.cc
index daafdbe..2612f9c 100644
--- a/components/bookmarks/browser/bookmark_storage.cc
+++ b/components/bookmarks/browser/bookmark_storage.cc
@@ -205,10 +205,10 @@ bool BookmarkStorage::SaveNow() {
     return false;
   }
 
-  std::string data;
-  if (!SerializeData(&data))
+  scoped_ptr<std::string> data(new std::string);
+  if (!SerializeData(data.get()))
     return false;
-  writer_.WriteNow(data);
+  writer_.WriteNow(data.Pass());
   return true;
 }