Eliminate copying of data in iOS protocol handler to improve performance

ios/net/crn_http_protocol_handler.mm

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "ios/net/crn_http_protocol_handler.h"
 
 #include <stdint.h>
 
 #include <memory>
 #include <utility>
@@ skipping 32 matching lines @@
 #if !defined(__has_feature) || !__has_feature(objc_arc)
 #error "This file requires ARC support."
 #endif
 
 namespace net {
 class HttpProtocolHandlerCore;
 }
 
 namespace {
 
-// Size of the buffer used to read the net::URLRequest.
-const int kIOBufferSize = 64 * 1024;
+// Minimum size of the buffer used to read the net::URLRequest.
+const int kIOBufferMinSize = 64 * 1024;
 
-// The maximum size of NSData that can be passed to the client 'didReceiveData'
-// callback. This value must always be greater or equal to |kIOBufferSize|.
-const int kClientMaxBufferSize = 4 * kIOBufferSize;
+// Maximum size of the buffer used to read the net::URLRequest.
+const int kIOBufferMaxSize = 16 * kIOBufferMinSize;  // 1MB
 
 // Global instance of the HTTPProtocolHandlerDelegate.
 net::HTTPProtocolHandlerDelegate* g_protocol_handler_delegate = nullptr;
 
 // Empty callback.
 void DoNothing(bool flag) {}
 
 }  // namespace
 
 // Bridge class to forward NSStream events to the HttpProtocolHandlerCore.
@@ skipping 113 matching lines @@
   // credentials if |auth_ok| is true.
   void CompleteAuthentication(bool auth_ok,
                               const base::string16& username,
                               const base::string16& password);
   void StripPostSpecificHeaders(NSMutableURLRequest* request);
   void CancelAfterSSLError();
   void ContinueAfterSSLError();
   void SSLErrorCallback(bool carryOn);
   void HostStateCallback(bool carryOn);
   void StartReading();
+  void AllocateReadBuffer(int last_read_data_size);
 
   base::ThreadChecker thread_checker_;
 
   // The NSURLProtocol client.
   id<CRNNetworkClientProtocol> client_;
-  scoped_refptr<IOBuffer> buffer_;
+  std::unique_ptr<char[]> read_buffer_;
+  int read_buffer_size_;
+  scoped_refptr<WrappedIOBuffer> read_buffer_wrapper_;
   base::scoped_nsobject<NSMutableURLRequest> request_;
   // Stream delegate to read the HTTPBodyStream.
   base::scoped_nsobject<CRWHTTPStreamDelegate> stream_delegate_;
   // Vector of readers used to accumulate a POST data stream.
   std::vector<std::unique_ptr<UploadElementReader>> post_data_readers_;
 
   // This cannot be a scoped pointer because it must be deleted on the IO
   // thread.
   URLRequest* net_request_;
 
   base::WeakPtr<RequestTracker> tracker_;
 
   DISALLOW_COPY_AND_ASSIGN(HttpProtocolHandlerCore);
 };
 
 HttpProtocolHandlerCore::HttpProtocolHandlerCore(NSURLRequest* request)
     : client_(nil),
