Cleanup gRPC util funcs for timeout and percent encoding #596

emcfarlane · 2023-09-26T18:15:43Z

Small cleanup on gRPC util funcs. Removes timeout related init func and improves percent decoder.

Fix for timeout value length: grpcEncodeTimeout("45s") was 45000m but should be 45000000u. 8 char limit should only be applied to the value not the value and unit.

-BenchmarkGRPCPercentEncoding-8           3277959               347.2 ns/op            64 B/op          2 allocs/op
-BenchmarkGRPCPercentDecoding-8          17811729                65.90 ns/op           32 B/op          1 allocs/op
-BenchmarkGRPCTimeoutEncoding-8          15360220                78.53 ns/op           40 B/op          4 allocs/op
+BenchmarkGRPCPercentEncoding-8          18958806                64.16 ns/op           32 B/op          1 allocs/op
+BenchmarkGRPCPercentDecoding-8          22806225                53.29 ns/op           16 B/op          1 allocs/op
+BenchmarkGRPCTimeoutEncoding-8          29902784                40.86 ns/op           24 B/op          2 allocs/op

protocol_grpc.go

mattrobenolt · 2023-09-26T18:37:36Z

Side note, in general, I'd be happy to help figure out a workflow to get us setup with benchcmp as a bit more of a standardized way to show results here. Big fan of benchcmp.

emcfarlane · 2023-09-26T18:39:03Z

@mattrobenolt that'd be nice!

protocol_grpc.go

mattrobenolt

Nice.

akshayjshah · 2023-10-02T17:45:05Z

Side note, in general, I'd be happy to help figure out a workflow to get us setup with benchcmp as a bit more of a standardized way to show results here. Big fan of benchcmp.

❤️ That would be amazing, @mattrobenolt. I'm surprised there isn't a halfway decent service for this already :/

`strconv.AppentInt` is available in all supported Go versions, so we might as well use it!

Don't overload the zero value to signal errors. Has no performance penalty.

akshayjshah

Left some comments, all of which I'm going to fix by pushing some commit's to Ed's branch. The two main issues are that:

We're leaving a little performance on the table by allocating extra strings when encoding timeouts,
We've undone a small performance optimization for gRPC handlers, and
I'd prefer not to remove the error return from unit lookup.

Ed, please take a look at my commits and make sure they're okay with you.

akshayjshah · 2023-10-02T17:55:43Z

protocol_grpc.go

+	case 'H':
+		return time.Hour
+	default:
+		return 0


API-wise, this doesn't feel like an improvement to me: we're overloading the zero value in a way that feels odd in Go, and returning an error won't measurably affect performance.

akshayjshah · 2023-10-02T17:58:50Z

protocol_grpc.go

+		size, unit = time.Hour, 'H'
+	}
+	value := timeout / size
+	return strconv.FormatInt(int64(value), 10 /* base */) + string(unit)


As long as we're golfing allocs, we can probably shave an alloc here by using strconv.AppendInt.

protocol_grpc.go

protocol_grpc_test.go

protocol_grpc.go

akshayjshah · 2023-10-02T20:13:52Z

protocol_grpc.go

@@ -134,7 +109,7 @@ type grpcHandler struct {
 }

 func (g *grpcHandler) Methods() map[string]struct{} {
-	return grpcAllowedMethods
+	return map[string]struct{}{http.MethodPost: {}}


This is an extra allocation along the request path. The returned map isn't accessible to Connect users, so I think we ought to keep the code as-is.

Was there a safety concern here, or were we just trying to get rid of globals?

protocol_grpc_test.go

akshayjshah · 2023-10-02T20:27:08Z

Pushed some extra commits to resolve all the review feedback I left. make bench now runs all the benchmarks in the root package, and you can use BENCH=Timeout make bench to run just a subset of tests.

With these changes, timeout encoding drops to one allocation.

emcfarlane · 2023-10-02T21:22:02Z

All good with me!

gRPC timeout and percent funcs

1b6eed7

emcfarlane changed the title ~~gRPC util cleanup timeout and percent~~ Cleanup gRPC util funcs for timeout and percent encoding Sep 26, 2023

emcfarlane self-assigned this Sep 26, 2023