[Cronet] Coalesce small buffers into single QUIC packet in GRPC on iOS.

components/cronet/ios/cronet_bidirectional_stream.h

 // Copyright 2016 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.
 
 #ifndef COMPONENTS_CRONET_IOS_CRONET_BIDIRECTIONAL_STREAM_H_
 #define COMPONENTS_CRONET_IOS_CRONET_BIDIRECTIONAL_STREAM_H_
 
 #include <memory>
+#include <vector>
 
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/synchronization/lock.h"
 #include "net/http/bidirectional_stream.h"
 
 namespace net {
 class HttpRequestHeaders;
 class WrappedIOBuffer;
 }  // namespace net
@@ skipping 28 matching lines @@
     virtual void OnSucceeded() = 0;
 
     virtual void OnFailed(int error) = 0;
 
     virtual void OnCanceled() = 0;
   };
 
   CronetBidirectionalStream(CronetEnvironment* environment, Delegate* delegate);
   ~CronetBidirectionalStream() override;
 
+  // Disables automatic flushing of each buffer passed to WriteData().
+  void disable_auto_flush(bool disable_auto_flush) {

[kapishnikov, 2016/06/13 21:21:57]

I remember we discussed to get rid of disable_auto_flush() method and make
flush() mandatory. Is it still here for backward compatibility?

[mef, 2016/06/14 00:20:38]

Yes. I think we should keep it optional for compatibility / ease of transition.
It is still present in Android as well.

+    disable_auto_flush_ = disable_auto_flush;
+  }
+
+  // Delays sending request headers until first call to Flush().
+  void delay_headers_until_flush(bool delay_headers_until_flush) {

[kapishnikov, 2016/06/13 21:21:57]

Should we use camelcase for the sake of API consistency even though the function
is "cheap"? I think the style guide gives the choice.

[mef, 2016/06/14 00:20:38]

I don't have strong preference, but I think Chromium doesn't use CamelCase for
simple getters/setters.

+    delay_headers_until_flush_ = delay_headers_until_flush;
+  }
+
   // Validates method and headers, initializes and starts the request. If
   // |end_of_stream| is true, then stream is half-closed after sending header
   // frame and no data is expected to be written.
   // Returns 0 if request is valid and started successfully,
   // Returns -1 if |method| is not valid HTTP method name.
   // Returns position of invalid header value in |headers| if header name is
   // not valid.
   int Start(const char* url,
             int priority,
             const char* method,
             const net::HttpRequestHeaders& headers,
             bool end_of_stream);
 
   // Reads more data into |buffer| up to |capacity| bytes.
   bool ReadData(char* buffer, int capacity);
 
   // Writes |count| bytes of data from |buffer|. The |end_of_stream| is
   // passed to remote to indicate end of stream.
   bool WriteData(const char* buffer, int count, bool end_of_stream);
 
+  // Sends buffers passed to WriteData().
+  bool Flush();

[kapishnikov, 2016/06/13 21:21:57]

What does the return boolean mean? Is it possible that the flush fails?

[mef, 2016/06/14 00:20:38]

Good point, changed to void.

+
   // Cancels the request. The OnCanceled callback is invoked when request is
   // caneceled, and not other callbacks are invoked afterwards..
   void Cancel();
 
   // Releases all resources for the request and deletes the object itself.
   void Destroy();
 
  private:
   // States of BidirectionalStream are tracked in |read_state_| and
   // |write_state_|.
@@ skipping 13 matching lines @@
     READING,
     // There is no more data to read and stream is half-closed by the remote
     // side.
     READING_DONE,
     // Stream is canceled.
     CANCELED,
     // Error has occured, stream is closed.
     ERROR,
     // Reading and writing are done, and the stream is closed successfully.
     SUCCESS,
-    // Waiting for WriteData() to be called.
-    WAITING_FOR_WRITE,
+    // Waiting for Flush() to be called.
+    WAITING_FOR_FLUSH,
     // Writing to the remote, callback will be invoked when done.
     WRITING,
     // There is no more data to write and stream is half-closed by the local
     // side.
     WRITING_DONE,
   };
 
+  // Container to hold buffers and sizes of the pending data to be written.
+  struct PendingWriteData {

[kapishnikov, 2016/06/13 21:21:57]

|sending_write_data_|, |flushing_write_data_| and |pending_write_data_| are
using this structure. Should we rename it to something like |DataBuffers| since
the structure by itself is not pinned to any particular state or usage.

[mef, 2016/06/14 00:20:38]

Done.

+    PendingWriteData();
+    ~PendingWriteData();
+
+    // Every IOBuffer in |write_buffer_list| points to the memory owned by the
+    // application.
+    std::vector<scoped_refptr<net::IOBuffer>> write_buffer_list;
+    // A list of the length of each IOBuffer in |write_buffer_list|.
+    std::vector<int> write_buffer_len_list;
+
+    DISALLOW_COPY_AND_ASSIGN(PendingWriteData);
+  };
+
   // net::BidirectionalStream::Delegate implementations:
   void OnStreamReady(bool request_headers_sent) override;
   void OnHeadersReceived(const net::SpdyHeaderBlock& response_headers) override;
   void OnDataRead(int bytes_read) override;
   void OnDataSent() override;
   void OnTrailersReceived(const net::SpdyHeaderBlock& trailers) override;
   void OnFailed(int error) override;
   // Helper method to derive OnSucceeded.
   void MaybeOnSucceded();
 
   void StartOnNetworkThread(
       std::unique_ptr<net::BidirectionalStreamRequestInfo> request_info);
   void ReadDataOnNetworkThread(scoped_refptr<net::WrappedIOBuffer> read_buffer,
                                int buffer_size);
   void WriteDataOnNetworkThread(scoped_refptr<net::WrappedIOBuffer> read_buffer,
                                 int buffer_size,
                                 bool end_of_stream);
+  void FlushOnNetworkThread();
   void CancelOnNetworkThread();
   void DestroyOnNetworkThread();
 
   // Read state is tracking reading flow. Only accessed on network thread.
   //                         / <--- READING <--- \
   //                         |                   |
   //                         \                   /
   // NOT_STARTED -> STARTED --> WAITING_FOR_READ -> READING_DONE -> SUCCESS
   State read_state_;
 
   // Write state is tracking writing flow.  Only accessed on network thread.
   //                         / <--- WRITING <---  \
   //                         |                    |
   //                         \                    /
-  // NOT_STARTED -> STARTED --> WAITING_FOR_WRITE -> WRITING_DONE -> SUCCESS
+  // NOT_STARTED -> STARTED --> WAITING_FOR_FLUSH -> WRITING_DONE -> SUCCESS
   State write_state_;
 
   bool write_end_of_stream_;
+  bool request_headers_sent_;
+
+  bool disable_auto_flush_;
+  bool delay_headers_until_flush_;
 
   CronetEnvironment* const environment_;
 
   scoped_refptr<net::WrappedIOBuffer> read_buffer_;
-  scoped_refptr<net::WrappedIOBuffer> write_buffer_;
+
+  // Write data that is sending.
+  std::unique_ptr<PendingWriteData> sending_write_data_;
+  // Write data that is flushed, but not sending yet.
+  std::unique_ptr<PendingWriteData> flushing_write_data_;
+  // Write data that is pending the flush.
+  std::unique_ptr<PendingWriteData> pending_write_data_;
+
   std::unique_ptr<net::BidirectionalStream> bidi_stream_;
   Delegate* delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(CronetBidirectionalStream);
 };
 
 }  // namespace cronet
 
 #endif  // COMPONENTS_CRONET_IOS_CRONET_BIDIRECTIONAL_STREAM_H_