New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 706515 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 3
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 3
Type: Bug

Blocked on:
issue 709144



Sign in to add a comment

[Cronet] Performance of NSURLSessionDataTask using Cronet is below platform for large response.

Project Member Reported by mef@chromium.org, Mar 29 2017

Issue description

In some scenarios performance of NSURLSessionDataTask that uses Cronet stack is below platform.

One of these scenarios is large size of response (>1mb).

I've done some experiments with test http server running locally on the iPad Mini2 (MF544LL/A) (https://codereview.chromium.org/2776583004) and got the following numbers for 10mb downloads:

Platform: Elapsed Average:93.586791 ms Max:112.994969 ms
Cronet: Elapsed Average:141.780800 ms Max:173.210979 ms

This issue will be used to track the work. 
 

Comment 1 by mef@chromium.org, Mar 29 2017

Cc: lilyhoughton@chromium.org kapishnikov@chromium.org
Owner: mef@chromium.org
Status: Assigned (was: Untriaged)

Comment 2 by mef@chromium.org, Mar 29 2017

Cc: droger@chromium.org sdefresne@chromium.org
droger@ - by any chance do you remember why crn_http_protocol_handler.mm uses 4k read buffer? 

I've tried to increase it to 32k - 128k and got promising results (in synthetic test). 
"Platform: Elapsed Average:93.586791 ms Max:112.994969 ms
Cronet: Elapsed Average:141.780800 ms Max:173.210979 ms"

@Misha, Is this happening after we disable the HTTP cache? seems so
Cc: ianswett@chromium.org

Comment 5 by mef@chromium.org, Apr 11 2017

Blockedon: 709144
It appears that QUIC buffer size could also affect upload speeds.

Comment 6 by mef@chromium.org, Apr 13 2017

Ping, I've run same test with different cache configurations, and disk cache affects performance of this particular test very negatively, especially when buffer size is 4k.


by disabling cache, Cronet will be better in this particular test case? do we still need to increase the buffer size to see the gain? if yes, what's a good buffer size to use?

Comment 8 by mef@chromium.org, Apr 14 2017

Yes, by disabling cache I see Cronet getting better results in my test.
Increasing buffer size from 4kb to 64-128kb shows best results in this test.

Comment 9 by droger@chromium.org, Apr 14 2017

I think 4k was used because that was what desktop was using at that time. It seemed to work well, and we definitely didn't try to tweak the value.
Project Member

Comment 10 by bugdroid1@chromium.org, Apr 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/054d4e484b45c4c7872eec3459ccb1c5f85a3b1e

commit 054d4e484b45c4c7872eec3459ccb1c5f85a3b1e
Author: mef <mef@chromium.org>
Date: Wed Apr 19 20:41:19 2017

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}

[modify] https://crrev.com/054d4e484b45c4c7872eec3459ccb1c5f85a3b1e/components/cronet/ios/test/cronet_http_test.mm
[modify] https://crrev.com/054d4e484b45c4c7872eec3459ccb1c5f85a3b1e/components/cronet/ios/test/start_cronet.mm
[modify] https://crrev.com/054d4e484b45c4c7872eec3459ccb1c5f85a3b1e/components/cronet/ios/test/test_server.cc
[modify] https://crrev.com/054d4e484b45c4c7872eec3459ccb1c5f85a3b1e/components/cronet/ios/test/test_server.h
[modify] https://crrev.com/054d4e484b45c4c7872eec3459ccb1c5f85a3b1e/components/grpc_support/test/quic_test_server.cc
[modify] https://crrev.com/054d4e484b45c4c7872eec3459ccb1c5f85a3b1e/components/grpc_support/test/quic_test_server.h
[modify] https://crrev.com/054d4e484b45c4c7872eec3459ccb1c5f85a3b1e/ios/net/crn_http_protocol_handler.mm

Misha, do you android cronet url request can benefit for a larger buffer size as well?

Right now we are using the default buffer size of 32KB, same as the one in the android example.

private static final int READ_BUFFER_SIZE = 32 * 1024;
mBuffer = ByteBuffer.allocateDirect(READ_BUFFER_SIZE);

Comment 12 by mef@chromium.org, May 10 2017

Zhihua, that's a great question. 

I would expect that it depends on network, device and app usage. 

Cronet UrlRequest on Android lets app to provide its own buffer, and you may want to experiment to see if bigger or smaller buffer is better.
Project Member

Comment 13 by bugdroid1@chromium.org, Oct 31 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5872bbebe979ebb59886bfc20f91683ff313a594

commit 5872bbebe979ebb59886bfc20f91683ff313a594
Author: Lily Houghton <lilyhoughton@chromium.org>
Date: Tue Oct 31 14:56:59 2017

[Cronet] Basic performance testing.

This adds some very basic performance testing to the Cronet test suite.

Bug:  706515 
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I64f4a24baf8fe92d2d9d865752cbff4bcb2df12c
Reviewed-on: https://chromium-review.googlesource.com/649991
Reviewed-by: Misha Efimov <mef@chromium.org>
Reviewed-by: Andrei Kapishnikov <kapishnikov@chromium.org>
Commit-Queue: Andrei Kapishnikov <kapishnikov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512830}
[modify] https://crrev.com/5872bbebe979ebb59886bfc20f91683ff313a594/components/cronet/ios/test/BUILD.gn
[modify] https://crrev.com/5872bbebe979ebb59886bfc20f91683ff313a594/components/cronet/ios/test/cronet_http_test.mm
[add] https://crrev.com/5872bbebe979ebb59886bfc20f91683ff313a594/components/cronet/ios/test/cronet_performance_test.mm
[modify] https://crrev.com/5872bbebe979ebb59886bfc20f91683ff313a594/components/cronet/ios/test/cronet_test_base.h
[modify] https://crrev.com/5872bbebe979ebb59886bfc20f91683ff313a594/components/cronet/ios/test/cronet_test_base.mm

