Remove unnecessary array allocation in cronet_bidirectional_stream_adapter.cc

components/cronet/android/cronet_bidirectional_stream_adapter.cc

 // Copyright 2015 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.
 
 #include "cronet_bidirectional_stream_adapter.h"
 
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 31 matching lines @@
 template <typename JavaArrayType>
 size_t SafeGetArrayLength(JNIEnv* env, JavaArrayType jarray) {
   DCHECK(jarray);
   jsize length = env->GetArrayLength(jarray);
   DCHECK_GE(length, 0) << "Invalid array length: " << length;
   return static_cast<size_t>(std::max(0, length));
 }
 
 }  // namespace
 
+PendingWriteData::PendingWriteData(JNIEnv* env,
+                                   jobjectArray jwrite_buffer_list,
+                                   jintArray jwrite_buffer_pos_list,
+                                   jintArray jwrite_buffer_limit_list,
+                                   jboolean jwrite_end_of_stream) {
+  this->jwrite_buffer_list.Reset(env, jwrite_buffer_list);
+  this->jwrite_buffer_pos_list.Reset(env, jwrite_buffer_pos_list);
+  this->jwrite_buffer_limit_list.Reset(env, jwrite_buffer_limit_list);
+  this->jwrite_end_of_stream = jwrite_end_of_stream;
+}
+
+PendingWriteData::~PendingWriteData() {
+  // Reset global references.
+  this->jwrite_buffer_list.Reset();

[mef, 2016/05/13 15:12:58]

nit: you don't need this->

[xunjieli, 2016/05/13 15:35:21]

Done.

+  this->jwrite_buffer_pos_list.Reset();
+  this->jwrite_buffer_limit_list.Reset();
+}
+
 static jlong CreateBidirectionalStream(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& jbidi_stream,
     jlong jurl_request_context_adapter,
     jboolean jdisable_auto_flush) {
   CronetURLRequestContextAdapter* context_adapter =
       reinterpret_cast<CronetURLRequestContextAdapter*>(
           jurl_request_context_adapter);
   DCHECK(context_adapter);
 
@@ skipping 95 matching lines @@
     jboolean jend_of_stream) {
   size_t buffers_array_size = SafeGetArrayLength(env, jbyte_buffers.obj());
   size_t pos_array_size = SafeGetArrayLength(env, jbyte_buffers.obj());
   size_t limit_array_size = SafeGetArrayLength(env, jbyte_buffers.obj());
   if (buffers_array_size != pos_array_size ||
       buffers_array_size != limit_array_size) {
     DLOG(ERROR) << "Illegal arguments.";
     return JNI_FALSE;
   }
 
-  IOBufferWithByteBufferList buffers;
+  std::unique_ptr<PendingWriteData> pending_write_data;
+  pending_write_data.reset(
+      new PendingWriteData(env, jbyte_buffers.obj(), jbyte_buffers_pos.obj(),
+                           jbyte_buffers_limit.obj(), jend_of_stream));
   for (size_t i = 0; i < buffers_array_size; ++i) {
     ScopedJavaLocalRef<jobject> jbuffer(
-        env, env->GetObjectArrayElement(jbyte_buffers, i));
+        env, env->GetObjectArrayElement(
+                 pending_write_data->jwrite_buffer_list.obj(), i));
     void* data = env->GetDirectBufferAddress(jbuffer.obj());
     if (!data)
       return JNI_FALSE;
     jint pos;
-    env->GetIntArrayRegion(jbyte_buffers_pos.obj(), i, 1, &pos);
+    env->GetIntArrayRegion(pending_write_data->jwrite_buffer_pos_list.obj(), i,
+                           1, &pos);
     jint limit;
-    env->GetIntArrayRegion(jbyte_buffers_limit.obj(), i, 1, &limit);
+    env->GetIntArrayRegion(pending_write_data->jwrite_buffer_limit_list.obj(),
+                           i, 1, &limit);
     DCHECK_LE(pos, limit);
-    scoped_refptr<IOBufferWithByteBuffer> write_buffer(
-        new IOBufferWithByteBuffer(
-            env, base::android::JavaParamRef<jobject>(env, jbuffer.Release()),
-            data, pos, limit));
-    buffers.push_back(write_buffer);
+    scoped_refptr<net::WrappedIOBuffer> write_buffer(
+        new net::WrappedIOBuffer(static_cast<char*>(data) + pos));

[mef, 2016/05/13 15:12:58]

Can we keep using IOBufferWithByteBuffer here? Yes, it will hold additional
reference, but it will guarantee that write_buffer is pointing to valid memory
block.

[xunjieli, 2016/05/13 15:35:22]

But we have the same check on line 193. Isn't the check in the constructor of
IOBufferWithByteBuffer not needed then?

[mef, 2016/05/16 13:54:17]

My concern was that Java objects could theoretically get  released / destroyed
before WrappedIOBuffer.
I can't think of the plausible scenario though, so I'm ok with just adding
comment in PendingWriteData that write_buffer_list buffers point to memory owned
by buffers in jwrite_buffer_list.

[xunjieli, 2016/05/16 14:12:28]

Done.

+    pending_write_data->write_buffer_list.push_back(write_buffer);
+    pending_write_data->write_buffer_len_list.push_back(limit - pos);
   }
