Dedicated goroutine for writing. #1498

MakMukhi · 2017-08-30T23:25:32Z

Don't block a writer on the underlying syscall since the context may expire and the writer will sit blocked on the syscall to finish.
Also, this gives performance benefits.

dfawley

This looks really good for such a big change. Just a few minor things.

dfawley · 2017-09-06T23:02:42Z

.travis.yml

@@ -18,4 +18,4 @@ before_install:

 script:
  - if [[ "$TRAVIS_GO_VERSION" = 1.9* && "$ARCH" != "386" ]]; then ./vet.sh || exit 1; fi
-  - make test testrace
+  - travis_wait make test testrace


Revert in this PR?

dfawley · 2017-09-06T23:03:05Z

server.go

@@ -891,9 +891,6 @@ func (s *Server) processStreamingRPC(t transport.ServerTransport, stream *transp
 		trInfo:                trInfo,
 		statsHandler:          sh,
 	}
-	if ss.cp != nil {


I don't think we want this change in this PR (?). This buffer is still relevant and I think this is beneficial for streaming RPCs.

This is actually needed, since now a SendMsg call can be made again before the previous msg was written on the wire. In such a case if we have a common buffer, we'll run into a race; concurrent access of the underlying buffer by encode and the writer goroutine.

Interesting. Did you run the benchmark for streaming large messages with only one concurrent call/connection? (Does that benchmark do repeated sends on the stream or just one send and one receive?) That is the only place where this change should show much impact, so it would be interesting to see how much, if at all, it was impacted.

The streaming benchmark does sent multiple messages on a stream.
Run on master:

Stream-noTrace-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_1-reqSize_1048576B-respSize_1048576B-Compressor_false latency_0s-kbps_0-MTU_0-maxConcurrentCalls_1-reqSize_1048576B-respSize_1048576B-Compressor_false 2116 4727531 ns/op 10686814 B/op 2390 allocs/op Histogram (unit: ms) Count: 2116 Min: 3.9 Max: 6.1 Avg: 4.73 ------------------------------------------------------------ [ 3.892775, 3.892776) 1 0.0% 0.0% [ 3.892776, 3.892780) 0 0.0% 0.0% [ 3.892780, 3.892801) 0 0.0% 0.0% [ 3.892801, 3.892905) 0 0.0% 0.0% [ 3.892905, 3.893434) 0 0.0% 0.0% [ 3.893434, 3.896116) 0 0.0% 0.0% [ 3.896116, 3.909704) 1 0.0% 0.1% [ 3.909704, 3.978559) 7 0.3% 0.4% [ 3.978559, 4.327462) 240 11.3% 11.8% # [ 4.327462, 6.095440) 1866 88.2% 100.0% ######### [ 6.095440, inf) 1 0.0% 100.0%

On loopy writer:

Stream-noTrace-latency_0s-kbps_0-MTU_0-maxConcurrentCalls_1-reqSize_1048576B-respSize_1048576B-Compressor_false latency_0s-kbps_0-MTU_0-maxConcurrentCalls_1-reqSize_1048576B-respSize_1048576B-Compressor_false 2615 3824685 ns/op 10669306 B/op 1798 allocs/op Histogram (unit: ms) Count: 2615 Min: 3.2 Max: 4.7 Avg: 3.82 ------------------------------------------------------------ [ 3.182852, 3.182853) 1 0.0% 0.0% [ 3.182853, 3.182857) 0 0.0% 0.0% [ 3.182857, 3.182876) 0 0.0% 0.0% [ 3.182876, 3.182968) 0 0.0% 0.0% [ 3.182968, 3.183415) 0 0.0% 0.0% [ 3.183415, 3.185592) 0 0.0% 0.0% [ 3.185592, 3.196198) 2 0.1% 0.1% [ 3.196198, 3.247853) 15 0.6% 0.7% [ 3.247853, 3.499428) 283 10.8% 11.5% # [ 3.499428, 4.724684) 2313 88.5% 100.0% ######### [ 4.724684, inf) 1 0.0% 100.0%

dfawley · 2017-09-06T23:04:59Z

test/end2end_test.go

+	bl := l.beLazy
+	l.mu.Unlock()
+	if bl {
+		// The sleep duration here needs to less that the leakCheck deadline.


dfawley · 2017-09-06T23:05:53Z

test/end2end_test.go

+}
+
+func (l *lazyConn) Write(b []byte) (int, error) {
+	l.mu.Lock()


Atomic instead of lock?

dfawley · 2017-09-06T23:07:09Z

test/end2end_test.go

+	if _, err := tc.EmptyCall(ctx, &testpb.Empty{}); grpc.Code(err) != codes.DeadlineExceeded {
+		t.Fatalf("TestService/EmptyCall(_, _) = _, %v, want _, context.DeadlineExceeded", err)
+	}
+	if time.Now().After(t1.Add(2 * timeout)) {


if time.Since(t1) > 2 * time.Second {

?

dfawley · 2017-09-06T23:11:30Z

transport/control.go

+
+func (qb *quotaPool) acquireWithVersion() (<-chan int, uint64) {
+	qb.mu.Lock()
+	version := qb.version


Maybe an atomic instead of a lock? You still need the lock for incrementing the version at the same time as the add(), and for compareAndExecute, but that is presumably a more-rare operation than just getting the channel.

dfawley · 2017-09-06T23:12:56Z

transport/http2_client.go

@@ -270,44 +262,45 @@ func newHTTP2Client(ctx context.Context, addr TargetInfo, opts ConnectOptions) (
 		return nil, connectionErrorf(true, err, "transport: preface mismatch, wrote %d bytes; want %d", n, len(clientPreface))
 	}
 	if t.initialWindowSize != defaultWindowSize {
-		err = t.framer.writeSettings(true, http2.Setting{
+		err = t.framer.fr.WriteSettings(http2.Setting{


These are still being done without a deadline?

Offline discussion. Follow-up PR will take care of this case.

dfawley · 2017-09-06T23:15:44Z

transport/http2_client.go

+			s.localSendQuota.add(ltq - ps) // It's ok if we make it negative.
+			var endStream bool
+			// See if this is the last frame to be written.
+			if opts.Last {


Is it possible to combine these conditions and still have conditions understandable?

Offline discussion. Leaving it as is. Let's chat more on it if needed.

dfawley · 2017-09-06T23:21:49Z

transport/transport.go

+		case <-done:
+			return
+		}
+	subloop:


How about "hasData" or something to explain what distinguishes this from the above?

MakMukhi requested review from menghanl and dfawley August 30, 2017 23:25

irfansharif mentioned this pull request Aug 31, 2017

perf: investigate gRPC packeting optimizations cockroachdb/cockroach#17370

Closed

dfawley modified the milestone: 1.7 Release Aug 31, 2017

dfawley added the Type: Bug label Aug 31, 2017

lyuxuan assigned dfawley Aug 31, 2017

MakMukhi force-pushed the loopy_writer branch 2 times, most recently from 5e48328 to de64f47 Compare September 1, 2017 00:35

dfawley assigned MakMukhi and unassigned dfawley Sep 1, 2017

MakMukhi assigned dfawley and unassigned MakMukhi Sep 1, 2017

MakMukhi force-pushed the loopy_writer branch from cd360b6 to d855a19 Compare September 2, 2017 13:26

MakMukhi added 10 commits September 5, 2017 13:45

Added test

2f6ecba

Server-side implementation

c6bc0a5

Client implementation

fc647d8

Clean-up

58aac6c

Check for shutdown channel before attempting to write

b2fddc1

mend

baafb40

travis wait

b7a979d

Writer experiences push-back from output buffer

031fa05

Stream level push back is more performant

3895b7f

Fix transport sendQuota leak

b6eeb31

MakMukhi force-pushed the loopy_writer branch from 8b852e3 to b6eeb31 Compare September 5, 2017 20:45

dfawley reviewed Sep 6, 2017

View reviewed changes

Post-review updates.

6ace5ba

MakMukhi changed the title ~~Loopy writer~~ Dedicated goroutine for writing data. Sep 8, 2017

MakMukhi changed the title ~~Dedicated goroutine for writing data.~~ Dedicated goroutine for writing. Sep 8, 2017

MakMukhi mentioned this pull request Sep 12, 2017

High latency while doing concurrent unary rpc over same connection(grpc-go) #1512

Closed

dfawley approved these changes Sep 14, 2017

View reviewed changes

MakMukhi merged commit 894322f into grpc:master Sep 14, 2017

dfawley mentioned this pull request Sep 19, 2017

Unbounded net.Conn writes in user goroutine lead to missed context deadlines/cancellation #1453

Closed

MakMukhi mentioned this pull request Sep 26, 2017

gRPC call is stuck even with timeout set in the context #1078

Closed

dfawley mentioned this pull request Oct 19, 2017

Bug preventing batching of writes #1256

Closed

MakMukhi deleted the loopy_writer branch May 4, 2018 02:06

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dedicated goroutine for writing. #1498

Dedicated goroutine for writing. #1498

MakMukhi commented Aug 30, 2017

dfawley left a comment

dfawley Sep 6, 2017

dfawley Sep 6, 2017

MakMukhi Sep 8, 2017

dfawley Sep 14, 2017

MakMukhi Sep 14, 2017

dfawley Sep 6, 2017

dfawley Sep 6, 2017

dfawley Sep 6, 2017

dfawley Sep 6, 2017

dfawley Sep 6, 2017

MakMukhi Sep 8, 2017

dfawley Sep 6, 2017

MakMukhi Sep 8, 2017

dfawley Sep 6, 2017

Dedicated goroutine for writing. #1498

Dedicated goroutine for writing. #1498

Conversation

MakMukhi commented Aug 30, 2017

dfawley left a 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

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