Remove buf copy when the compressor exist #1427

ZhouyihaiDing · 2017-08-07T17:22:31Z

Re-use the buffer used by compressor. Doesn't need to create new "buf" and copy the old data.

dfawley · 2017-08-07T20:35:28Z

rpc_util.go

-		length uint
+		b          []byte
+		length     uint
+		payloadLen = 1


These two should be const instead.

dfawley · 2017-08-07T20:43:02Z

rpc_util.go

 	}
+
 	if length > math.MaxUint32 {


This is unlikely to even trigger if there is a 4GB message. length should be a uint64 if we're serious about this.

If the size can be larger than 4GB, we need to add one more byte (or 2 to 3 bits?) in the header for every message sent to save the length of the message. It's that worth?

That's not an option -- we must follow the protocol, which is 4 bytes to encode the length.

My point is about the type of the variables. Checking whether a uint is > MaxUint32 will only work on platforms where a uint is 64 bits.

tl;dr: please make length a uint64.

dfawley · 2017-08-07T20:52:40Z

rpc_util.go

 		if cp != nil {
+			// pre-alloc place
+			cbuf.Write(make([]byte, 5))


Be consistent about your use of constants -- use "payloadLen+sizeLen" where you have 5.

dfawley · 2017-08-07T20:52:47Z

rpc_util.go

+		// Write payload format
+		b[0] = byte(compressionMade)
+		// Write length of b into buf
+		binary.BigEndian.PutUint32(b[1:], uint32(length))


1->payloadLen

dfawley · 2017-08-07T20:55:13Z

rpc_util.go


+	var buf []byte


How about inverting things to share the length-writing and wirelength code?

var buf []byte if cp == nil { buf = make([]byte, payloadLen+sizeLen+len(b)) buf[0] = byte(compressionNone) } else { buf = b buf[0] = byte(compressionMade) } binary.BigEndian.PutUint32(buf[1:], uint32(length)) if outPayload != nil { outPayload.WireLength = len(buf) } return buf, nil

You are right. I thought buf = b will assign new alloc for buf, but it turns out it doesn't.

dfawley · 2017-08-08T17:19:15Z

Since len(b) returns a int. It seems wrong because length > math.MaxUint32 will always returns false.

Good point. Only on those systems where "int" is 32 bits. Presumably those systems can't have slices bigger than 4GB, so I suppose this is fine. Why not leave "length" an int (instead of a uint, with casting), then?

dfawley · 2017-08-11T17:08:42Z

call.go

@@ -102,18 +102,21 @@ func sendRequest(ctx context.Context, dopts dialOptions, compressor Compressor,
 			Client: true,
 		}
 	}
-	outBuf, err := encode(dopts.codec, args, compressor, cbuf, outPayload)
+	outBuf, outData, err := encode(dopts.codec, args, compressor, cbuf, outPayload)


rename "outBuf" to "outHdr" please, to represent what it has. Or just "hdr" and "data" would be fine, too -- it might look a bit nicer that way.

Please make a similar change everywhere encode is called.

dfawley · 2017-08-11T17:09:40Z

call.go

 	if err != nil {
 		return err
 	}
 	if c.maxSendMessageSize == nil {
 		return Errorf(codes.Internal, "callInfo maxSendMessageSize field uninitialized(nil)")
 	}
-	if len(outBuf) > *c.maxSendMessageSize {
+	if len(outBuf)+len(outData) > *c.maxSendMessageSize {


I don't think it's well-specified, but I would say actually this check should not include the header. So len(outData) alone is better.

Please make a similar change everywhere encode is called.

dfawley · 2017-08-11T17:10:38Z

call.go

-	if err == nil && outPayload != nil {
+	opts.Last = false
+	errHeader := t.Write(stream, outBuf, opts)
+	opts.Last = true


This should be saved before setting it to false (on line 115) and restored to its previous value here instead of set to true.

There is the same problem in another spot, too, please fix all of them.

dfawley · 2017-08-11T17:12:20Z

call_test.go

 	if err != nil {
 		t.Errorf("Failed to encode the response: %v", err)
 		return
 	}
 	h.t.Write(s, reply, &transport.Options{})
+	h.t.Write(s, replyData, &transport.Options{})


This creates another allocation. This would be better:

o := &transport.Options{} h.t.Write(..., o) h.t.Write(..., o)

This occurs below, too.

dfawley · 2017-08-11T17:33:11Z

rpc_util.go

@@ -289,59 +289,53 @@ func (p *parser) recvMsg(maxReceiveMessageSize int) (pf payloadFormat, msg []byt

 // encode serializes msg and prepends the message header. If msg is nil, it


This comment needs updating.

dfawley · 2017-08-11T17:35:51Z

rpc_util.go

 	}
+
+	length = len(b)


Let's just remove "length" now. len(b) is fine.

dfawley · 2017-08-11T17:41:02Z

rpc_util_test.go

@@ -109,7 +109,8 @@ func TestEncode(t *testing.T) {
 	}{
 		{nil, nil, []byte{0, 0, 0, 0, 0}, nil},


This hardly tests anything!

Done. Assert both header and data.

ZhouyihaiDing · 2017-08-16T22:49:22Z

Update end2end_test.go to make it pass travis. Can you help me review it again?
Thank you very much!

dfawley · 2017-08-23T16:17:05Z

call.go

 	if err != nil {
 		return err
 	}
 	if c.maxSendMessageSize == nil {
 		return Errorf(codes.Internal, "callInfo maxSendMessageSize field uninitialized(nil)")
 	}
-	if len(outBuf) > *c.maxSendMessageSize {
-		return Errorf(codes.ResourceExhausted, "grpc: trying to send message larger than max (%d vs. %d)", len(outBuf), *c.maxSendMessageSize)
+	if len(hdr)+len(data) > *c.maxSendMessageSize {


I think we said we wouldn't include the header as the message for this check. (Also the error message would be out of sync.)

dfawley · 2017-08-23T16:20:24Z

rpc_util.go

+	if len(b) < secondStart {
+		secondStart = len(b)
+	}
+	bufHeader = append(bufHeader, b[:secondStart]...)


Oh, I see what happened here.

Please keep outputting this the way it was before: only the header + all of the message data.

Then pass them into the transport.Write function as two slices (instead of making two separate calls), then combine within there. This way, the knowledge that a frame is 16kb is contained within the transport, which is desirable as it's really a function of the transport not gRPC.

Done. Transport.Write will append data to the header slice.

dfawley · 2017-08-23T23:51:51Z

transport/http2_client.go

-func (t *http2Client) Write(s *Stream, data []byte, opts *Options) error {
-	r := bytes.NewBuffer(data)
+func (t *http2Client) Write(s *Stream, hdr []byte, data []byte, opts *Options) error {
+	secondStart := 16379 // 16384 - payloadLen - sizeLen


I think this should be http2MaxFrameLen - len(hdr), to avoid presuming the length of the header.

dfawley · 2017-08-24T00:03:14Z

transport/http2_client.go

+			if isLastSlice && opts.Last {
+				endStream = true
+			} else if !isLastSlice && len(data) != 0 {
+				r = bytes.NewBuffer(data)


r.Reset(data)

Not Done. I need some time to check the overhead if using slice directly.

dfawley · 2017-08-24T16:09:35Z

transport/http2_client.go

@@ -763,11 +767,19 @@ func (t *http2Client) Write(s *Stream, data []byte, opts *Options) error {
 			t.writableChan <- 0
 			continue
 		}
-		if r.Len() == 0 && t.framer.adjustNumWriters(0) == 1 {
+		if r.Len() == 0 && t.framer.adjustNumWriters(0) == 1 && isLastSlice {


How about moving this after the new if r.Len() == 0 that you added so you don't need to check isLastSlice?

dfawley · 2017-08-24T16:10:12Z

transport/http2_server.go

@@ -821,8 +821,15 @@ func (t *http2Server) WriteStatus(s *Stream, st *status.Status) error {

 // Write converts the data into HTTP2 data frame and sends it out. Non-nil error
 // is returns if it fails (e.g., framing error, transport error).
-func (t *http2Server) Write(s *Stream, data []byte, opts *Options) (err error) {
+func (t *http2Server) Write(s *Stream, hdr []byte, data []byte, opts *Options) (err error) {


....soooo much duplication :/

All of the comments in http2Client.Write apply here as well.

(Don't try to fix the duplication in this PR.)

This reverts commit 01089b2.

grpc#1427 avoided the allocation of an extra buffer by re-using the buffer generated by the Compressor. It did so by separating out the header and data buffers and delegating to the transport layer the chunking of the two into appropriately sized frames. At the transport layer however, given the affinity to generate 16KB frames, we re-concatenated the header and data buffers and subsequently flushed out contiguous chunks. Given that the header buffer was always 5KB in size, merging the data bytes into the header buffer necessarily involved allocations and copying. In practice for every {Client,Server}Transport.Write we would allocate & copy from 0 to ~16KB bytes of the data buffer into the header buffer. By reverting grpc#1427 and re-working the implementation of rpc_util.go:encode, we can stage a single buffer comprised of both the serialized{,compressed} message with the pre-pended message header. The transport layer is again agnostic to the exact delineation and can simply write out 16KB chunks as needed without having to copy/allocate.

This reverts commit 01089b2.

grpc#1427 avoided the allocation of an extra buffer by re-using the buffer generated by the Compressor. It did so by separating out the header and data buffers and delegating to the transport layer the chunking of the two into appropriately sized frames. At the transport layer however, given the affinity to generate 16KB frames, we re-concatenated the header and data buffers and subsequently flushed out contiguous chunks. Given that the header buffer was always 5KB in size, merging the data bytes into the header buffer necessarily involved allocations and copying. In practice for every {Client,Server}Transport.Write we would allocate & copy from 0 to ~16KB bytes of the data buffer into the header buffer. By reverting grpc#1427 and re-working the implementation of rpc_util.go:encode, we can stage a single buffer comprised of both the serialized{,compressed} message with the pre-pended message header. The transport layer is again agnostic to the exact delineation and can simply write out 16KB chunks as needed without having to copy/allocate.

dfawley reviewed Aug 7, 2017

View reviewed changes

ZhouyihaiDing force-pushed the pr_remove_cbuf branch 17 times, most recently from 3c0da79 to 394a7de Compare August 10, 2017 18:44

dfawley self-assigned this Aug 10, 2017

dfawley reviewed Aug 11, 2017

View reviewed changes

ZhouyihaiDing force-pushed the pr_remove_cbuf branch 5 times, most recently from a48e9e6 to 46a54bc Compare August 11, 2017 21:52

dfawley approved these changes Aug 14, 2017

View reviewed changes

ZhouyihaiDing force-pushed the pr_remove_cbuf branch 3 times, most recently from 388cab6 to 3cb43d6 Compare August 14, 2017 23:33

ZhouyihaiDing force-pushed the pr_remove_cbuf branch 2 times, most recently from e3d0ddd to 03d3d52 Compare August 16, 2017 22:31

ZhouyihaiDing removed their assignment Aug 17, 2017

dfawley self-assigned this Aug 17, 2017

ZhouyihaiDing force-pushed the pr_remove_cbuf branch from 03d3d52 to debf39a Compare August 17, 2017 21:43

fix 2 buf send testcase race; add filling first buff

01dd4b2

ZhouyihaiDing force-pushed the pr_remove_cbuf branch from debf39a to 01dd4b2 Compare August 17, 2017 22:52

dfawley added the Type: Performance Performance improvements (CPU, network, memory, etc) label Aug 22, 2017

dfawley reviewed Aug 23, 2017

View reviewed changes

dfawley assigned ZhouyihaiDing and unassigned dfawley Aug 23, 2017

ZhouyihaiDing force-pushed the pr_remove_cbuf branch from 3251fc6 to b24131b Compare August 23, 2017 21:06

transport.write receives 2 slices; revert end2end_test

14bc07e

ZhouyihaiDing force-pushed the pr_remove_cbuf branch from b24131b to 14bc07e Compare August 23, 2017 22:40

ZhouyihaiDing assigned dfawley and unassigned ZhouyihaiDing Aug 24, 2017

dfawley reviewed Aug 24, 2017

View reviewed changes

second slice start position; forceFlush condition

2724578

dfawley approved these changes Aug 24, 2017

View reviewed changes

fill the last frame of the header buff to 16k

b0671b6

dfawley approved these changes Aug 24, 2017

View reviewed changes

dfawley merged commit 01089b2 into grpc:master Aug 25, 2017

irfansharif added a commit to irfansharif/grpc-go-fork that referenced this pull request Aug 27, 2017

Revert "Remove buf copy when the compressor exist (grpc#1427)"

348ffa3

This reverts commit 01089b2.

irfansharif mentioned this pull request Aug 27, 2017

grpc: zero-alloc {Client,Server}Transport.Write paths #1482

Closed

irfansharif added a commit to irfansharif/grpc-go-fork that referenced this pull request Aug 27, 2017

Revert "Remove buf copy when the compressor exist (grpc#1427)"

ec30c14

This reverts commit 01089b2.

menghanl pushed a commit to menghanl/grpc-go that referenced this pull request Aug 30, 2017

Remove buf copy when the compressor exist (grpc#1427)

f21c815

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

		@@ -289,59 +289,53 @@ func (p *parser) recvMsg(maxReceiveMessageSize int) (pf payloadFormat, msg []byt

		// encode serializes msg and prepends the message header. If msg is nil, it

		@@ -109,7 +109,8 @@ func TestEncode(t *testing.T) {
		}{
		{nil, nil, []byte{0, 0, 0, 0, 0}, nil},

Remove buf copy when the compressor exist #1427

Remove buf copy when the compressor exist #1427

Conversation

ZhouyihaiDing commented Aug 7, 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

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 commented Aug 8, 2017 via email

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

ZhouyihaiDing commented Aug 16, 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

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