Use the same hpack encoder on a transport and share it between RPCs. #1536

MakMukhi · 2017-09-23T01:12:47Z

hpack encoding will now happen in the loopy writer goroutine right before the header is written on the wire. This is a more efficient use of hpack encoder since it maintains state and can be more performant in subsequent call.
As a result, the stats handler call with OutHeader can not include the wire length of the header written since it's not yet calculated. We might add this field back in the future which would require changing the code quite a bit.

dfawley · 2017-09-25T21:18:02Z

transport/http2_client.go

@@ -209,6 +210,8 @@ func newHTTP2Client(ctx context.Context, addr TargetInfo, opts ConnectOptions) (
 		goAway:            make(chan struct{}),
 		awakenKeepalive:   make(chan struct{}, 1),
 		framer:            newFramer(conn),
+		hBuf:              &buf,


hBuf: &bytes.Buffer{}

I know we talked about this but now I realize that hEnc needs the same buffer.

Yes, that should have been obvious, sorry!

dfawley · 2017-09-25T21:18:37Z

transport/http2_client.go

 	// Check if credentials.PerRPCCredentials were provided via call options.
 	// Note: if these credentials are provided both via dial options and call
 	// options, then both sets of credentials will be applied.
 	if callCreds := callHdr.Creds; callCreds != nil {
 		if !t.isSecure && callCreds.RequireTransportSecurity() {
-			return nil, streamErrorf(codes.Unauthenticated, "transport: cannot send secure credentials on an insecure connection")
+			return nil, streamErrorf(codes.Unauthenticated, "transport: cannot send secure credentials on an insecure conneciton")


dfawley · 2017-09-25T21:23:00Z

transport/http2_client.go

+	// Make the slice of certain predictable size to reduce allocations made by append.
+	hfLen := 7 // :method, :scheme, :path, :authority, content-type, user-agent, te
+	hfLen += len(authData) + len(callAuthData)
+	headerFields := make([]hpack.HeaderField, 0, hfLen)


This will result in reallocations if there is any user-provided metadata.

It probably would be a win to count it first.

I thought of that but we'll need to loop through the and see if there are any reserved headers in there or not twice (one to count and one to actually append in the slice). And that holds true for other headers too. So I ended up counting the ones that are easily countable. And since append allocates twice the current size. I'm guessing there won't be too many allocations happening.
Thoughts?

Could you add some benchmarks to cover this so we can decide? We don't have anything that includes metadata. If the number of entries and size of each entry can be tweaked through settings, that would be ideal. Ideally we could independently configure client headers, server headers, and server trailers. It would be interesting to compare the performance of both approaches in the following scenarios:

no metadata

with a small amount of metadata (~2)

with a large amount of metadata (~20)

dfawley · 2017-09-25T21:23:20Z

transport/http2_client.go

-			Client:      true,
-			WireLength:  bufLen,
+			Client: true,
+			//WireLength: // TODO(mmukhi): Revisit this if needed.


dfawley · 2017-09-25T21:23:33Z

transport/http2_client.go

@@ -961,7 +939,7 @@ func (t *http2Client) handleGoAway(f *http2.GoAwayFrame) {
 		t.notifyError(connectionErrorf(true, nil, "received illegal http2 GOAWAY frame: stream ID %d is even", f.LastStreamID))
 		return
 	}
-	// A client can receive multiple GoAways from server (look at https://github.com/grpc/grpc-go/issues/1387).
+	// A client can recieve multiple GoAways from server (look at https://github.com/grpc/grpc-go/issues/1387).


except after "c"

dfawley · 2017-09-25T21:35:18Z

transport/http2_server.go

-	hEnc := hpack.NewEncoder(hBuf)
-	hEnc.WriteField(hpack.HeaderField{Name: ":status", Value: "200"})
-	hEnc.WriteField(hpack.HeaderField{Name: "content-type", Value: "application/grpc"})
+	headerFields := make([]hpack.HeaderField, 0, 2) // aleast :status, content-type will be there if none else.


I think it makes sense to do the math to avoid the reallocation/copy.

Same argument as above.

dfawley · 2017-09-25T21:35:48Z

transport/http2_server.go

@@ -782,18 +758,13 @@ func (t *http2Server) WriteStatus(s *Stream, st *status.Status) error {
 		headersSent = true
 	}

-	hBuf := bytes.NewBuffer([]byte{}) // TODO(mmukhi): Try and re-use this memory.
-	hEnc := hpack.NewEncoder(hBuf)
+	headerFields := make([]hpack.HeaderField, 0, 2) // grpc-status and grpc-message will be there if none else.


Do the math?

Same argument as above.

dfawley · 2017-09-25T21:35:54Z

transport/http2_server.go

 	if t.stats != nil {
 		outTrailer := &stats.OutTrailer{
-			WireLength: bufLen,
+		// WireLength:TODO(mmukhi): Revisit this later, if needed.


dfawley · 2017-09-25T21:36:05Z

transport/http2_server.go

@@ -938,7 +908,7 @@ func (t *http2Server) keepalive() {
 	maxAge := time.NewTimer(t.kp.MaxConnectionAge)
 	keepalive := time.NewTimer(t.kp.Time)
 	// NOTE: All exit paths of this function should reset their
-	// respective timers. A failure to do so will cause the
+	// respecitve timers. A failure to do so will cause the


*respective

dfawley · 2017-09-25T21:36:36Z

transport/http2_server.go

-		err = t.framer.fr.WriteHeaders(i.p)
-	case *continuationFrame:
-		err = t.framer.fr.WriteContinuation(i.streamID, i.endHeaders, i.headerBlockFragment)
+		t.hBuf.Reset()


Can this logic be shared with the client's?

transport.go or control.go: headerFrameHandler(hEnc, hBuf, framer)

Actually, most of these cases in item handler can be shared between the server and the client along with most of Write method's code and I'm guessing there are other such opportunities too scattered throughout the transport layer. I kept it this way to be consistent with the rest of the code and with hopes that one day we'll refactor the transport to take care of all this duplication.

SGTM, let's try not to put it off for too long. Maybe add a TODO in this PR...

MakMukhi added the Type: Performance Performance improvements (CPU, network, memory, etc) label Sep 23, 2017

MakMukhi requested review from menghanl and dfawley September 23, 2017 01:12

MakMukhi force-pushed the reuse_hEnc branch from c81f8d3 to b817f5e Compare September 23, 2017 01:48

dfawley self-assigned this Sep 25, 2017

MakMukhi added 2 commits September 25, 2017 13:59

Use the same hpack encoder on a transport and share it between RPCs.

567b1ea

Remove WireLength field from OutHeader.

f7f1e8e

MakMukhi force-pushed the reuse_hEnc branch from b817f5e to f7f1e8e Compare September 25, 2017 21:07

dfawley reviewed Sep 25, 2017

View reviewed changes

MakMukhi added 2 commits September 25, 2017 15:35

Post-review updates

2d53de8

mend

bc6999b

dfawley approved these changes Sep 28, 2017

View reviewed changes

dfawley merged commit 6014154 into grpc:master Sep 28, 2017

menghanl added this to the 1.7 Release milestone Sep 29, 2017

MakMukhi deleted the reuse_hEnc branch May 4, 2018 02:09

lock bot locked as resolved and limited conversation to collaborators Oct 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the same hpack encoder on a transport and share it between RPCs. #1536

Use the same hpack encoder on a transport and share it between RPCs. #1536

MakMukhi commented Sep 23, 2017 •

edited

Loading

dfawley Sep 25, 2017

MakMukhi Sep 25, 2017

dfawley Sep 28, 2017

dfawley Sep 25, 2017

dfawley Sep 25, 2017

MakMukhi Sep 25, 2017

dfawley Sep 28, 2017

dfawley Sep 25, 2017

dfawley Sep 25, 2017

dfawley Sep 25, 2017

MakMukhi Sep 25, 2017

dfawley Sep 25, 2017

MakMukhi Sep 25, 2017

dfawley Sep 25, 2017

dfawley Sep 25, 2017

dfawley Sep 25, 2017

MakMukhi Sep 25, 2017

dfawley Sep 28, 2017

Use the same hpack encoder on a transport and share it between RPCs. #1536

Use the same hpack encoder on a transport and share it between RPCs. #1536

Conversation

MakMukhi commented Sep 23, 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

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

MakMukhi commented Sep 23, 2017 •

edited

Loading