Use 64kb read buffer in CRNHttpProtocolHandler on iOS.

Use 64kb instead of 4kb read buffer in CRNHttpProtocolHandler on iOS. 

This reduces number of reads and reallocations if UrlRequest::Read has more data available.

Change QuicTestServer to return simple response without HTTP/2 trailers, so it doesn't break the QuicHttpStream.

BUG=706515
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.mac:ios-simulator-cronet

Review-Url: https://codereview.chromium.org/2776583004
Cr-Commit-Position: refs/heads/master@{#465738}
Committed: https://chromium.googlesource.com/chromium/src/+/054d4e484b45c4c7872eec3459ccb1c5f85a3b1e

[mef, 2017-03-24 22:28:38]

Description was changed from

==========
Cronet experiments with receiving large data using NSURLSession API.

BUG=
==========

to

==========
Cronet experiments with receiving large data using NSURLSession API.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[mef, 2017-03-29 19:59:26]

Description was changed from

==========
Cronet experiments with receiving large data using NSURLSession API.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Cronet experiments with receiving large data using NSURLSession API.

BUG=706515
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[mef, 2017-04-13 20:29:07]

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

[commit-bot: I haz the power, 2017-04-13 20:30:05]

Dry run: 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-04-13 21:10:22]

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

[commit-bot: I haz the power, 2017-04-13 21:10:22]

Dry run: This issue passed the CQ dry run.

[mef, 2017-04-14 18:58:57]

Description was changed from

==========
Cronet experiments with receiving large data using NSURLSession API.

BUG=706515
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Use 64kb instead of 4kb read buffer in CRNHttpProtocolHandler on iOS. 

This reduces number of reads and reallocations if UrlRequest::Read has more data
available.

Change QuicTestServer to return simple response without HTTP/2 trailers, so it
doesn't break the QuicHttpStream.

BUG=706515
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[mef, 2017-04-14 18:58:58]

mef@chromium.org changed reviewers:
+ droger@chromium.org, kapishnikov@chromium.org

[mef, 2017-04-14 19:03:58]

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

[commit-bot: I haz the power, 2017-04-14 19:04:41]

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

[mef, 2017-04-14 19:04:44]

Description was changed from

==========
Use 64kb instead of 4kb read buffer in CRNHttpProtocolHandler on iOS. 

This reduces number of reads and reallocations if UrlRequest::Read has more data
available.

Change QuicTestServer to return simple response without HTTP/2 trailers, so it
doesn't break the QuicHttpStream.

BUG=706515
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Use 64kb instead of 4kb read buffer in CRNHttpProtocolHandler on iOS. 

This reduces number of reads and reallocations if UrlRequest::Read has more data
available.

Change QuicTestServer to return simple response without HTTP/2 trailers, so it
doesn't break the QuicHttpStream.

BUG=706515
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.mac:ios-simulator-cronet
==========

[mef, 2017-04-14 19:05:53]

PTAL. The read buffer size increase gets this synthetic test to perform better
than platform on device and simulator.

[commit-bot: I haz the power, 2017-04-14 19:44:51]

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

[commit-bot: I haz the power, 2017-04-14 19:44:52]

Dry run: This issue passed the CQ dry run.

[kapishnikov, 2017-04-19 16:14:17]

https://codereview.chromium.org/2776583004/diff/80001/components/cronet/ios/t...
File components/cronet/ios/test/cronet_http_test.mm (right):

https://codereview.chromium.org/2776583004/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/cronet_http_test.mm:59: [_responseData dealloc];
Are the test compiled under ARC? If yes, it should be enough to set
_responseData to nil.

https://codereview.chromium.org/2776583004/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/cronet_http_test.mm:164:
dispatch_semaphore_wait([delegate_ semaphore], DISPATCH_TIME_FOREVER);
Why are we changing the timeout to DISPATCH_TIME_FOREVER? I think it would be
better to have some reasonable timeout and check the return value for 0.

https://codereview.chromium.org/2776583004/diff/80001/components/cronet/ios/t...
File components/cronet/ios/test/test_server.cc (right):

https://codereview.chromium.org/2776583004/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/test_server.cc:25:
base::LazyInstance<std::string>::Leaky g_big_data_body =
Why should g_big_data_body be global? Why can't we generate the response locally
at the time of the request? What if there are two concurrent requests with
different request data lengths?