Project Member

Comment 14 by bugdroid1@chromium.org, Nov 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0c815cfa7f855fb4494def7dfe7e1dec97941f9c

commit 0c815cfa7f855fb4494def7dfe7e1dec97941f9c
Author: kapishnikov <kapishnikov@chromium.org>
Date: Thu Nov 30 18:01:01 2017

Use "<=" instead of "<" in perf test

It is not entirely correct to use "<" to compare number of failed requests.
E.g. it will always fail if the total number of requests is '1'.
If the total number of requests is 3 then no failures are tolerated while
one should be allowed.

BUG= 706515 

Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I77469141d5b789ca793a1cf8c3a79a9de5206a84
Reviewed-on: https://chromium-review.googlesource.com/800973
Commit-Queue: Andrei Kapishnikov <kapishnikov@chromium.org>
Reviewed-by: Andrei Kapishnikov <kapishnikov@chromium.org>
Reviewed-by: Misha Efimov <mef@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520598}
[modify] https://crrev.com/0c815cfa7f855fb4494def7dfe7e1dec97941f9c/components/cronet/ios/test/cronet_performance_test.mm

Project Member

Comment 15 by bugdroid1@chromium.org, Nov 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d7ac949be778b9726fe2e3c5cdbcd31f64586b7c

commit d7ac949be778b9726fe2e3c5cdbcd31f64586b7c
Author: kapishnikov <kapishnikov@chromium.org>
Date: Thu Nov 30 18:08:57 2017

Fix printing of downloaded bytes in Cronet perf tests.

[delegate_ totalBytesReceivedPerTask][task] returns a NSNumber
that should be converted to integer for proper printing.

BUG= 706515 

Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I57c7ecea727a13ef5a79a205dee1c134dfe93eea
Reviewed-on: https://chromium-review.googlesource.com/797952
Reviewed-by: Misha Efimov <mef@chromium.org>
Reviewed-by: Andrei Kapishnikov <kapishnikov@chromium.org>
Commit-Queue: Andrei Kapishnikov <kapishnikov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520600}
[modify] https://crrev.com/d7ac949be778b9726fe2e3c5cdbcd31f64586b7c/components/cronet/ios/test/cronet_performance_test.mm

Project Member

Comment 16 by bugdroid1@chromium.org, Nov 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7fe493a1b29becbc069b6a7b8e58cf51b23b27d8

commit 7fe493a1b29becbc069b6a7b8e58cf51b23b27d8
Author: kapishnikov <kapishnikov@chromium.org>
Date: Thu Nov 30 18:23:54 2017

Use Int64_t for PerfResult.total_bytes_downloaded to avoid overflow.

BUG= 706515 

Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I4e5a3fb4b42c8367abb0e46d8f5383950442e064
Reviewed-on: https://chromium-review.googlesource.com/801050
Reviewed-by: Misha Efimov <mef@chromium.org>
Reviewed-by: Andrei Kapishnikov <kapishnikov@chromium.org>
Commit-Queue: Andrei Kapishnikov <kapishnikov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520609}
[modify] https://crrev.com/7fe493a1b29becbc069b6a7b8e58cf51b23b27d8/components/cronet/ios/test/cronet_performance_test.mm

Project Member

Comment 17 by bugdroid1@chromium.org, Jan 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6d5eb3c9d43994e50643107e90733fe36afeb1d8

commit 6d5eb3c9d43994e50643107e90733fe36afeb1d8
Author: kapishnikov <kapishnikov@chromium.org>
Date: Thu Jan 18 17:58:33 2018

Fix the iOS performance test to correctly detect failed requests

Without the change the test considers a requests failed only
if it timed out. It doesn't take into consideration the response
staus code.

BUG= 706515 

Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Ibe938c0486547cbe64ce2545fa016b71c28ff1d3
Reviewed-on: https://chromium-review.googlesource.com/862665
Commit-Queue: Andrei Kapishnikov <kapishnikov@chromium.org>
Reviewed-by: Misha Efimov <mef@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530195}
[modify] https://crrev.com/6d5eb3c9d43994e50643107e90733fe36afeb1d8/components/cronet/ios/test/cronet_performance_test.mm
[modify] https://crrev.com/6d5eb3c9d43994e50643107e90733fe36afeb1d8/components/cronet/ios/test/cronet_pkp_test.mm
[modify] https://crrev.com/6d5eb3c9d43994e50643107e90733fe36afeb1d8/components/cronet/ios/test/cronet_test_base.h
[modify] https://crrev.com/6d5eb3c9d43994e50643107e90733fe36afeb1d8/components/cronet/ios/test/cronet_test_base.mm

Project Member

Comment 18 by bugdroid1@chromium.org, Jan 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9d9af179d07ce51b2facc781fdeabc5e7b6863f7

commit 9d9af179d07ce51b2facc781fdeabc5e7b6863f7
Author: kapishnikov <kapishnikov@chromium.org>
Date: Thu Jan 18 18:05:15 2018

Fixed a bug in throughput calculation of iOS perf tests

That was the reason why throughput on iOS was shown 10x lower than
on Android.

BUG= 706515 

Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: If8e583c547e3f196431279e3ceb29efd113971e8
Reviewed-on: https://chromium-review.googlesource.com/864742
Reviewed-by: Misha Efimov <mef@chromium.org>
Commit-Queue: Andrei Kapishnikov <kapishnikov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530200}
[modify] https://crrev.com/9d9af179d07ce51b2facc781fdeabc5e7b6863f7/components/cronet/ios/test/cronet_performance_test.mm

Status: Fixed (was: Assigned)
Performance is now on par.

Sign in to add a comment