Use 64kb read buffer in CRNHttpProtocolHandler on iOS.

components/cronet/ios/test/cronet_http_test.mm

Index: components/cronet/ios/test/cronet_http_test.mm
diff --git a/components/cronet/ios/test/cronet_http_test.mm b/components/cronet/ios/test/cronet_http_test.mm
index e1642feeaf9cbb5a6ad0cff8fdde7d7e7a03d6b1..4dfbafc26717742d8b07e7c4fadf24801de1d413 100644
--- a/components/cronet/ios/test/cronet_http_test.mm
+++ b/components/cronet/ios/test/cronet_http_test.mm
@@ -28,21 +28,19 @@
 // Completion semaphore for this TestDelegate. When the request this delegate is
 // attached to finishes (either successfully or with an error), this delegate
 // signals this semaphore.
-@property(assign, nonatomic) dispatch_semaphore_t semaphore;
-
-// Body of response received by the request this delegate is attached to.
-@property(retain, nonatomic) NSString* responseBody;
+@property(assign, atomic) dispatch_semaphore_t semaphore;
 
 // Error the request this delegate is attached to failed with, if any.
-@property(retain, nonatomic) NSError* error;
+@property(retain, atomic) NSError* error;
 
 @end
 
 @implementation TestDelegate
 @synthesize semaphore = _semaphore;
-@synthesize responseBody = _responseBody;
 @synthesize error = _error;
 
