[BlobAsync] Faster shortcuttin, make renderer controller leaky & alive.

content/child/blob_storage/blob_transport_controller.cc

Index: content/child/blob_storage/blob_transport_controller.cc
diff --git a/content/child/blob_storage/blob_transport_controller.cc b/content/child/blob_storage/blob_transport_controller.cc
index c2b639ad921d66879b031117191d1fa01bad0db0..3c8373ec94342e48fe3b6cfa221d0cb49c4e828f 100644
--- a/content/child/blob_storage/blob_transport_controller.cc
+++ b/content/child/blob_storage/blob_transport_controller.cc
@@ -27,6 +27,7 @@ using storage::BlobItemBytesRequest;
 using storage::BlobItemBytesResponse;
 using storage::IPCBlobItemRequestStrategy;
 using storage::DataElement;
+using storage::kBlobStorageIPCThresholdBytes;
 
 namespace content {
 
@@ -36,8 +37,7 @@ using ConsolidatedItem = BlobConsolidation::ConsolidatedItem;
 using ReadStatus = BlobConsolidation::ReadStatus;
 
 namespace {
-const size_t kLargeThresholdBytes = 250 * 1024;
-static base::LazyInstance<BlobTransportController> g_controller =
+static base::LazyInstance<BlobTransportController>::Leaky g_controller =
     LAZY_INSTANCE_INITIALIZER;
 
 // This keeps the process alive while blobs are being transferred.
@@ -54,23 +54,40 @@ BlobTransportController* BlobTransportController::GetInstance() {
   return g_controller.Pointer();
 }
 
-BlobTransportController::~BlobTransportController() {}
-
+// static
 void BlobTransportController::InitiateBlobTransfer(
     const std::string& uuid,
     scoped_ptr<BlobConsolidation> consolidation,
-    IPC::Sender* sender,
+    scoped_refptr<ThreadSafeSender> sender,
+    const scoped_refptr<base::SingleThreadTaskRunner>& io_runner,
     scoped_refptr<base::SingleThreadTaskRunner> main_runner) {
+  if (main_runner->BelongsToCurrentThread()) {
+    IncChildProcessRefCount();

[michaeln, 2016/04/05 00:18:04]

having to do this immediately makes sense

+  } else {
+    main_runner->PostTask(FROM_HERE, base::Bind(&IncChildProcessRefCount));
+  }
+
+  // If we fit in IPC, then shortcut our process by sending the descriptions
+  // right away. We schedule our task on the IO thread first as an extra
+  // precaution that we will store the consolidation object before we get any
+  // response from the browser.
   BlobConsolidation* consolidation_ptr = consolidation.get();
-  if (blob_storage_.empty()) {
-    main_thread_runner_ = std::move(main_runner);
-    main_thread_runner_->PostTask(FROM_HERE,
-                                  base::Bind(&IncChildProcessRefCount));
+  IPC::Sender* sender_ptr = sender.get();
+  bool presend_descriptions =
+      consolidation->total_memory() <= kBlobStorageIPCThresholdBytes;
+  io_runner->PostTask(
+      FROM_HERE,
+      base::Bind(&BlobTransportController::InitiateBlobTransferOnIOThread,
+                 base::Unretained(BlobTransportController::GetInstance()), uuid,
+                 base::Passed(std::move(consolidation)),
+                 base::Passed(std::move(sender)), presend_descriptions,
+                 base::Passed(std::move(main_runner))));
+  if (presend_descriptions) {

[michaeln, 2016/04/05 00:18:04]

Are you sure the presending is necessary? If it's necessary for smaller blobs,
does that mean larger blobs are still bugged?

Oh, I see, this is not about correctness, this is to negate a perf regression.
Do you know if it actually does negate the perf regression?

Hmmm, I think if ThreadSafeSender were smart enough to put the message on the
wire immediately when invoked from the IO thread instead of posting an
intermediary task to do so, this presending would not make a difference. But I
don't think ThreadSafeSender/SyncMessageFilter has those smarts, and giving it
those smarts could have other consequences as a result changing the other
message ordering???

[dmurph, 2016/04/05 00:32:04]

I don't know if it does, but I strongly suspect this was the reason. Can I leave
this in there (it's a valid optimization nonetheless), and then make a new patch
for syncmessagefilter?

+    std::vector<storage::DataElement> descriptions;
+    BlobTransportController::GetDescriptions(
+        consolidation_ptr, kBlobStorageIPCThresholdBytes, &descriptions);
+    sender_ptr->Send(new BlobStorageMsg_StartBuildingBlob(uuid, descriptions));
   }
-  blob_storage_[uuid] = std::move(consolidation);
-  std::vector<storage::DataElement> descriptions;
-  GetDescriptions(consolidation_ptr, kLargeThresholdBytes, &descriptions);
-  sender->Send(new BlobStorageMsg_StartBuildingBlob(uuid, descriptions));
 }
 
 void BlobTransportController::OnMemoryRequest(
@@ -115,16 +132,7 @@ void BlobTransportController::OnDone(const std::string& uuid) {
   ReleaseBlobConsolidation(uuid);
 }
 
-void BlobTransportController::ClearForTesting() {
-  if (!blob_storage_.empty() && main_thread_runner_) {
-    main_thread_runner_->PostTask(FROM_HERE,
-                                  base::Bind(&DecChildProcessRefCount));
-  }
-  blob_storage_.clear();
-}
-
-BlobTransportController::BlobTransportController() {}
-
+// static
 void BlobTransportController::GetDescriptions(
     BlobConsolidation* consolidation,
     size_t max_data_population,
@@ -178,6 +186,37 @@ void BlobTransportController::GetDescriptions(
   }
 }
 
+BlobTransportController::BlobTransportController() {}
+
+BlobTransportController::~BlobTransportController() {}
+
+void BlobTransportController::ClearForTesting() {
+  if (!blob_storage_.empty() && main_thread_runner_) {
+    main_thread_runner_->PostTask(FROM_HERE,
+                                  base::Bind(&DecChildProcessRefCount));
+  }
+  blob_storage_.clear();
+}
+
+void BlobTransportController::InitiateBlobTransferOnIOThread(
+    const std::string& uuid,
+    scoped_ptr<BlobConsolidation> consolidation,
+    scoped_refptr<ThreadSafeSender> sender,
+    bool sent_descriptions,
+    scoped_refptr<base::SingleThreadTaskRunner> main_runner) {
+  if (!main_thread_runner_.get()) {
+    main_thread_runner_ = std::move(main_runner);
+  }
+  BlobConsolidation* consolidation_ptr = consolidation.get();
+  blob_storage_[uuid] = std::move(consolidation);
+  if (!sent_descriptions) {
+    std::vector<storage::DataElement> descriptions;
+    GetDescriptions(consolidation_ptr, kBlobStorageIPCThresholdBytes,
+                    &descriptions);
+    sender->Send(new BlobStorageMsg_StartBuildingBlob(uuid, descriptions));
+  }
+}
+
 BlobTransportController::ResponsesStatus BlobTransportController::GetResponses(
     const std::string& uuid,
     const std::vector<BlobItemBytesRequest>& requests,
@@ -255,12 +294,9 @@ BlobTransportController::ResponsesStatus BlobTransportController::GetResponses(
 
 void BlobTransportController::ReleaseBlobConsolidation(
     const std::string& uuid) {
-  // If we erased something and we're now empty, release the child process
-  // ref count and deref the main thread runner.
-  if (blob_storage_.erase(uuid) && blob_storage_.empty()) {
+  if (blob_storage_.erase(uuid)) {
     main_thread_runner_->PostTask(FROM_HERE,
                                   base::Bind(&DecChildProcessRefCount));
-    main_thread_runner_ = nullptr;
   }
 }