+
   context_->PostTaskToNetworkThread(
       FROM_HERE,
       base::Bind(&CronetBidirectionalStreamAdapter::WritevDataOnNetworkThread,
-                 base::Unretained(this), buffers, jend_of_stream));
-
+                 base::Unretained(this),
+                 base::Passed(std::move(pending_write_data))));
   return JNI_TRUE;
 }
 
 void CronetBidirectionalStreamAdapter::Destroy(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& jcaller,
     jboolean jsend_on_canceled) {
   // Destroy could be called from any thread, including network thread (if
   // posting task to executor throws an exception), but is posted, so |this|
   // is valid until calling task is complete. Destroy() is always called from
@@ skipping 47 matching lines @@
       env, owner_.obj(), read_buffer_->byte_buffer(), bytes_read,
       read_buffer_->initial_position(), read_buffer_->initial_limit(),
       bidi_stream_->GetTotalReceivedBytes());
   // Free the read buffer. This lets the Java ByteBuffer be freed, if the
   // embedder releases it, too.
   read_buffer_ = nullptr;
 }
 
 void CronetBidirectionalStreamAdapter::OnDataSent() {
   DCHECK(context_->IsOnNetworkThread());
-  DCHECK(!write_buffer_list_.empty());
+  DCHECK(pending_write_data_);
 
   JNIEnv* env = base::android::AttachCurrentThread();
-  base::android::ScopedJavaLocalRef<jclass> byte_buffer_clazz(
-      env, env->FindClass("java/nio/ByteBuffer"));
-  size_t size = write_buffer_list_.size();
-  jobjectArray jbuffer_array =
-      env->NewObjectArray(size, byte_buffer_clazz.obj(), NULL);
-  base::android::CheckException(env);
-  std::vector<int> initial_positions;
-  std::vector<int> initial_limits;
-  for (size_t i = 0; i < size; ++i) {
-    env->SetObjectArrayElement(jbuffer_array, i,
-                               write_buffer_list_[i]->byte_buffer());
-    initial_positions.push_back(write_buffer_list_[i]->initial_position());
-    initial_limits.push_back(write_buffer_list_[i]->initial_limit());
-  }
-  ScopedJavaLocalRef<jobjectArray> jbuffers(env, jbuffer_array);
-  ScopedJavaLocalRef<jintArray> jinitial_positions =
-      base::android::ToJavaIntArray(env, initial_positions);
-  ScopedJavaLocalRef<jintArray> jinitial_limits =
-      base::android::ToJavaIntArray(env, initial_limits);

[mef, 2016/05/13 15:12:58]

yay!

   // Call into Java.
   cronet::Java_CronetBidirectionalStream_onWritevCompleted(
-      env, owner_.obj(), jbuffers.obj(), jinitial_positions.obj(),
-      jinitial_limits.obj(), write_end_of_stream_);
-  // Free the write buffers. This lets the Java ByteBuffer be freed, if the
+      env, owner_.obj(), pending_write_data_->jwrite_buffer_list.obj(),
+      pending_write_data_->jwrite_buffer_pos_list.obj(),
+      pending_write_data_->jwrite_buffer_limit_list.obj(),
+      pending_write_data_->jwrite_end_of_stream);
+  // Free the java objects. This lets the Java ByteBuffers be freed, if the
   // embedder releases it, too.