https://codereview.chromium.org/2776583004/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/test_server.cc:52:
DCHECK(base::StringToInt64(base::StringPiece(data_size_str), &data_size));
Two questions here:
1) DCHECK is going to be removed in the release version, so the call to
StringToInt64 will also be removed. I think we should make the test functional
regardless of the debug or release variant.
2) What is the benefit of using DCHECK instead of CHECK in the test code in
general?

[mef, 2017-04-19 17:39:10]

Thanks, PTAL.

https://codereview.chromium.org/2776583004/diff/80001/components/cronet/ios/t...
File components/cronet/ios/test/cronet_http_test.mm (right):

https://codereview.chromium.org/2776583004/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/cronet_http_test.mm:59: [_responseData dealloc];
On 2017/04/19 16:14:16, kapishnikov wrote:
> Are the test compiled under ARC? If yes, it should be enough to set
> _responseData to nil.

Currently they are NOT. IIUIC Chrome does not use ARC, but I'm not sure whether
Cronet or its tests should. Here is a patch of this CL with ARC enabled for
Cronet targets: https://codereview.chromium.org/2829703002

WDYT?

https://codereview.chromium.org/2776583004/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/cronet_http_test.mm:164:
dispatch_semaphore_wait([delegate_ semaphore], DISPATCH_TIME_FOREVER);
On 2017/04/19 16:14:16, kapishnikov wrote:
> Why are we changing the timeout to DISPATCH_TIME_FOREVER? I think it would be
> better to have some reasonable timeout and check the return value for 0.

Done. One second was too short for perf tests. I've changed it to 20 seconds.

https://codereview.chromium.org/2776583004/diff/80001/components/cronet/ios/t...
File components/cronet/ios/test/test_server.cc (right):

https://codereview.chromium.org/2776583004/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/test_server.cc:25:
base::LazyInstance<std::string>::Leaky g_big_data_body =
On 2017/04/19 16:14:16, kapishnikov wrote:
> Why should g_big_data_body be global? Why can't we generate the response
locally
> at the time of the request? What if there are two concurrent requests with
> different request data lengths?
I would like to avoid allocation of big chunks of memory for every response as I
would expect that it could affect results. 
Looking a little deeper it seems like RawHttpResponse should work a little
better for this.

https://codereview.chromium.org/2776583004/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/test_server.cc:52:
DCHECK(base::StringToInt64(base::StringPiece(data_size_str), &data_size));
On 2017/04/19 16:14:17, kapishnikov wrote:
> Two questions here:
> 1) DCHECK is going to be removed in the release version, so the call to
> StringToInt64 will also be removed. I think we should make the test functional
> regardless of the debug or release variant.
FWIW this code didn't affect functionality, just checking that prepared response
body matches the expectation.

> 2) What is the benefit of using DCHECK instead of CHECK in the test code in
> general?
I don't think there is one, just a force of habit.

[kapishnikov, 2017-04-19 18:19:01]

https://codereview.chromium.org/2776583004/diff/80001/components/cronet/ios/t...
File components/cronet/ios/test/cronet_http_test.mm (right):

https://codereview.chromium.org/2776583004/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/cronet_http_test.mm:59: [_responseData dealloc];
On 2017/04/19 17:39:09, mef wrote:
> On 2017/04/19 16:14:16, kapishnikov wrote:
> > Are the test compiled under ARC? If yes, it should be enough to set
> > _responseData to nil.
> 
> Currently they are NOT. IIUIC Chrome does not use ARC, but I'm not sure
whether
> Cronet or its tests should. Here is a patch of this CL with ARC enabled for
> Cronet targets: https://codereview.chromium.org/2829703002
> 
> WDYT?

I think ARC is the way to go.  Let's keep the enabling of ARC as a separate CL.

