[Mojo-Loading] Split too large data chunk in renderer

content/child/url_response_body_consumer.cc

Index: content/child/url_response_body_consumer.cc
diff --git a/content/child/url_response_body_consumer.cc b/content/child/url_response_body_consumer.cc
index 398ea85b19dd0b8a7abee23d5cd06dd880dfc261..fddd7768a3c7a60127b1a4d436a449e13d25f4bc 100644
--- a/content/child/url_response_body_consumer.cc
+++ b/content/child/url_response_body_consumer.cc
@@ -15,6 +15,8 @@
 
 namespace content {
 
+constexpr uint32_t URLResponseBodyConsumer::kMaxNumConsumedBytesInTask;
+
 class URLResponseBodyConsumer::ReceivedData final
     : public RequestPeer::ReceivedData {
  public:
@@ -98,6 +100,7 @@ void URLResponseBodyConsumer::OnReadable(MojoResult unused) {
     return;
 
   DCHECK(!is_in_on_readable_);
+  uint32_t num_bytes_consumed = 0;
 
   // Protect |this| as RequestPeer::OnReceivedData may call deref.
   scoped_refptr<URLResponseBodyConsumer> protect(this);
@@ -122,6 +125,20 @@ void URLResponseBodyConsumer::OnReadable(MojoResult unused) {
       NotifyCompletionIfAppropriate();
       return;
     }
+    DCHECK_LE(num_bytes_consumed, kMaxNumConsumedBytesInTask);
+    available =
+        std::min(available, kMaxNumConsumedBytesInTask - num_bytes_consumed);
+    if (available == 0) {
+      // We've already consumed many bytes in this task. Defer the remaining
+      // to the next task.
+      result = mojo::EndReadDataRaw(handle_.get(), 0);
+      DCHECK_EQ(result, MOJO_RESULT_OK);
+      task_runner_->PostTask(FROM_HERE,
+                             base::Bind(&URLResponseBodyConsumer::OnReadable,
+                                        AsWeakPtr(), MOJO_RESULT_OK));

[kinuko, 2017/01/20 02:56:28]

Do you have some stats / numbers how would this affect?

[yhirano, 2017/01/20 03:30:50]

No.

Some justifications:
- AsyncResourceHandler has kMaxAllocationSize and this is corresponding to it.
- This (not post-tasking, but splitting a chunk) fixes
NaClBrowserTestPnacl.PnaclErrorHandling
NaClBrowserTestPnaclSubzero.PnaclErrorHandling (see
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...
and
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...).

[kinuko, 2017/01/20 04:57:35]

Thanks, one more question for me to understand... any reason we use different
numbers for kMaxAllocationSize and kMaxNumConsumedBytesInTask?

[kinuko, 2017/01/20 04:58:53]

Also-- do you anticipate we'll have more IO tasks (than traditional IPC cases)
by this or not?  Just want to understand how these dynamics change.

[yhirano, 2017/01/20 05:06:10]

This function runs on the main thread so I think IO tasks are unrelated.

[yhirano, 2017/01/20 05:06:10]

There is no reason. If you are happier with 32k, I can use the value.

[kinuko, 2017/01/20 05:12:29]

I'm just trying to do a due diligence to see if we have any justification about
why we use the number.  Could you have a comment about why the number is chosen
at least?  If we put some constant without any comments other people will have
no insight about why it's chosen and will likely blindly keep following it...
that's what I'm afraid.

[kinuko, 2017/01/20 05:12:29]

Oops, that's right, sorry. What about the # of main thread tasks then?

[yhirano, 2017/01/20 06:04:54]

Added some comments on the header file.

[yhirano, 2017/01/20 06:04:54]

I expect the number of tasks will be greater than that with the current
mojo-loading implementation and less than that with the current IPC
implementation as chunks can be merged up to 64kb with this CL.

+      return;
+    }
+    num_bytes_consumed += available;
     ResourceDispatcher::PendingRequestInfo* request_info =
         resource_dispatcher_->GetPendingRequestInfo(request_id_);
     DCHECK(request_info);