-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rpc: add BenchmarkGRPCPing #136558
rpc: add BenchmarkGRPCPing #136558
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @tbg)
pkg/rpc/context_test.go
line 2272 at r1 (raw file):
return anyresp, nil }, SU: func(srv grpcutils.GRPCTest_StreamUnaryServer) error {
Does the benchmark use this one-directional stream?
pkg/rpc/context_test.go
line 2302 at r1 (raw file):
cliRPCCtx := newTestContext(uuid.MakeV4(), clock, maxOffset, stopper) cliRPCCtx.NodeID.Set(ctx, 2) cc, err := cliRPCCtx.grpcDialRaw(ctx, remoteAddr, DefaultClass)
I'm just now finding that the sysbench
microbenchmarks exercise the loopbackTransport
, which means that they bypass the TCP stack and disable snappy compression. Is that the case here are well?
pkg/rpc/context_test.go
line 2342 at r1 (raw file):
b.SetBytes(int64(req.Size() + resp.Size())) b.ResetTimer() defer b.StopTimer()
nit: is this needed? Is there some cleanup that this is guarding against measuring?
This benchmarks simple unary requests across a gRPC server with CockroachDB-specific settings (snappy compression, etc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TFTRS!
bors r+
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @cthumuluru-crdb and @nvanbenschoten)
pkg/rpc/context_test.go
line 2272 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Does the benchmark use this one-directional stream?
No, removing.
pkg/rpc/context_test.go
line 2302 at r1 (raw file):
I'm just now finding that the
sysbench
microbenchmarks exercise theloopbackTransport
I assume you'll do something about that, right? I think I saw the effect of this when I looked at some profiles and the only marshaling/unmarshaling I could see was for the raft transport. Though I'm confused why this wouldn't have been elided as well: possible it just happens to not call into a code path that has access to the local transport?
Either way, we don't hit that here. The loopback transport is only used when we are using an rpcContext
to connect to its own AdvertiseAddr
:
/pkg/rpc/context.go#L1351-L1354
transport := tcpTransport
if rpcCtx.ContextOptions.AdvertiseAddr == target && !rpcCtx.ClientOnly {
// See the explanation on loopbackDialFn for an explanation about this.
transport = loopbackTransport
but here we are starting a grpc server at a random unused port. I also verified this (unscientifically) with a printf.
pkg/rpc/context_test.go
line 2342 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
nit: is this needed? Is there some cleanup that this is guarding against measuring?
No, this was left over from an earlier version of the benchmark. Removing.
This benchmarks simple unary requests across a gRPC server with
CockroachDB-specific settings (snappy compression, etc).
The benchmarks show a problematic amount of allocations especially
at small payload sizes, which is likely a common case in CRDB.
We also see that it's highly beneficial to reuse RPC streams, i.e.
to use bidirectional streaming RPCs to implement unary request-response RPCs.
This is because gRPC implements unary RPCs using one-off streams, which
incurs high overhead. I would liken it to CockroachDB using DistSQL for point
lookups.
Results
Informs #134971.