credentials/alts: add simple TCP-based ALTS benchmark #8636
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8636 +/- ##
==========================================
- Coverage 81.96% 81.87% -0.09%
==========================================
Files 415 416 +1
Lines 40694 40789 +95
==========================================
+ Hits 33355 33397 +42
- Misses 5950 6019 +69
+ Partials 1389 1373 -16 🚀 New features to boost your workflow:
|
|
@gtcooke94 Could you please review this? Thanks. |
easwars
added
Type: Testing
Area: Auth
|
Just want to check, the purpose here is quick performance checks on ALTS internals? |
|
Yep, just ALTS internals. This is helpful for benchmarking performance changes, and I've been using it for looking at throughput + memory allocation behavior. I find it useful, and it'd be a lot easier for me to have it upstreamed here. But no offense if the project would rather not add it -- I can live with that. |
This has been useful to me for quick testing to see whether changes are performant. Compared to other benchmarks in record_test, this: - Exercises TCP, which is more realistic than byte.Buffer - Runs the reader and writer in parallel rather than sequentually Since it only runs locally, it can be run easily. For example: ``` $ go test -run="^$" -bench="BenchmarkTCP" ./credentials/alts/internal/conn goos: linux goarch: amd64 pkg: google.golang.org/grpc/credentials/alts/internal/conn cpu: AMD Ryzen Threadripper PRO 3945WX 12-Cores BenchmarkTCP/size=1_KiB-12 100 10058898 ns/op 0 Mbps 97.34 cpu-usec/op 97.34 sys-usec/op 0 usr-usec/op BenchmarkTCP/size=4_KiB-12 100 10069605 ns/op 2.979 Mbps 146.2 cpu-usec/op 104.7 sys-usec/op 41.42 usr-usec/op BenchmarkTCP/size=64_KiB-12 100 10510067 ns/op 47.57 Mbps 1116 cpu-usec/op 859.7 sys-usec/op 256.7 usr-usec/op BenchmarkTCP/size=512_KiB-12 100 13707500 ns/op 291.8 Mbps 8167 cpu-usec/op 6872 sys-usec/op 1295 usr-usec/op BenchmarkTCP/size=1_MiB-12 100 17343824 ns/op 461.3 Mbps 16276 cpu-usec/op 13361 sys-usec/op 2915 usr-usec/op BenchmarkTCP/size=4_MiB-12 32 39019702 ns/op 820.1 Mbps 64387 cpu-usec/op 51945 sys-usec/op 12443 usr-usec/op PASS ok google.golang.org/grpc/credentials/alts/internal/conn 7.487s ```
|
@gtcooke94 : Looks like this is good for another pass. |
| } | ||
|
|
||
| // newTCPConnPair returns a pair of conns backed by TCP over loopback. | ||
| func newTCPConnPair(rp string, clientProtected []byte, serverProtected []byte) (*conn, *conn, error) { |
There was a problem hiding this comment.
Given that this function is intended to only be used from the newly added benchmark, you could consider making it a helper function by passing testing.B as the first parameter and calling b.Helper().
Also, please consider replacing calls to panic with calls to b.Fatal or b.Fatalf.
| } | ||
|
|
||
| // newTCPConnPair returns a pair of conns backed by TCP over loopback. | ||
| func newTCPConnPair(rp string, clientProtected []byte, serverProtected []byte) (*conn, *conn, error) { |
There was a problem hiding this comment.
Nit: Please consider replacing rp with recordProtocol as really short variable names make more sense where the scope of the variable is much smaller. This seems to be a reasonable large function, and rp is used way down below.
Also, clientProtected []byte, serverProtected []byte could be shortened as clientProtected, serverProtected []byte
|
|
||
| func benchmarkTCP(b *testing.B, size int) { | ||
| // Initialize the connection. | ||
| client, server, err := newTCPConnPair(rekeyRecordProtocol, nil, nil) |
There was a problem hiding this comment.
If the last two arguments are always expected to be nil, we could remove those parameters and pass nil to NewConn from newTCPConnPair.
| <-listenChan | ||
| clientTCP, err := net.DialTimeout("tcp4", address, 5*time.Second) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("failed to Dial: %w", err) |
There was a problem hiding this comment.
These calls to fmt.Errorf could also be replaced with calls to b.Fatalf and remove the error return value from this function. So, the caller of this function will not have to handle the error and call b.Fatal.
|
|
||
| // Get the ending rusage. | ||
| var endUsage unix.Rusage | ||
| if err := unix.Getrusage(unix.RUSAGE_SELF, &endUsage); err != nil { |
There was a problem hiding this comment.
Would it be possible to use more portable alternatives. The sys/unix package will probably only build on unix and unix-like systems.
Options that I see are:
- similar calls from the
syscallpackage: https://pkg.go.dev/syscall#Getrusage - Metrics from the runtime package: https://pkg.go.dev/syscall#Getrusage
easwars
changed the title
4 participants
This has been useful to me for quick testing to see whether changes are performant. Compared to other benchmarks in record_test, this:
Since it only runs locally, it can be run easily. For example:
RELEASE NOTES: none