New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 599902 link

Starred by 2 users
Status: Fixed
Owner:
█ xunji...@chromium.org
Last visit > 30 days ago
Closed: Apr 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Feature

Blocking:
issue 516342
issue 606950



Sign in to add a comment

Coalesce small buffers

Project Member Reported by xunji...@chromium.org, Apr 1 2016 
Each BidirectionalStream.write() creates one data packet. If the write buffers are small, this can be inefficient in terms of packet overhead and number of packets that need to be transmitted.

Suggested approach: add a flush() method to BidirectionalStream, and coalesce buffers if the caller specifies to do so.

Additionally, we can combine the header frame with data frames so that a small header frame doesn't take up one packet.

Comment 1 by xunji...@chromium.org, Apr 1 2016

Labels: -Type-Bug Type-Feature
I expect the first iteration on Android will take me 4 weeks. iOS side change is needed as well if we want to support this on iOS.

Comment 2 by rch@chromium.org, Apr 4 2016 

Are you planning to add an explicit "StartBuffering()" method (or some such) or will you be making the default behavior buffering?

Comment 3 by xunji...@chromium.org, Apr 4 2016 

I think the default behavior will be no-buffering. 

I am planning to add a flag, disableAutoFlush, to BidirectionalStream's constructor. If that flag is on, we will flush when flush() is called. Otherwise, every Write() call will implicitly flush. 

Is something like that will work for QUIC's case?
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 26 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/07a42ce317dfabf8764d5c3ea2887eef1e467905

commit 07a42ce317dfabf8764d5c3ea2887eef1e467905
Author: xunjieli <xunjieli@chromium.org>
Date: Tue Apr 26 20:05:31 2016

Coalesce small buffers in net::BidirectionalStream

This CL adds a flush mode and a flush() method to Cronet's
BidirectionalStream API and buffers writes until flush() is
called. By default, Cronet flushes after every write().

net::BidirectionalStreamQuicImpl uses ScopedPacketBundler
to bundle small writes into one data packet along with
request headers.

net::BidirectionalStreamSpdyImpl will do an extra memcpy
to combine small write buffers into a big one.
BidirectionalStreamSpdyImpl doesn't combine header frame
with data frame currently.

BUG= 599902 

Review URL: https://codereview.chromium.org/1856073002