-  write_buffer_list_.clear();
+  pending_write_data_.reset();
 }
 
 void CronetBidirectionalStreamAdapter::OnTrailersReceived(
     const net::SpdyHeaderBlock& response_trailers) {
   DCHECK(context_->IsOnNetworkThread());
   JNIEnv* env = base::android::AttachCurrentThread();
   cronet::Java_CronetBidirectionalStream_onResponseTrailersReceived(
       env, owner_.obj(), GetHeadersArray(env, response_trailers).obj());
 }
 
@@ skipping 37 matching lines @@
     return;
 
   if (bytes_read < 0) {
     OnFailed(bytes_read);
     return;
   }
   OnDataRead(bytes_read);
 }
 
 void CronetBidirectionalStreamAdapter::WritevDataOnNetworkThread(
-    const IOBufferWithByteBufferList& write_buffer_list,
-    bool end_of_stream) {
+    std::unique_ptr<PendingWriteData> pending_write_data) {
   DCHECK(context_->IsOnNetworkThread());
-  DCHECK(write_buffer_list_.empty());
-  DCHECK(!write_buffer_list.empty());
+  DCHECK(pending_write_data);
+  DCHECK(!pending_write_data_);
   DCHECK(!write_end_of_stream_);
 
   if (stream_failed_) {
     // If stream failed between the time when WritevData is invoked and
     // WritevDataOnNetworkThread is executed, do not call into |bidi_stream_|
     // since the underlying stream might have been destroyed. Do not invoke
     // Java callback either, since onError is posted when |stream_failed_| is
     // set to true.
     return;
   }
 
+  bool end_of_stream = pending_write_data->jwrite_end_of_stream == JNI_TRUE;
+  if (pending_write_data->write_buffer_list.size() == 1) {
+    bidi_stream_->SendData(pending_write_data->write_buffer_list[0],
+                           pending_write_data->write_buffer_len_list[0],
+                           end_of_stream);
+  } else {
+    bidi_stream_->SendvData(pending_write_data->write_buffer_list,
+                            pending_write_data->write_buffer_len_list,
+                            end_of_stream);
+  }
+
   write_end_of_stream_ = end_of_stream;
-  std::vector<net::IOBuffer*> buffers;
-  std::vector<int> lengths;
-  for (const auto& buffer : write_buffer_list) {
-    write_buffer_list_.push_back(buffer);
-    buffers.push_back(buffer.get());
-    lengths.push_back(buffer->initial_limit() - buffer->initial_position());
-  }
-  if (buffers.size() == 1) {
-    bidi_stream_->SendData(buffers[0], lengths[0], end_of_stream);
-  } else {
-    bidi_stream_->SendvData(buffers, lengths, end_of_stream);
-  }
+  pending_write_data_ = std::move(pending_write_data);

[mef, 2016/05/13 15:12:58]

should we assign this before calling SendData? IIRC it doesn't complete
synchronously now, but what if it will change?

[xunjieli, 2016/05/16 14:12:28]

Done.

 }
 
 void CronetBidirectionalStreamAdapter::DestroyOnNetworkThread(
     bool send_on_canceled) {
   DCHECK(context_->IsOnNetworkThread());
   if (send_on_canceled) {
     JNIEnv* env = base::android::AttachCurrentThread();
     cronet::Java_CronetBidirectionalStream_onCanceled(env, owner_.obj());
   }
   delete this;
 }
 
 base::android::ScopedJavaLocalRef<jobjectArray>
 CronetBidirectionalStreamAdapter::GetHeadersArray(
     JNIEnv* env,
     const net::SpdyHeaderBlock& header_block) {
   DCHECK(context_->IsOnNetworkThread());
 
   std::vector<std::string> headers;
   for (const auto& header : header_block) {
     headers.push_back(header.first.as_string());
     headers.push_back(header.second.as_string());
   }
   return base::android::ToJavaArrayOfStrings(env, headers);
 }
 
 }  // namespace cronet