https://codereview.chromium.org/2776583004/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/cronet_http_test.mm:205: NSTimeInterval elapsed =
[date timeIntervalSinceNow] * -1000.0;
NSTimeInterval should always contain the time interval in seconds (see
https://developer.apple.com/reference/foundation/nstimeinterval). Using it to
hold time in milleseconds could be misleading. I suggest, we convert the time to
milliseconds at the time of logging.

https://codereview.chromium.org/2776583004/diff/80001/components/cronet/ios/t...
File components/cronet/ios/test/test_server.cc (right):

https://codereview.chromium.org/2776583004/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/test_server.cc:25:
base::LazyInstance<std::string>::Leaky g_big_data_body =
On 2017/04/19 17:39:09, mef wrote:
> On 2017/04/19 16:14:16, kapishnikov wrote:
> > Why should g_big_data_body be global? Why can't we generate the response
> locally
> > at the time of the request? What if there are two concurrent requests with
> > different request data lengths?
> I would like to avoid allocation of big chunks of memory for every response as
I
> would expect that it could affect results. 
> Looking a little deeper it seems like RawHttpResponse should work a little
> better for this.

That makes sense.

https://codereview.chromium.org/2776583004/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/test_server.cc:52:
DCHECK(base::StringToInt64(base::StringPiece(data_size_str), &data_size));
On 2017/04/19 17:39:09, mef wrote:
> On 2017/04/19 16:14:17, kapishnikov wrote:
> > Two questions here:
> > 1) DCHECK is going to be removed in the release version, so the call to
> > StringToInt64 will also be removed. I think we should make the test
functional
> > regardless of the debug or release variant.
> FWIW this code didn't affect functionality, just checking that prepared
response
> body matches the expectation.
> 
> > 2) What is the benefit of using DCHECK instead of CHECK in the test code in
> > general?
> I don't think there is one, just a force of habit.
> 

I think the check was a good thing because it could help to detect a situation
when there is another parallel request that requests data with a different size.
What if we keep the code as it was and substitute DCHECK with CHECK?

https://codereview.chromium.org/2776583004/diff/100001/components/cronet/ios/...
File components/cronet/ios/test/cronet_http_test.mm (right):