+NSMutableArray<NSData*>* _responseData;
+
 - (id)init {
   if (self = [super init]) {
     _semaphore = dispatch_semaphore_create(0);
@@ -58,10 +56,25 @@
 }
 
 - (void)reset {
-  _responseBody = nil;
+  [_responseData dealloc];

[kapishnikov, 2017/04/19 16:14:16]

Are the test compiled under ARC? If yes, it should be enough to set
_responseData to nil.

[mef, 2017/04/19 17:39:09]

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?

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

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

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

ack, sgtm

+  _responseData = nil;
   _error = nil;
 }
 
+- (NSString*)responseBody {
+  if (_responseData == nil) {
+    return nil;
+  }
+  NSMutableString* body = [NSMutableString string];
+  for (NSData* data in _responseData) {
+    [body appendString:[[NSString alloc] initWithData:data
+                                             encoding:NSUTF8StringEncoding]];
+  }
+  VLOG(3) << "responseBody size:" << [body length]
+          << " chunks:" << [_responseData count];
+  return body;
+}
+
 - (void)URLSession:(NSURLSession*)session
     didBecomeInvalidWithError:(NSError*)error {
 }
@@ -93,13 +106,10 @@
 - (void)URLSession:(NSURLSession*)session
           dataTask:(NSURLSessionDataTask*)dataTask
     didReceiveData:(NSData*)data {
-  NSString* stringData =
-      [[NSString alloc] initWithData:data encoding:NSUTF8StringEncoding];
-  if (_responseBody == nil) {
-    _responseBody = stringData;
-  } else {
-    _responseBody = [_responseBody stringByAppendingString:stringData];
+  if (_responseData == nil) {
+    _responseData = [[NSMutableArray alloc] init];
   }
+  [_responseData addObject:data];
 }
 
 - (void)URLSession:(NSURLSession*)session
@@ -113,9 +123,6 @@
 @end
 
 namespace cronet {
-// base::TimeDelta would normally be ideal for this but it does not support
-// nanosecond resolution.
-static const int64_t ns_in_second = 1000000000LL;
 const char kUserAgent[] = "CronetTest/1.0.0.0";
 
 class HttpTest : public ::testing::Test {
@@ -154,9 +161,7 @@ class HttpTest : public ::testing::Test {
   void StartDataTaskAndWaitForCompletion(NSURLSessionDataTask* task) {
     [delegate_ reset];
     [task resume];
-    int64_t deadline_ns = 1 * ns_in_second;
-    dispatch_semaphore_wait([delegate_ semaphore],
-                            dispatch_time(DISPATCH_TIME_NOW, deadline_ns));
+    dispatch_semaphore_wait([delegate_ semaphore], DISPATCH_TIME_FOREVER);

[kapishnikov, 2017/04/19 16:14:16]

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.

[mef, 2017/04/19 17:39:09]

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

   }
 
   base::scoped_nsobject<NSURLSession> session_;
@@ -164,7 +169,7 @@ class HttpTest : public ::testing::Test {
 };
 
 TEST_F(HttpTest, NSURLSessionReceivesData) {
-  NSURL* url = net::NSURLWithGURL(GURL(grpc_support::kTestServerUrl));
+  NSURL* url = net::NSURLWithGURL(GURL(grpc_support::kTestServerSimpleUrl));
   __block BOOL block_used = NO;
   NSURLSessionDataTask* task = [session_ dataTaskWithURL:url];
   [Cronet setRequestFilterBlock:^(NSURLRequest* request) {
@@ -175,18 +180,47 @@ TEST_F(HttpTest, NSURLSessionReceivesData) {
   StartDataTaskAndWaitForCompletion(task);
   EXPECT_TRUE(block_used);
   EXPECT_EQ(nil, [delegate_ error]);
-  EXPECT_STREQ(grpc_support::kHelloBodyValue,
+  EXPECT_STREQ(grpc_support::kSimpleBodyValue,
                base::SysNSStringToUTF8([delegate_ responseBody]).c_str());
 }
 
+TEST_F(HttpTest, NSURLSessionReceivesBigHttpDataLoop) {
+  int iterations = 50;
+  long size = 10 * 1024 * 1024;
+  LOG(INFO) << "Downloading " << size << " bytes " << iterations << " times.";
+  NSTimeInterval elapsed_avg = 0;
+  NSTimeInterval elapsed_max = 0;
+  NSURL* url = net::NSURLWithGURL(GURL(TestServer::GetBigDataURL(size)));
+  for (int i = 0; i < iterations; ++i) {
+    [delegate_ reset];
+    __block BOOL block_used = NO;
+    NSURLSessionDataTask* task = [session_ dataTaskWithURL:url];
+    [Cronet setRequestFilterBlock:^(NSURLRequest* request) {
+      block_used = YES;
+      EXPECT_EQ([request URL], url);
+      return YES;
+    }];
+    NSDate* date = [NSDate date];
+    StartDataTaskAndWaitForCompletion(task);
+    NSTimeInterval elapsed = [date timeIntervalSinceNow] * -1000.0;

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

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.

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

Good point. Done.

+    elapsed_avg += elapsed;
+    if (elapsed > elapsed_max)
+      elapsed_max = elapsed;
+    EXPECT_TRUE(block_used);
+    EXPECT_EQ(nil, [delegate_ error]);
+  }
+  LOG(INFO) << "Elapsed Average:" << elapsed_avg / iterations
+            << "ms Max:" << elapsed_max << "ms";
+}
+
 TEST_F(HttpTest, GetGlobalMetricsDeltas) {
   NSData* delta1 = [Cronet getGlobalMetricsDeltas];
 
-  NSURL* url = net::NSURLWithGURL(GURL(grpc_support::kTestServerUrl));
+  NSURL* url = net::NSURLWithGURL(GURL(grpc_support::kTestServerSimpleUrl));
   NSURLSessionDataTask* task = [session_ dataTaskWithURL:url];
   StartDataTaskAndWaitForCompletion(task);
   EXPECT_EQ(nil, [delegate_ error]);
-  EXPECT_STREQ(grpc_support::kHelloBodyValue,
+  EXPECT_STREQ(grpc_support::kSimpleBodyValue,
                base::SysNSStringToUTF8([delegate_ responseBody]).c_str());
 
   NSData* delta2 = [Cronet getGlobalMetricsDeltas];