Specify the flow control behavior in BinderChannel protocol by TaWeiTu · Pull Request #258 · grpc/proposal

TaWeiTu · 2021-09-02T09:51:18Z

The flow control mechanism is specified in the internal document but was not included in this proposal. Also, both documents didn't contain the description of the flag FLAG_MESSAGE_DATA_IS_PARTIAL, which is crucial when breaking larger message into smaller chunks for flow control purposes.

This PR copies the description of flow control mechanism in the internal document to the proposal, and explicitly introduce FLAG_MESSAGE_DATA_IS_PARTIAL and its usage. Additionally, the num bytes field in ACKNOWLEDGE_BYTES requests should be of type long (64 bits) instead of int (32 bits) judging from the implenmentation.

L73-java-binderchannel/wireformat.md

sifmelcara · 2021-09-02T10:13:54Z

L73-java-binderchannel/wireformat.md


+## Flow Control
+
+BinderTransport now supports flow control, aiming to keep use of the process transaction buffer to at most 128k. Each transaction we send adds to an internal count of unacknowledged outbound data. If that exceeds 128k, we’ll stop sending transactions until we receive an acknowledgement message from the transport peer. Each transport sends an acknowledgement for each 16k received, which should avoid blocking the transport most of the time.


Probably worth to mention that we want to do this because Android binder implementation has an internal buffer with fixed size?

sifmelcara · 2021-09-03T06:55:14Z

if i understand correctly amendments to accepted proposal should not include number in title and commit message

markb74 · 2021-09-06T12:43:24Z

L73-java-binderchannel/wireformat.md

    *   From either transport to its peer's binder. Reports reception of
-        transaction data for flow control.
+        transaction data for flow control. The "num bytes" field should contain
+        the sum of "data size" of the parcels received until now.


Worth adding a footnote link to Parcel.dataSize()?
https://developer.android.com/reference/android/os/Parcel#dataSize()

markb74 · 2021-09-06T12:43:36Z

L73-java-binderchannel/wireformat.md

 version = int;
 protocol extension flags = int;
-num bytes = int;
+num bytes = long;


Oh good catch, sorry about that!

No problem!

markb74 · 2021-09-06T12:46:31Z

L73-java-binderchannel/wireformat.md

    included.
 *   FLAG_MESSAGE_DATA_IS_PARCELABLE (0x40) - Indicates the message is a parcelable
    object.
+*   FLAG_MESSAGE_DATA_IS_PARTIAL (0x80) - Indicates the message is partial and the receiver should continue reading.


Proposed change to make this clearer:
Where a message doesn't fit into a single transaction, the message will be sent in multiple transactions with this bit set on all but the final transaction of the message.

markb74 · 2021-09-06T12:47:33Z

L73-java-binderchannel/wireformat.md


+## Flow Control
+
+Due to the limited binder buffer size of 1MB, BinderTransport now supports flow control, aiming to keep use of the process transaction buffer to at most 128k. Each transaction we send adds to an internal count of unacknowledged outbound data (here the count refers to the "data size" of the parcels). If that exceeds 128k, we’ll stop sending transactions until we receive an acknowledgement message from the transport peer. Each transport sends an acknowledgement for each 16k received, which should avoid blocking the transport most of the time.


"now supports flow control" -> "supports transport-level flow control"

The reason I say this is that we still need to support real stream-level flow control separately.

markb74 · 2021-09-06T12:50:01Z

L73-java-binderchannel/wireformat.md

+
+Due to the limited binder buffer size of 1MB, BinderTransport now supports flow control, aiming to keep use of the process transaction buffer to at most 128k. Each transaction we send adds to an internal count of unacknowledged outbound data (here the count refers to the "data size" of the parcels). If that exceeds 128k, we’ll stop sending transactions until we receive an acknowledgement message from the transport peer. Each transport sends an acknowledgement for each 16k received, which should avoid blocking the transport most of the time.
+
+For message larger than the flow control window size, the transport can choose to split it into multiple chunks using the FLAG_MESSAGE_DATA_IS_PARTIAL flag.


