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

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

With this CL, URLResponseBodyConsumer splits a too large data chunk to smaller
chunks. Also, it stops sending chunks and defers the remaining jobs to
subsequent tasks if it has already sent many bytes to the client in the task.

The main motivation is performance (thread janks), but splitting a large chunk
is needed also because some existing clients (e.g., NaCl compiler) cannot
handle large chunks.

BUG=603396
Review-Url: https://codereview.chromium.org/2644053002
Cr-Commit-Position: refs/heads/master@{#445002}
Committed: https://chromium.googlesource.com/chromium/src/+/b51a6f0bc428a9bcc23c681d32588fcbbe22924c

[yhirano, 2017-01-19 08:05:07]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 08:05:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2017-01-19 08:07:29]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 08:07:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2017-01-19 08:10:09]

yhirano@chromium.org changed reviewers:
+ tzik@chromium.org

[commit-bot: I haz the power, 2017-01-19 09:20:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 09:20:30]

Dry run: This issue passed the CQ dry run.

[yhirano, 2017-01-19 10:38:35]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 10:38:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2017-01-19 11:06:42]

lgtm

[commit-bot: I haz the power, 2017-01-19 11:28:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 11:29:00]

Dry run: This issue passed the CQ dry run.

[yhirano, 2017-01-19 15:38:16]

yhirano@chromium.org changed reviewers:
+ jam@chromium.org

[yhirano, 2017-01-19 15:38:16]

+jam@ for OWNER review.

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

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

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

https://codereview.chromium.org/2644053002/diff/40001/content/child/url_respo...
File content/child/url_response_body_consumer.cc (right):

https://codereview.chromium.org/2644053002/diff/40001/content/child/url_respo...
content/child/url_response_body_consumer.cc:138: AsWeakPtr(), MOJO_RESULT_OK));
Do you have some stats / numbers how would this affect?

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

https://codereview.chromium.org/2644053002/diff/40001/content/child/url_respo...
File content/child/url_response_body_consumer.cc (right):

https://codereview.chromium.org/2644053002/diff/40001/content/child/url_respo...
content/child/url_response_body_consumer.cc:138: AsWeakPtr(), MOJO_RESULT_OK));
On 2017/01/20 02:56:28, kinuko wrote:
> Do you have some stats / numbers how would this affect?

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]

https://codereview.chromium.org/2644053002/diff/40001/content/child/url_respo...
File content/child/url_response_body_consumer.cc (right):

https://codereview.chromium.org/2644053002/diff/40001/content/child/url_respo...
content/child/url_response_body_consumer.cc:138: AsWeakPtr(), MOJO_RESULT_OK));
On 2017/01/20 03:30:50, yhirano wrote:
> On 2017/01/20 02:56:28, kinuko wrote:
> > Do you have some stats / numbers how would this affect?
> 
> 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_...).

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]

https://codereview.chromium.org/2644053002/diff/40001/content/child/url_respo...
File content/child/url_response_body_consumer.cc (right):

https://codereview.chromium.org/2644053002/diff/40001/content/child/url_respo...
content/child/url_response_body_consumer.cc:138: AsWeakPtr(), MOJO_RESULT_OK));
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]

https://codereview.chromium.org/2644053002/diff/40001/content/child/url_respo...
File content/child/url_response_body_consumer.cc (right):

https://codereview.chromium.org/2644053002/diff/40001/content/child/url_respo...
content/child/url_response_body_consumer.cc:138: AsWeakPtr(), MOJO_RESULT_OK));
On 2017/01/20 04:57:35, kinuko wrote:
> On 2017/01/20 03:30:50, yhirano wrote:
> > On 2017/01/20 02:56:28, kinuko wrote:
> > > Do you have some stats / numbers how would this affect?
> > 
> > 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_...).
> 
> Thanks, one more question for me to understand... any reason we use different
> numbers for kMaxAllocationSize and kMaxNumConsumedBytesInTask?

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

https://codereview.chromium.org/2644053002/diff/40001/content/child/url_respo...
content/child/url_response_body_consumer.cc:138: AsWeakPtr(), MOJO_RESULT_OK));
On 2017/01/20 04:58:53, kinuko wrote:
> 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.

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

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

https://codereview.chromium.org/2644053002/diff/40001/content/child/url_respo...
File content/child/url_response_body_consumer.cc (right):

https://codereview.chromium.org/2644053002/diff/40001/content/child/url_respo...
content/child/url_response_body_consumer.cc:138: AsWeakPtr(), MOJO_RESULT_OK));
On 2017/01/20 05:06:10, yhirano wrote:
> On 2017/01/20 04:58:53, kinuko wrote:
> > 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.
> 
> This function runs on the main thread so I think IO tasks are unrelated.

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

https://codereview.chromium.org/2644053002/diff/40001/content/child/url_respo...
content/child/url_response_body_consumer.cc:138: AsWeakPtr(), MOJO_RESULT_OK));
On 2017/01/20 05:06:10, yhirano wrote:
> On 2017/01/20 04:57:35, kinuko wrote:
> > On 2017/01/20 03:30:50, yhirano wrote:
> > > On 2017/01/20 02:56:28, kinuko wrote:
> > > > Do you have some stats / numbers how would this affect?
> > > 
> > > 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_...).
> > 
> > Thanks, one more question for me to understand... any reason we use
different
> > numbers for kMaxAllocationSize and kMaxNumConsumedBytesInTask?
> 
> There is no reason. If you are happier with 32k, I can use the value.

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.

