[cronet] Re-use Direct ByteBuffer for Cronet upload

This CL makes Cronet upload reuse a Java ByteBuffer object if the
underlying net::IOBuffer's address and buffer length are unchanged.

This should reduce the number of constructor calls to
NewDirectByteBuffer().

Bug: 756841
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester
Change-Id: I751242095e2ba5793750d5a91e2bc3b10ec8b7a1
Reviewed-on: https://chromium-review.googlesource.com/624196Reviewed-by: Andrei Kapishnikov <kapishnikov@chromium.org>
Commit-Queue: Helen Li <xunjieli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496067}

components/cronet/android/cronet_upload_data_stream_adapter.cc

@@ -11,9 +11,11 @@
 #include "base/android/jni_string.h"
 #include "base/bind.h"
 #include "base/logging.h"
+#include "base/memory/ptr_util.h"
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/cronet/android/cronet_url_request_adapter.h"
+#include "components/cronet/android/io_buffer_with_byte_buffer.h"
 #include "jni/CronetUploadDataStream_jni.h"
 
 using base::android::JavaParamRef;
@@ -44,15 +46,16 @@ void CronetUploadDataStreamAdapter::Read(net::IOBuffer* buffer, int buf_len) {
   DCHECK(network_task_runner_);
   DCHECK(network_task_runner_->BelongsToCurrentThread());
   DCHECK_GT(buf_len, 0);
-  DCHECK(!buffer_.get());
-  buffer_ = buffer;
 
-  // TODO(mmenke):  Consider preserving the java buffer across reads, when the
-  // IOBuffer's data pointer and its length are unchanged.
   JNIEnv* env = base::android::AttachCurrentThread();
-  base::android::ScopedJavaLocalRef<jobject> java_buffer(
-      env, env->NewDirectByteBuffer(buffer->data(), buf_len));
-  Java_CronetUploadDataStream_readData(env, jupload_data_stream_, java_buffer);
+  // Allow buffer reuse if |buffer| and |buf_len| are exactly the same as the
+  // ones used last time.
+  if (!(buffer_ && buffer_->io_buffer()->data() == buffer->data() &&
+        buffer_->io_buffer_len() == buf_len)) {
+    buffer_ = base::MakeUnique<ByteBufferWithIOBuffer>(env, buffer, buf_len);
+  }
+  Java_CronetUploadDataStream_readData(env, jupload_data_stream_,
+                                       buffer_->byte_buffer());
 }
 
 void CronetUploadDataStreamAdapter::Rewind() {
@@ -82,7 +85,6 @@ void CronetUploadDataStreamAdapter::OnReadSucceeded(
     bool final_chunk) {
   DCHECK(bytes_read > 0 || (final_chunk && bytes_read == 0));
 
-  buffer_ = nullptr;
   network_task_runner_->PostTask(
       FROM_HERE, base::Bind(&CronetUploadDataStream::OnReadSuccess,
                             upload_data_stream_, bytes_read, final_chunk));

components/cronet/android/cronet_upload_data_stream_adapter.h

@@ -19,6 +19,7 @@ class SingleThreadTaskRunner;
 }  // namespace base
 
 namespace cronet {
+class ByteBufferWithIOBuffer;
 
 // The Adapter holds onto a reference to the IOBuffer that is currently being
 // written to in Java, so may not be deleted until any read operation in Java
@@ -66,9 +67,8 @@ class CronetUploadDataStreamAdapter : public CronetUploadDataStream::Delegate {
   scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_;
   base::WeakPtr<CronetUploadDataStream> upload_data_stream_;
 
-  // Used to keep the read buffer alive until the callback from Java has been
-  // received.
-  scoped_refptr<net::IOBuffer> buffer_;
+  // Keeps the net::IOBuffer and Java ByteBuffer alive until the next Read().
+  std::unique_ptr<ByteBufferWithIOBuffer> buffer_;
 
   DISALLOW_COPY_AND_ASSIGN(CronetUploadDataStreamAdapter);
 };

components/cronet/android/io_buffer_with_byte_buffer.cc

@@ -24,4 +24,14 @@ IOBufferWithByteBuffer::IOBufferWithByteBuffer(
 
 IOBufferWithByteBuffer::~IOBufferWithByteBuffer() {}
 
+ByteBufferWithIOBuffer::ByteBufferWithIOBuffer(JNIEnv* env,
+                                               net::IOBuffer* io_buffer,
+                                               int io_buffer_len)
+    : io_buffer_(io_buffer), io_buffer_len_(io_buffer_len) {
+  byte_buffer_.Reset(
+      env, env->NewDirectByteBuffer(io_buffer_->data(), io_buffer_len_));
+}
+
+ByteBufferWithIOBuffer::~ByteBufferWithIOBuffer() {}
+
 }  // namespace cronet

components/cronet/android/io_buffer_with_byte_buffer.h

@@ -48,6 +48,31 @@ class IOBufferWithByteBuffer : public net::WrappedIOBuffer {
   DISALLOW_COPY_AND_ASSIGN(IOBufferWithByteBuffer);
 };
 
+// Represents a Java direct ByteBuffer backed by a net::IOBuffer. Keeps both the
+// net::IOBuffer and the Java ByteBuffer object alive until destroyed.
+class ByteBufferWithIOBuffer {
+ public:
+  ByteBufferWithIOBuffer(JNIEnv* env,
+                         net::IOBuffer* io_buffer,
+                         int io_buffer_len);
+
+  ~ByteBufferWithIOBuffer();
+  const net::IOBuffer* io_buffer() const { return io_buffer_.get(); }
+  int io_buffer_len() const { return io_buffer_len_; }
+
+  const base::android::JavaRef<jobject>& byte_buffer() const {
+    return byte_buffer_;
+  }
+
+ private:
+  scoped_refptr<net::IOBuffer> io_buffer_;
+  int io_buffer_len_;
+
+  base::android::ScopedJavaGlobalRef<jobject> byte_buffer_;
+
+  DISALLOW_COPY_AND_ASSIGN(ByteBufferWithIOBuffer);
+};
+
 }  // namespace cronet
 
 #endif  // COMPONENTS_CRONET_ANDROID_IO_BUFFER_WITH_BYTE_BUFFER_H_

components/cronet/android/java/src/org/chromium/net/impl/CronetUploadDataStream.java

@@ -216,6 +216,7 @@ public final class CronetUploadDataStream extends UploadDataSink {
                         String.format("Read upload data length %d exceeds expected length %d",
                                 mLength - mRemainingLength, mLength));
             }
+            mByteBuffer.position(0);
             mByteBuffer = null;
             mInWhichUserCallback = UserCallback.NOT_IN_CALLBACK;