https://codereview.chromium.org/2776583004/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_http_test.mm:168:
dispatch_semaphore_wait([delegate_ semaphore],
Are we interested in the result of dispatch_semaphore_wait to check that the
wait was successful? The data task may not be completed if the method timed out.
Maybe we should fail with a proper error message in this case.

https://codereview.chromium.org/2776583004/diff/100001/components/cronet/ios/...
File components/cronet/ios/test/test_server.cc (right):

https://codereview.chromium.org/2776583004/diff/100001/components/cronet/ios/...
components/cronet/ios/test/test_server.cc:114: // Response line with headers.
Since g_big_data_body is global, can we add a check here to verify that the
g_big_data_body contains null, 0-length data? This will force the previous
caller to release the data before this method can be called again.

[mef, 2017-04-19 19:04:48]

Thanks, PTAL!

https://codereview.chromium.org/2776583004/diff/80001/components/cronet/ios/t...
File components/cronet/ios/test/cronet_http_test.mm (right):

https://codereview.chromium.org/2776583004/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/cronet_http_test.mm:59: [_responseData dealloc];
On 2017/04/19 18:19:01, kapishnikov wrote:
> On 2017/04/19 17:39:09, mef wrote:
> > On 2017/04/19 16:14:16, kapishnikov wrote:
> > > Are the test compiled under ARC? If yes, it should be enough to set
> > > _responseData to nil.
> > 
> > Currently they are NOT. IIUIC Chrome does not use ARC, but I'm not sure
> whether
> > Cronet or its tests should. Here is a patch of this CL with ARC enabled for
> > Cronet targets: https://codereview.chromium.org/2829703002
> > 
> > WDYT?
> 
> I think ARC is the way to go.  Let's keep the enabling of ARC as a separate
CL.

ack, sgtm

https://codereview.chromium.org/2776583004/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/cronet_http_test.mm:205: NSTimeInterval elapsed =
[date timeIntervalSinceNow] * -1000.0;
On 2017/04/19 18:19:01, kapishnikov wrote:
> NSTimeInterval should always contain the time interval in seconds (see
> https://developer.apple.com/reference/foundation/nstimeinterval). Using it to
> hold time in milleseconds could be misleading. I suggest, we convert the time
to
> milliseconds at the time of logging.

Good point. Done.

https://codereview.chromium.org/2776583004/diff/80001/components/cronet/ios/t...
File components/cronet/ios/test/test_server.cc (right):

https://codereview.chromium.org/2776583004/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/test_server.cc:25:
base::LazyInstance<std::string>::Leaky g_big_data_body =
On 2017/04/19 18:19:01, kapishnikov wrote:
> On 2017/04/19 17:39:09, mef wrote:
> > On 2017/04/19 16:14:16, kapishnikov wrote:
> > > Why should g_big_data_body be global? Why can't we generate the response
> > locally
> > > at the time of the request? What if there are two concurrent requests with
> > > different request data lengths?
> > I would like to avoid allocation of big chunks of memory for every response
as
> I
> > would expect that it could affect results. 
> > Looking a little deeper it seems like RawHttpResponse should work a little
> > better for this.
> 
> That makes sense.

Acknowledged.

https://codereview.chromium.org/2776583004/diff/100001/components/cronet/ios/...
File components/cronet/ios/test/cronet_http_test.mm (right):

https://codereview.chromium.org/2776583004/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_http_test.mm:168:
dispatch_semaphore_wait([delegate_ semaphore],
On 2017/04/19 18:19:01, kapishnikov wrote:
> Are we interested in the result of dispatch_semaphore_wait to check that the
> wait was successful? The data task may not be completed if the method timed
out.
> Maybe we should fail with a proper error message in this case.

Done.

https://codereview.chromium.org/2776583004/diff/100001/components/cronet/ios/...
File components/cronet/ios/test/test_server.cc (right):

https://codereview.chromium.org/2776583004/diff/100001/components/cronet/ios/...
components/cronet/ios/test/test_server.cc:114: // Response line with headers.
On 2017/04/19 18:19:01, kapishnikov wrote:
> Since g_big_data_body is global, can we add a check here to verify that the
> g_big_data_body contains null, 0-length data? This will force the previous
> caller to release the data before this method can be called again.

Done.

[mef, 2017-04-19 19:05:05]

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

[commit-bot: I haz the power, 2017-04-19 19:06:25]

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

[kapishnikov, 2017-04-19 19:12:10]

lgtm

[commit-bot: I haz the power, 2017-04-19 19:57:35]

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

[commit-bot: I haz the power, 2017-04-19 19:57:36]

Dry run: This issue passed the CQ dry run.

[mef, 2017-04-19 20:26:07]

mef@chromium.org changed reviewers:
+ ellyjones@chromium.org
- droger@chromium.org

[mef, 2017-04-19 20:28:54]

Elly, could you OWNER-approve ios/net/crn_http_protocol_handler.mm? 

The small read buffer size causes constant reallocations of NSData in
OnReadCompleted:
https://cs.chromium.org/chromium/src/ios/net/crn_http_protocol_handler.mm?rcl...
if there is more data available.

[Elly Fong-Jones, 2017-04-19 20:30:25]

ios/net lgtm

[mef, 2017-04-19 20:32:10]

Thanks!

[mef, 2017-04-19 20:32:16]

The CQ bit was checked by mef@chromium.org

[commit-bot: I haz the power, 2017-04-19 20:33:09]

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-04-19 20:41:22]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492633936091250,
"parent_rev": "39b3663c37c90a8c6b13f50069eb017e2a1e3931", "commit_rev":
"054d4e484b45c4c7872eec3459ccb1c5f85a3b1e"}

[commit-bot: I haz the power, 2017-04-19 20:41:33]

Description was changed from

==========
Use 64kb instead of 4kb read buffer in CRNHttpProtocolHandler on iOS. 

This reduces number of reads and reallocations if UrlRequest::Read has more data
available.

Change QuicTestServer to return simple response without HTTP/2 trailers, so it
doesn't break the QuicHttpStream.

BUG=706515
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.mac:ios-simulator-cronet
==========

to

==========
Use 64kb instead of 4kb read buffer in CRNHttpProtocolHandler on iOS. 

This reduces number of reads and reallocations if UrlRequest::Read has more data
available.

Change QuicTestServer to return simple response without HTTP/2 trailers, so it
doesn't break the QuicHttpStream.

BUG=706515
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.mac:ios-simulator-cronet

Review-Url: https://codereview.chromium.org/2776583004
Cr-Commit-Position: refs/heads/master@{#465738}
Committed:
https://chromium.googlesource.com/chromium/src/+/054d4e484b45c4c7872eec3459cc...
==========

[commit-bot: I haz the power, 2017-04-19 20:41:34]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/054d4e484b45c4c7872eec3459cc...