[yhirano, 2017-01-20 05:54:07]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 05:54:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2017-01-20 06:04:55]

https://codereview.chromium.org/2644053002/diff/40001/content/child/url_respo...
File content/child/url_response_body_consumer.cc (right):

https://codereview.chromium.org/2644053002/diff/40001/content/child/url_respo...
content/child/url_response_body_consumer.cc:138: AsWeakPtr(), MOJO_RESULT_OK));
On 2017/01/20 05:12:29, kinuko wrote:
> On 2017/01/20 05:06:10, yhirano wrote:
> > On 2017/01/20 04:57:35, kinuko wrote:
> > > On 2017/01/20 03:30:50, yhirano wrote:
> > > > On 2017/01/20 02:56:28, kinuko wrote:
> > > > > Do you have some stats / numbers how would this affect?
> > > > 
> > > > 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_...).
> > > 
> > > Thanks, one more question for me to understand... any reason we use
> different
> > > numbers for kMaxAllocationSize and kMaxNumConsumedBytesInTask?
> > 
> > There is no reason. If you are happier with 32k, I can use the value.
> 
> 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.

Added some comments on the header file.

https://codereview.chromium.org/2644053002/diff/40001/content/child/url_respo...
content/child/url_response_body_consumer.cc:138: AsWeakPtr(), MOJO_RESULT_OK));
On 2017/01/20 05:12:29, kinuko wrote:
> On 2017/01/20 05:06:10, yhirano wrote:
> > On 2017/01/20 04:58:53, kinuko wrote:
> > > 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.
> > 
> > This function runs on the main thread so I think IO tasks are unrelated.
> 
> Oops, that's right, sorry. What about the # of main thread tasks then?

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.

[kinuko, 2017-01-20 06:07:42]

Thanks!! lgtm

On 2017/01/20 06:04:55, yhirano wrote:
>
https://codereview.chromium.org/2644053002/diff/40001/content/child/url_respo...
> File content/child/url_response_body_consumer.cc (right):
> 
>
https://codereview.chromium.org/2644053002/diff/40001/content/child/url_respo...
> content/child/url_response_body_consumer.cc:138: AsWeakPtr(),
MOJO_RESULT_OK));
> On 2017/01/20 05:12:29, kinuko wrote:
> > On 2017/01/20 05:06:10, yhirano wrote:
> > > On 2017/01/20 04:57:35, kinuko wrote:
> > > > On 2017/01/20 03:30:50, yhirano wrote:
> > > > > On 2017/01/20 02:56:28, kinuko wrote:
> > > > > > Do you have some stats / numbers how would this affect?
> > > > > 
> > > > > 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_...).
> > > > 
> > > > Thanks, one more question for me to understand... any reason we use
> > different
> > > > numbers for kMaxAllocationSize and kMaxNumConsumedBytesInTask?
> > > 
> > > There is no reason. If you are happier with 32k, I can use the value.
> > 
> > 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.
> 
> Added some comments on the header file.
> 
>
https://codereview.chromium.org/2644053002/diff/40001/content/child/url_respo...
> content/child/url_response_body_consumer.cc:138: AsWeakPtr(),
MOJO_RESULT_OK));
> On 2017/01/20 05:12:29, kinuko wrote:
> > On 2017/01/20 05:06:10, yhirano wrote:
> > > On 2017/01/20 04:58:53, kinuko wrote:
> > > > 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.
> > > 
> > > This function runs on the main thread so I think IO tasks are unrelated.
> > 
> > Oops, that's right, sorry. What about the # of main thread tasks then?
> 
> 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.

Gotcha, makes sense.

[yhirano, 2017-01-20 06:13:08]

The CQ bit was unchecked by yhirano@chromium.org

[yhirano, 2017-01-20 06:13:09]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2017-01-20 06:13:09]

The patchset sent to the CQ was uploaded after l-g-t-m from tzik@chromium.org
Link to the patchset: https://codereview.chromium.org/2644053002/#ps60001
(title: "fix")

[commit-bot: I haz the power, 2017-01-20 06:13:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 06:41:28]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484892789412150,
"parent_rev": "6625f12670a758be05fd8cb88ad3e90b6a9bcfd5", "commit_rev":
"b51a6f0bc428a9bcc23c681d32588fcbbe22924c"}

[commit-bot: I haz the power, 2017-01-20 06:42:05]

Description was changed from

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

With this CL, URLResponseBodyConsumer splits a too large data chunk to smaller
chunks. Also, it stops sending chunks and defers the remaining jobs to
subsequent tasks if it has already sent many bytes to the client in the task.

The main motivation is performance (thread janks), but splitting a large chunk
is needed also because some existing clients (e.g., NaCl compiler) cannot
handle large chunks.

BUG=603396
==========

to

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

With this CL, URLResponseBodyConsumer splits a too large data chunk to smaller
chunks. Also, it stops sending chunks and defers the remaining jobs to
subsequent tasks if it has already sent many bytes to the client in the task.

The main motivation is performance (thread janks), but splitting a large chunk
is needed also because some existing clients (e.g., NaCl compiler) cannot
handle large chunks.

BUG=603396

Review-Url: https://codereview.chromium.org/2644053002
Cr-Commit-Position: refs/heads/master@{#445002}
Committed:
https://chromium.googlesource.com/chromium/src/+/b51a6f0bc428a9bcc23c681d3258...
==========

[commit-bot: I haz the power, 2017-01-20 06:42:07]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b51a6f0bc428a9bcc23c681d3258...