-      buffer_(new IOBuffer(kIOBufferSize)),
+      read_buffer_size_(kIOBufferMinSize),
+      read_buffer_wrapper_(nullptr),
       net_request_(nullptr) {
   // The request will be accessed from another thread. It is safer to make a
   // copy to avoid conflicts.
   // The copy is mutable, because that request will be given to the client in
   // case of a redirect, but with a different URL. The URL must be created
   // from the absoluteString of the original URL, because mutableCopy only
   // shallowly copies the request, and just retains the non-threadsafe NSURL.
   thread_checker_.DetachFromThread();
   request_.reset([request mutableCopy]);
+  read_buffer_.reset(new char[kIOBufferMinSize]);
   [request_ setURL:[NSURL URLWithString:[[request URL] absoluteString]]];
 }
 
 void HttpProtocolHandlerCore::HandleStreamEvent(NSStream* stream,
                                                 NSStreamEvent event) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(stream_delegate_);
   switch (event) {
     case NSStreamEventErrorOccurred:
       DLOG(ERROR)
           << "Failed to read POST data: "
           << base::SysNSStringToUTF8([[stream streamError] description]);
       StopListeningStream(stream);
       StopRequestWithError(NSURLErrorUnknown, ERR_UNEXPECTED);
       break;
     case NSStreamEventEndEncountered:
       StopListeningStream(stream);
       if (!post_data_readers_.empty()) {
         // NOTE: This call will result in |post_data_readers_| being cleared,
         // which is the desired behavior.
         net_request_->set_upload(base::MakeUnique<ElementsUploadDataStream>(
             std::move(post_data_readers_), 0));
         DCHECK(post_data_readers_.empty());
       }
       net_request_->Start();
       if (tracker_)
         tracker_->StartRequest(net_request_);
       break;
     case NSStreamEventHasBytesAvailable: {
-      NSUInteger length;
+      NSInteger length;
       DCHECK([stream isKindOfClass:[NSInputStream class]]);
-      length = [(NSInputStream*)stream read:(unsigned char*)buffer_->data()
-                                  maxLength:kIOBufferSize];
+      // TODO(crbug.com/738025): Dynamically change the size of the read buffer
+      // to improve the read (POST) performance, see AllocateReadBuffer() &
+      // avoid unnecessary data copy.
+      length = [(NSInputStream*)stream

[sdefresne, 2017/06/29 16:32:53]

drive-by: style guide bans casts using C style
(https://google.github.io/styleguide/cppguide.html#Casting). Since you are
already updating this file, would you mind converting them to C++ cast, and
ObjCCastStrict (from base/mac/foundation_util.h)?

      length = [base::max::ObjCCastStrict<NSInputStream>(stream)
               read:static_cast<unsigned char*>(buffer_->data())
          maxLength:read_buffer_size_];

Note that you can remove the DCHECK if you use ObjCCastStrict as it uses
-isKindOfClass:.

[kapishnikov, 2017/06/29 19:40:11]

Done. It is good to know. Thanks!

+          read:(unsigned char*)(read_buffer_.get())maxLength:read_buffer_size_];

[mef, 2017/06/29 16:18:38]

nit: weird formatting. Maybe try git cl fomat ios/net?

[kapishnikov, 2017/06/29 19:40:11]

That was the output of "git cl format". Agree that it is strange.

       if (length) {

[mef, 2017/06/29 16:18:38]

Length could be -1 if error occurred. We should handle that.

[kapishnikov, 2017/06/29 19:40:11]

Nice catch. Fixed it.

-        std::vector<char> owned_data(buffer_->data(), buffer_->data() + length);
+        std::vector<char> owned_data(read_buffer_.get(),
+                                     read_buffer_.get() + length);
         post_data_readers_.push_back(
             base::MakeUnique<UploadOwnedBytesElementReader>(&owned_data));
       }
       break;
     }
     case NSStreamEventNone:
     case NSStreamEventOpenCompleted:
     case NSStreamEventHasSpaceAvailable:
       break;
     default:
@@ skipping 221 matching lines @@
     tracker_->CaptureHeaders(net_request_);
     long long expectedContentLength = [response expectedContentLength];
     if (expectedContentLength > 0)
       tracker_->CaptureExpectedLength(net_request_, expectedContentLength);
   }
 
   // Don't call any function on the response from now on, as the client may be
   // using it and the object is not re-entrant.
   [client_ didReceiveResponse:response];
 
-  int bytes_read = net_request_->Read(buffer_.get(), kIOBufferSize);
+  read_buffer_wrapper_ = new WrappedIOBuffer((const char*)(read_buffer_.get()));
+  int bytes_read =
+      net_request_->Read(read_buffer_wrapper_.get(), read_buffer_size_);
   if (bytes_read == net::ERR_IO_PENDING)
     return;
 
   if (bytes_read >= 0) {
     OnReadCompleted(net_request_, bytes_read);
   } else {
     int error = bytes_read;
     StopRequestWithError(IOSErrorCode(error), error);
   }
 }
 
 void HttpProtocolHandlerCore::OnReadCompleted(URLRequest* request,
                                               int bytes_read) {
   DCHECK_NE(net::ERR_IO_PENDING, bytes_read);
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (net_request_ == nullptr)
     return;
 
   DCHECK_EQ(net_request_, request);
-  DCHECK_GE(kClientMaxBufferSize, kIOBufferSize);
 
-  // Read all we can from the socket and put it into data.
-  // TODO(droger): It may be possible to avoid some of the copies (using
-  // WrappedIOBuffer for example).
+  // Read data from the socket until no bytes left to read.
   uint64_t total_bytes_read = 0;
   while (bytes_read > 0) {
-    base::scoped_nsobject<NSMutableData> data(
-        [[NSMutableData alloc] initWithCapacity:bytes_read]);
-    // |bytes_read| should always be less or equal to |kClientMaxBufferSize|.
-    // This is ensured by the fact that the max read buffer size (i.e.
-    // |kIOBufferSize|) is always smaller or equal to |kClientMaxBufferSize|.
-    while (bytes_read > 0 &&
-           [data length] + bytes_read <= kClientMaxBufferSize) {
-      total_bytes_read += bytes_read;
-      [data appendBytes:buffer_->data() length:bytes_read];
-      bytes_read = request->Read(buffer_.get(), kIOBufferSize);
-    }
+    total_bytes_read += bytes_read;
+    // The NSData will take the ownership of |buffer_|.
+    NSData* data =
+        [NSData dataWithBytesNoCopy:read_buffer_.release() length:bytes_read];
+    // If the data is not encoded in UTF8, the NSString is nil.
+    DVLOG(3) << "To client:" << std::endl
+             << base::SysNSStringToUTF8([[NSString alloc]
+                    initWithData:data
+                        encoding:NSUTF8StringEncoding]);
+    // Pass the read data to the client.
+    [client_ didLoadData:data];
 
-    if ([data length] > 0) {
-      // If the data is not encoded in UTF8, the NSString is nil.
-      DVLOG(3) << "To client:" << std::endl
-               << base::SysNSStringToUTF8([[NSString alloc]
-                      initWithData:data
-                          encoding:NSUTF8StringEncoding]);
-      [client_ didLoadData:data];
-    }
+    // Allocate a new buffer and continue reading from the socket.
+    AllocateReadBuffer(bytes_read);
+    read_buffer_wrapper_ =
+        new WrappedIOBuffer((const char*)(read_buffer_.get()));
+    bytes_read = request->Read(read_buffer_wrapper_.get(), read_buffer_size_);
   }
 
   if (tracker_)
     tracker_->CaptureReceivedBytes(request, total_bytes_read);
 
   if (bytes_read == net::OK) {
     // If there is nothing more to read.
     StopNetRequest();
     [client_ didFinishLoading];
   } else if (bytes_read != net::ERR_IO_PENDING) {
     // If there was an error (not canceled).
     int error = bytes_read;
     StopRequestWithError(IOSErrorCode(error), error);
   }
 }
 