Let's move this paragraph to a section above Flow Control, called "Large Messages".

Message splitting is only indirectly related to flow control, so including this here is kind of "mixing concerns".

You can say that we limit the amount of message data sent in a single transaction to 16k. Messages larger than that will be sent in multiple sequential transactions, with all but the last transaction having FLAG_MESSAGE_DATA_IS_PARTIAL set.

sifmelcara · 2021-09-06T15:41:17Z

nit: 128k -> 128KB. 16k -> 16KB

TaWeiTu · 2021-09-06T15:55:50Z

nit: 128k -> 128KB. 16k -> 16KB

Done.

markb74 · 2021-09-07T11:04:49Z

L73-java-binderchannel/wireformat.md

+
+## Flow Control
+
+Due to the limited binder buffer size of 1MB, BinderTransport supports


Consider: "Due to the limited binder buffer size of 1MB" -> "Due to Android's limited per-process transaction buffer of 1MB"

Adds a bit more context and makes it clear this buffer size is out of our control.

Done. Thanks for the review and suggestions!

L73: Specify the flow control behavior

d68c352

TaWeiTu requested review from markb74 and sifmelcara September 2, 2021 09:51

TaWeiTu mentioned this pull request Sep 2, 2021

[binder] Handle inbound flow control grpc/grpc#27228

Merged

sifmelcara reviewed Sep 2, 2021

View reviewed changes

Address comments

202bc13

sifmelcara changed the title ~~L73: Specify the flow control behavior~~ Specify the flow control behavior in BinderChannel protocol Sep 3, 2021

Specify the meaning of "num bytes"

d11057c

sifmelcara requested a review from ejona86 September 6, 2021 04:14

markb74 reviewed Sep 6, 2021

View reviewed changes

Address review comments

7bf3001

TaWeiTu force-pushed the binder-transport-flow-control branch from 2d5b172 to 7bf3001 Compare September 6, 2021 13:04

TaWeiTu and others added 2 commits September 6, 2021 21:09

(Partially) format the document

c52b591

Add a link to Parcel.dataSize()

1705d73

k -> KB

9ed0da8

markb74 reviewed Sep 7, 2021

View reviewed changes

markb74 approved these changes Sep 7, 2021

View reviewed changes

Address comments

675248c

sifmelcara merged commit 4c4a06d into grpc:master Sep 27, 2021


		## Flow Control

		BinderTransport now supports flow control, aiming to keep use of the process transaction buffer to at most 128k. Each transaction we send adds to an internal count of unacknowledged outbound data. If that exceeds 128k, we’ll stop sending transactions until we receive an acknowledgement message from the transport peer. Each transport sends an acknowledgement for each 16k received, which should avoid blocking the transport most of the time.


		## Flow Control

		Due to the limited binder buffer size of 1MB, BinderTransport now supports flow control, aiming to keep use of the process transaction buffer to at most 128k. Each transaction we send adds to an internal count of unacknowledged outbound data (here the count refers to the "data size" of the parcels). If that exceeds 128k, we’ll stop sending transactions until we receive an acknowledgement message from the transport peer. Each transport sends an acknowledgement for each 16k received, which should avoid blocking the transport most of the time.


		Due to the limited binder buffer size of 1MB, BinderTransport now supports flow control, aiming to keep use of the process transaction buffer to at most 128k. Each transaction we send adds to an internal count of unacknowledged outbound data (here the count refers to the "data size" of the parcels). If that exceeds 128k, we’ll stop sending transactions until we receive an acknowledgement message from the transport peer. Each transport sends an acknowledgement for each 16k received, which should avoid blocking the transport most of the time.

		For message larger than the flow control window size, the transport can choose to split it into multiple chunks using the FLAG_MESSAGE_DATA_IS_PARTIAL flag.

Conversation

TaWeiTu commented Sep 2, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sifmelcara commented Sep 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markb74 Sep 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markb74 Sep 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sifmelcara commented Sep 6, 2021

Uh oh!

TaWeiTu commented Sep 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

markb74 Sep 6, 2021 •

edited

Loading

markb74 Sep 6, 2021 •

edited

Loading