transport: consume per-stream inflow windows ahead of time - large speedup for big messages #1073

apolcyn · 2017-02-14T03:34:08Z

This is one solution to the problem with grpc vs. http1 that was noticed in #1043. (fixed 64K window causing too many round trips for large messages).

Consumes inflow stream windows based off of requested read size (so e.g., gives stream window update of 2M for 2M grpc message before reading it in, but doesn't change connection window).

This technique used in C-core? cc @ctiller.

Can have big speedup for large messages (e.g, ~10s for 2M request/response down to ~1s, with 150ms RTT).

AFAICS Stream.Read isn't safe to do this with since io.Read could be called repeatedly - possible to do but to me a rename seems safer... So this changes transport package to expose Stream.ReadFull(_), which tries to be equivalent to current io.ReadFull("stream", _)

apolcyn · 2017-02-21T20:01:59Z

cc @menghanl, this is the Read -> ReadFull change to transport.Stream discussed

apolcyn · 2017-02-22T01:33:50Z

cc @sailat

MakMukhi · 2017-03-07T01:09:14Z

transport/transport.go

@@ -186,14 +186,15 @@ type Stream struct {
 	recvCompress string
 	sendCompress string
 	buf          *recvBuffer
-	dec          io.Reader
 	fc           *inFlow
 	recvQuota    uint32
 	// The accumulated inbound quota pending for window update.
 	updateQuota uint32
 	// The handler to control the window update procedure for both this
 	// particular stream and the associated transport.


The comments need to be updated to explain the new behavior.

MakMukhi · 2017-03-07T01:16:40Z

transport/transport.go

+	}
+	defer func() { s.readFullErr = err }()
+	s.streamWindowHandler(int64(len(p)))
+	return io.ReadFull(s.sr, p)


n, err = io.ReadFull(s.sr, p) return

instead of

return io.ReadFull(s.sr, p)

So that the return values n and err get updated and the defer function has a valid value of err to populate s.readFullErr.

Actually, FYI, return values are assigned via a return statement, so return io.ReadFull(s.sr, p) is fine. Example: https://play.golang.org/p/V1qMks7o9s.

MakMukhi · 2017-03-07T01:34:58Z

transport/control.go

@@ -149,45 +149,45 @@ type inFlow struct {
 	mu sync.Mutex
 	// pendingData is the overall data which have been received but not been
 	// consumed by applications.
-	pendingData uint32


The http2 framer.WriteWindowUpdate(streamID, incr uint32) doesn't support int64. It's better to let the sizes be in uint32.

apolcyn · 2017-03-07T18:10:52Z

@MakMukhi

The http2 framer.WriteWindowUpdate(streamID, incr uint32) doesn't support int64. It's better to let the sizes be in uint32.

I think this points to a bug:
The max legal window update and window is (2^31 - 1), but with the max possible grpc message being (2^32 - 1), this PR could give an illegal/overflowing window update.

apolcyn · 2017-03-09T04:21:15Z

This is UNSAFE as is - there is a major bug here: if we do a pre-emptive window read that puts the pendingData in the window below zero, then receive data on the stream but the app doesn't read it, and then the stream is cancelled, then the pending data on that stream won't be given back to the connection (as is the case currently) - the connection will eventually deadlock if it happens enough.

apolcyn · 2017-03-10T01:08:04Z

latest updates tentatively fix the race in which pending data in a closed stream's recvBuffer isn't given back to the transport.

it also moves the API break where transport.Stream exports ReadFull instead of Read

glycerine · 2017-03-27T20:11:18Z

I tried this PR as a solution to the slow performance, but it just truncated my 512 message stream after 3 messages. It seems half-baked/buggy.

apolcyn · 2017-03-28T02:58:13Z

@glycerine there was a major bug here due to a simple misuse of io.LimitReader which I think you were hitting. The latest updates fix that.

Though this could still use some more cleanup, I'd like to get this in as a fix to the high-latency/large-message issues, since it doesn't need the user knobs.

This should really help for scenarios involving:

high latency
large messages
low concurrency (the stream window is dynamic but the 1M conn window still doesn't move).

Of course more than welcome to retest with the latest commits here - I'd expect about 15x speedup for the situation above (with window bottleneck moving from 64K to 1M)

apolcyn · 2017-03-28T06:44:44Z

go1.6.3 only failed on travis on FlowControlLogicRace - likely a real issue.

apolcyn · 2017-03-28T20:14:48Z

actually re-running travis has FlowControlLogicRace pass, and I haven't been able to reproduce that failure either on this branch or master, with go1.6.3 or go1.8

… size

apolcyn · 2017-04-20T22:36:42Z

cc @dfawley btw this is the PR I mentioned offline. Just squashed and rebased to resolve conflicts. I think this is ok for a look.

MakMukhi · 2017-04-24T22:46:48Z

transport/control.go

-	if f.pendingData == 0 {
-		return 0
+	if n > http2MaxWindowUpdate {
+		grpclog.Fatalf("potential window update too large. onRead(n) where n is %v; max n is %v", f.pendingUpdate, http2MaxWindowUpdate)


Should this be a fatalf? When can such a condition occur?

removed this

MakMukhi · 2017-04-24T22:48:46Z

transport/control.go

+	f.mu.Lock()
+	defer f.mu.Unlock()
+	if f.loanedWindowSpace > 0 {
+		grpclog.Fatalf("pre-consuming window space while there is pre-consumed window space still  outstanding")


Again, Fatalf might be too harsh.

MakMukhi · 2017-04-24T23:32:04Z

transport/transport.go

+	}
+	defer func() { r.readFullErr = err }()
+	committedReadAmount := min(maxSingleStreamWindowUpdate, uint32(len(p)))
+	r.loanSpaceInStreamWindow(committedReadAmount)


Seems like we will loan out window space every single time. There may be times when we don't even need the loan.
Does it make sense to have a few if conditions like these:

if len(p) - fc.pendingData < fc.limit { // no need to loan } else { delta = (len(p) - fc.pendingData) - fc.limit loan(delta) }

still haven't addressed this yet but might revisit

MakMukhi · 2017-04-24T23:33:15Z

transport/http_util.go

@@ -495,6 +495,9 @@ func (f *framer) writeSettingsAck(forceFlush bool) error {
 }

 func (f *framer) writeWindowUpdate(forceFlush bool, streamID, incr uint32) error {
+	if incr > http2MaxWindowUpdate {
+		grpclog.Fatalf("attempted window update too large. have %v; max is %v", incr, http2MaxWindowUpdate)


Again Fatalf seems too harsh.

dfawley · 2017-04-24T16:58:03Z

transport/control.go

@@ -161,34 +171,51 @@ type inFlow struct {
 	limit uint32

 	mu sync.Mutex
-	// pendingData is the overall data which have been received but not been
+	// PendingData is the overall data which have been received but not been


You changed the capitalization here but the field is still unexported. Maybe just remove the "PendingData is" part?

dfawley · 2017-04-24T16:59:57Z

transport/control.go

 	// consumed by applications.
 	pendingData uint32
 	// The amount of data the application has consumed but grpc has not sent
 	// window update for them. Used to reduce window update frequency.
 	pendingUpdate uint32
+
+	// This is temporary space in the incoming flow control that can be granted at convenient times
+	// to prevent the sender from stalling for lack flow control space.


nit: lack of flow control

dfawley · 2017-04-24T17:10:12Z

transport/control.go

+	f.mu.Lock()
+	defer f.mu.Unlock()
+	if f.loanedWindowSpace > 0 {
+		grpclog.Fatalf("pre-consuming window space while there is pre-consumed window space still  outstanding")


Nit: extra space between still & outstanding.

dfawley · 2017-04-24T17:14:25Z

transport/control.go

+	}
+	f.loanedWindowSpace = n
+
+	if f.loanedWindowSpace+f.pendingUpdate >= f.limit/4 {


Minor improvement:

wu := f.pendingUpdate + f.loanedWindowSpace if wu >= f.limit/4 { f.pendingUpdate = 0 return wu } return 0

dfawley · 2017-04-24T17:14:44Z

transport/control.go

+	}
+	f.loanedWindowSpace = n
+
+	if f.loanedWindowSpace+f.pendingUpdate >= f.limit/4 {


What is the significance of the 25% here? Maybe make that a const if it's arbitrary?

dfawley · 2017-04-24T23:10:17Z

transport/transport.go

+	if s.readFullErr != nil {
+		return 0, s.readFullErr
+	}
+	defer func() { s.readFullErr = err }()


Do you know if the compiler optimizes this?

If not, this would be better:

n, err = io.ReadFull() s.readFullErr = err return n, err

dfawley · 2017-04-24T23:32:22Z

transport/transport.go

+	if r.readFullErr != nil {
+		return 0, r.readFullErr
+	}
+	defer func() { r.readFullErr = err }()


Similar comment regarding defer vs. straight-line.

MakMukhi · 2017-04-25T22:57:01Z

transport/transport_test.go

+	// before server starts reading.
+	time.Sleep(2 * time.Second)
+	_, err := s.ReadFull(p)
+	if err != nil {


Perhaps log the error

MakMukhi · 2017-04-25T22:58:15Z

transport/transport_test.go

+	}
+	p := make([]byte, len(req))
+	_, err := s.ReadFull(p)
+	if err != nil {


MakMukhi · 2017-04-25T23:04:21Z

transport/transport_test.go

+			p := make([]byte, len(expectedResponseLarge))
+
+			// Give time to server to begin sending before client starts reading.
+			time.Sleep(2 * time.Second)


Is this needed on both sides? The server will sleep for 2 seconds before reading and sending response back. Could we just call s.ReadFull and that'll wait for server to send all p bytes?

I think was needed on both sides only since I wanted to have a good chance that the reader will have started trying to read before any bytes have arrived, on both client and server.

But that said this is racey and I'm wondering if this would be better replaced by unit tests on an inFlow instance ... doing some experimentation.

MakMukhi · 2017-04-25T23:06:37Z

transport/transport_test.go

@@ -1120,10 +1257,12 @@ func TestClientWithMisbehavedServer(t *testing.T) {
 	if err := ct.Write(s, d, &Options{Last: true, Delay: false}); err != nil && err != io.EOF {
 		t.Fatalf("Failed to write: %v", err)
 	}
+	// reflect to get the inner recvBufferReader, which reads without doing window updates


s/reflect/type assert/

MakMukhi · 2017-04-25T23:07:18Z

transport/transport_test.go

@@ -1182,7 +1321,9 @@ func TestEncodingRequiredStatus(t *testing.T) {
 		t.Fatalf("Failed to write the request: %v", err)
 	}
 	p := make([]byte, http2MaxFrameLen)
-	if _, err := s.dec.Read(p); err != io.EOF {
+	// reflect to get the plain recvBufferReader from the stream's stream reader, which doesn't do window updates


same thing here and below

…ate logic

apolcyn · 2017-06-21T01:10:53Z

superseded by #1248

apolcyn assigned menghanl Feb 21, 2017

apolcyn mentioned this pull request Mar 1, 2017

Connection latency significantly affects throughput #1043

Closed

MakMukhi reviewed Mar 7, 2017

View reviewed changes

apolcyn force-pushed the revise_flow_control_master branch 4 times, most recently from ee57995 to d54684c Compare March 9, 2017 01:59

apolcyn force-pushed the revise_flow_control_master branch from 9d2a30d to 605ea24 Compare March 10, 2017 00:32

apolcyn force-pushed the revise_flow_control_master branch from 311884b to eef27ff Compare March 28, 2017 04:01

apolcyn mentioned this pull request Apr 3, 2017

flake seen in TestStreamsQuotaRecovery #1164

Closed

apolcyn force-pushed the revise_flow_control_master branch from ca0352d to 20d2c2c Compare April 20, 2017 18:26

consume per-stream inflow window preemptively based on requested read…

78b0b88

… size

apolcyn force-pushed the revise_flow_control_master branch from 20d2c2c to 78b0b88 Compare April 20, 2017 22:14

MakMukhi reviewed Apr 24, 2017

View reviewed changes

dfawley reviewed Apr 24, 2017

View reviewed changes

inline defers and replace magic number with constant

a4cd790

MakMukhi reviewed Apr 25, 2017

View reviewed changes

apolcyn added 3 commits April 25, 2017 17:48

add streaming test message sizes that reveals a bug in new window upd…

2ce06bb

…ate logic

fix bug in which small window space loans are leaked

a3b4d67

get rid of unnecessary assert and log more in tests

307862f

fix uint underflow bug from previous bug fix

edfb984

apolcyn force-pushed the revise_flow_control_master branch from b9639e3 to edfb984 Compare April 26, 2017 04:03

apolcyn added 2 commits April 26, 2017 16:35

add bytebuf tests to end2end tests

5083fc7

add in padding test to end2end tests that uses http2 server tester

4fe2caf

dfawley assigned apolcyn and unassigned menghanl May 12, 2017

hsaliak mentioned this pull request May 17, 2017

Can make default/initial Window Size configurable ? #760

Closed

MakMukhi mentioned this pull request May 18, 2017

Expand stream's flow control in case of an active read. #1248

Merged

apolcyn closed this Jun 21, 2017

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

transport: consume per-stream inflow windows ahead of time - large speedup for big messages #1073

transport: consume per-stream inflow windows ahead of time - large speedup for big messages #1073

Conversation

apolcyn commented Feb 14, 2017 • edited Loading

apolcyn commented Feb 21, 2017

apolcyn commented Feb 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apolcyn commented Mar 7, 2017 • edited Loading

apolcyn commented Mar 9, 2017

apolcyn commented Mar 10, 2017

glycerine commented Mar 27, 2017

apolcyn commented Mar 28, 2017

apolcyn commented Mar 28, 2017

apolcyn commented Mar 28, 2017

apolcyn commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MakMukhi Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apolcyn commented Jun 21, 2017

apolcyn commented Feb 14, 2017 •

edited

Loading

apolcyn commented Mar 7, 2017 •

edited

Loading

MakMukhi Apr 24, 2017 •

edited

Loading

dfawley Apr 24, 2017 •

edited

Loading