+void HttpProtocolHandlerCore::AllocateReadBuffer(int last_read_data_size) {
+  if (last_read_data_size == read_buffer_size_) {
+    // If the whole buffer was filled with data then increase the buffer size
+    // for the next read but don't exceed |kIOBufferMaxSize|.
+    read_buffer_size_ = std::min(read_buffer_size_ * 2, kIOBufferMaxSize);
+  } else if (read_buffer_size_ / 2 >= last_read_data_size) {
+    // If only a half or less of the buffer was filled with data then reduce
+    // the buffer size for the next read but not make it smaller than
+    // |kIOBufferMinSize|.
+    read_buffer_size_ = std::max(read_buffer_size_ / 2, kIOBufferMinSize);
+  }
+  read_buffer_.reset(new char[read_buffer_size_]);
+}
+
 HttpProtocolHandlerCore::~HttpProtocolHandlerCore() {
   DCHECK(thread_checker_.CalledOnValidThread());
   [client_ cancelAuthRequest];
   DCHECK(!net_request_);
   DCHECK(!stream_delegate_);
 }
 
 // static
 void HttpProtocolHandlerCore::Destruct(const HttpProtocolHandlerCore* x) {
   scoped_refptr<base::SingleThreadTaskRunner> task_runner =
@@ skipping 389 matching lines @@
                  base::Bind(&net::HttpProtocolHandlerCore::Cancel, _core));
   [_protocolProxy invalidate];
 }
 
 - (void)stopLoading {
   [self cancelRequest];
   _protocolProxy.reset();
 }
 
 @end