Cr-Commit-Position: refs/heads/master@{#389865}

[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/components/cronet/android/api/src/org/chromium/net/CronetEngine.java
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/components/cronet/android/cronet_bidirectional_stream_adapter.cc
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/components/cronet/android/cronet_bidirectional_stream_adapter.h
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/components/cronet/ios/cronet_bidirectional_stream.cc
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/components/cronet/ios/cronet_bidirectional_stream.h
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/components/cronet/ios/cronet_c_for_grpc.cc
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/components/cronet/ios/cronet_c_for_grpc.h
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/net/http/bidirectional_stream.cc
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/net/http/bidirectional_stream.h
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/net/http/bidirectional_stream_impl.h
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/net/http/bidirectional_stream_unittest.cc
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/net/http/http_stream_factory_impl_unittest.cc
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/net/quic/bidirectional_stream_quic_impl.cc
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/net/quic/bidirectional_stream_quic_impl.h
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/net/quic/bidirectional_stream_quic_impl_unittest.cc
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/net/quic/quic_chromium_client_stream.cc
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/net/quic/quic_chromium_client_stream.h
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/net/quic/quic_chromium_client_stream_test.cc
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/net/quic/test_tools/quic_test_packet_maker.cc
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/net/quic/test_tools/quic_test_packet_maker.h
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/net/spdy/bidirectional_stream_spdy_impl.cc
[modify] https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905/net/spdy/bidirectional_stream_spdy_impl.h

Comment 5 by xunji...@chromium.org, Apr 26 2016

Blocking: 606950
Status: Fixed (was: Assigned)
Marking this as fixed. Filed  Issue 606950  separately to do the same API change on iOS.
Project Member

Comment 6 by bugdroid1@chromium.org, Jun 3 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bcb0f86e685b9d603bebb97e9574e2e0e32948c2

commit bcb0f86e685b9d603bebb97e9574e2e0e32948c2
Author: xunjieli <xunjieli@chromium.org>
Date: Fri Jun 03 00:49:29 2016

[Cronet]Make delaying sending request headers explicit in bidirectional stream

Always delaying sending request headers when
disableAutoFlush is not safe (in the case of bidirectional
streaming). Because server might be expecting request
headers before sending a response, while client might only
call SendData/SendvData after server responds.

This CL adds an explicit flag to tell
net::BidirectionalStream when to delay sending request
headers and coalesce them with data frames in
SendData/SendvData.

BUG= 599902 

Review-Url: https://codereview.chromium.org/1992953004
Cr-Commit-Position: refs/heads/master@{#397567}

[modify] https://crrev.com/bcb0f86e685b9d603bebb97e9574e2e0e32948c2/components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java
[modify] https://crrev.com/bcb0f86e685b9d603bebb97e9574e2e0e32948c2/components/cronet/android/api/src/org/chromium/net/CronetEngine.java
[modify] https://crrev.com/bcb0f86e685b9d603bebb97e9574e2e0e32948c2/components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java
[modify] https://crrev.com/bcb0f86e685b9d603bebb97e9574e2e0e32948c2/components/cronet/android/cronet_bidirectional_stream_adapter.cc
[modify] https://crrev.com/bcb0f86e685b9d603bebb97e9574e2e0e32948c2/components/cronet/android/cronet_bidirectional_stream_adapter.h
[modify] https://crrev.com/bcb0f86e685b9d603bebb97e9574e2e0e32948c2/components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java
[modify] https://crrev.com/bcb0f86e685b9d603bebb97e9574e2e0e32948c2/components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
[modify] https://crrev.com/bcb0f86e685b9d603bebb97e9574e2e0e32948c2/components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java
[modify] https://crrev.com/bcb0f86e685b9d603bebb97e9574e2e0e32948c2/components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java
[modify] https://crrev.com/bcb0f86e685b9d603bebb97e9574e2e0e32948c2/components/cronet/ios/cronet_bidirectional_stream.cc
[modify] https://crrev.com/bcb0f86e685b9d603bebb97e9574e2e0e32948c2/components/cronet/ios/cronet_bidirectional_stream.h
[modify] https://crrev.com/bcb0f86e685b9d603bebb97e9574e2e0e32948c2/net/http/bidirectional_stream.cc
[modify] https://crrev.com/bcb0f86e685b9d603bebb97e9574e2e0e32948c2/net/http/bidirectional_stream.h
[modify] https://crrev.com/bcb0f86e685b9d603bebb97e9574e2e0e32948c2/net/http/bidirectional_stream_impl.h
[modify] https://crrev.com/bcb0f86e685b9d603bebb97e9574e2e0e32948c2/net/http/bidirectional_stream_unittest.cc
[modify] https://crrev.com/bcb0f86e685b9d603bebb97e9574e2e0e32948c2/net/http/http_stream_factory_impl_unittest.cc
[modify] https://crrev.com/bcb0f86e685b9d603bebb97e9574e2e0e32948c2/net/quic/bidirectional_stream_quic_impl.cc
[modify] https://crrev.com/bcb0f86e685b9d603bebb97e9574e2e0e32948c2/net/quic/bidirectional_stream_quic_impl.h
[modify] https://crrev.com/bcb0f86e685b9d603bebb97e9574e2e0e32948c2/net/quic/bidirectional_stream_quic_impl_unittest.cc
[modify] https://crrev.com/bcb0f86e685b9d603bebb97e9574e2e0e32948c2/net/spdy/bidirectional_stream_spdy_impl.cc
[modify] https://crrev.com/bcb0f86e685b9d603bebb97e9574e2e0e32948c2/net/spdy/bidirectional_stream_spdy_impl.h

Comment 7 by xunji...@chromium.org, Jun 3 2016

Labels: Merge-Request-52
May I have the approval for merging the CL in #6 to M52? The CL is used in Cronet, and it doesn't impact Chrome. Thanks!

Comment 8 by tin...@google.com, Jun 3 2016

Labels: -Merge-Request-52 Merge-Approved-52 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M52 (branch: 2743)
Project Member

Comment 9 by bugdroid1@chromium.org, Jun 3 2016

Labels: -merge-approved-52 merge-merged-2743
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/99c95d82a432bc0e542df6af09c4af8f1d9c4786

commit 99c95d82a432bc0e542df6af09c4af8f1d9c4786
Author: xunjieli <xunjieli@chromium.org>
Date: Fri Jun 03 22:04:49 2016

[Cronet]Make delaying sending request headers explicit in bidirectional stream

Always delaying sending request headers when
disableAutoFlush is not safe (in the case of bidirectional
streaming). Because server might be expecting request
headers before sending a response, while client might only
call SendData/SendvData after server responds.

This CL adds an explicit flag to tell
net::BidirectionalStream when to delay sending request
headers and coalesce them with data frames in
SendData/SendvData.

BUG= 599902 

TBR=mef@chromium.org

NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/1992953004
Cr-Commit-Position: refs/heads/master@{#397567}
(cherry picked from commit bcb0f86e685b9d603bebb97e9574e2e0e32948c2)

Review-Url: https://codereview.chromium.org/2031923006
Cr-Commit-Position: refs/branch-heads/2743@{#214}
Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939}

[modify] https://crrev.com/99c95d82a432bc0e542df6af09c4af8f1d9c4786/components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java
[modify] https://crrev.com/99c95d82a432bc0e542df6af09c4af8f1d9c4786/components/cronet/android/api/src/org/chromium/net/CronetEngine.java
[modify] https://crrev.com/99c95d82a432bc0e542df6af09c4af8f1d9c4786/components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java
[modify] https://crrev.com/99c95d82a432bc0e542df6af09c4af8f1d9c4786/components/cronet/android/cronet_bidirectional_stream_adapter.cc
[modify] https://crrev.com/99c95d82a432bc0e542df6af09c4af8f1d9c4786/components/cronet/android/cronet_bidirectional_stream_adapter.h
[modify] https://crrev.com/99c95d82a432bc0e542df6af09c4af8f1d9c4786/components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java
[modify] https://crrev.com/99c95d82a432bc0e542df6af09c4af8f1d9c4786/components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
[modify] https://crrev.com/99c95d82a432bc0e542df6af09c4af8f1d9c4786/components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java
[modify] https://crrev.com/99c95d82a432bc0e542df6af09c4af8f1d9c4786/components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java
[modify] https://crrev.com/99c95d82a432bc0e542df6af09c4af8f1d9c4786/components/cronet/ios/cronet_bidirectional_stream.cc
[modify] https://crrev.com/99c95d82a432bc0e542df6af09c4af8f1d9c4786/components/cronet/ios/cronet_bidirectional_stream.h
[modify] https://crrev.com/99c95d82a432bc0e542df6af09c4af8f1d9c4786/net/http/bidirectional_stream.cc
[modify] https://crrev.com/99c95d82a432bc0e542df6af09c4af8f1d9c4786/net/http/bidirectional_stream.h
[modify] https://crrev.com/99c95d82a432bc0e542df6af09c4af8f1d9c4786/net/http/bidirectional_stream_impl.h
[modify] https://crrev.com/99c95d82a432bc0e542df6af09c4af8f1d9c4786/net/http/bidirectional_stream_unittest.cc
[modify] https://crrev.com/99c95d82a432bc0e542df6af09c4af8f1d9c4786/net/http/http_stream_factory_impl_unittest.cc
[modify] https://crrev.com/99c95d82a432bc0e542df6af09c4af8f1d9c4786/net/quic/bidirectional_stream_quic_impl.cc
[modify] https://crrev.com/99c95d82a432bc0e542df6af09c4af8f1d9c4786/net/quic/bidirectional_stream_quic_impl.h
[modify] https://crrev.com/99c95d82a432bc0e542df6af09c4af8f1d9c4786/net/quic/bidirectional_stream_quic_impl_unittest.cc
[modify] https://crrev.com/99c95d82a432bc0e542df6af09c4af8f1d9c4786/net/spdy/bidirectional_stream_spdy_impl.cc
[modify] https://crrev.com/99c95d82a432bc0e542df6af09c4af8f1d9c4786/net/spdy/bidirectional_stream_spdy_impl.h
Project Member

Comment 10 by bugdroid1@chromium.org, Jun 6 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a3fef780c8c8e050a37ecc7175c92bffc30d38fa

commit a3fef780c8c8e050a37ecc7175c92bffc30d38fa
Author: mef <mef@chromium.org>
Date: Mon Jun 06 16:25:12 2016

[Cronet] Send request headers automatically in bidirectional stream on iOS.

BUG= 599902 

Review-Url: https://codereview.chromium.org/2039943002
Cr-Commit-Position: refs/heads/master@{#398045}

[modify] https://crrev.com/a3fef780c8c8e050a37ecc7175c92bffc30d38fa/components/cronet/ios/cronet_bidirectional_stream.cc
Project Member

Comment 11 by bugdroid1@chromium.org, Jun 27 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/22220beab4feedd951cb23fc2a03019174f18ce0

commit 22220beab4feedd951cb23fc2a03019174f18ce0
Author: xunjieli <xunjieli@chromium.org>
Date: Mon Jun 27 13:59:08 2016

[Cronet] Fix BidirectionalStream.flush()

If a flush() is delayed because there is a write pending, we will incorrectly
flush data in mPendingQueue as well. This leads to undeterministic behavior
in buffer coalescing. This CL makes delayed flush() to only flush mFlushQueue.

This CL also fixes a bug where end of stream flag is incorrectly set to true
when there is still data in mPendingQueue.

This CL adds a regression test.

BUG= 599902 

Review-Url: https://codereview.chromium.org/2078353003
Cr-Commit-Position: refs/heads/master@{#402163}

[modify] https://crrev.com/22220beab4feedd951cb23fc2a03019174f18ce0/components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java
[modify] https://crrev.com/22220beab4feedd951cb23fc2a03019174f18ce0/components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java

Comment 12 by xunji...@chromium.org, Aug 17 2017

Blocking: 516342
►

Sign in to add a comment