alts: set max_frame_size in server/client handshake req

[rockspore]

RELEASE NOTES: N/A

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.16%. Comparing base (ebaf486) to head (cf14126).
⚠️ Report is 6 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8583      +/-   ##
==========================================
- Coverage   82.16%   82.16%   -0.01%     
==========================================
  Files         415      415              
  Lines       40686    40688       +2     
==========================================
- Hits        33431    33430       -1     
- Misses       5873     5876       +3     
  Partials     1382     1382              

Files with missing lines	Coverage Δ
credentials/alts/internal/handshaker/handshaker.go	70.20% <100.00%> (-5.82%)	⬇️

... and 26 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[arjan-bal]

Hi @rockspore, there's a concern in increasing the frame sizes. The buffers used by ALTS credentials can only grow, but not shrink. If there are hundreds of subchannels (see internal doc go/gcs-dp-connections-number-issue and yaqs/8697043517219799040) created in an idle gRPC client, these buffers can hold on to significant amount of memory if they are 1MB each. We may want to have a way for the buffers to shrink before making this change. gRPC Go has a buffer pool to reuse byte slices. This pool could be used to reduce the impact of allocating and freeing buffers.

[rockspore]

Hi @rockspore, there's a concern in increasing the frame sizes. The buffers used by ALTS credentials can only grow, but not shrink. If there are hundreds of subchannels (see internal doc go/gcs-dp-connections-number-issue and yaqs/8697043517219799040) created in an idle gRPC client, these buffers can hold on to significant amount of memory if they are 1MB each. We may want to have a way for the buffers to shrink before making this change. gRPC Go has a buffer pool to reuse byte slices. This pool could be used to reduce the impact of allocating and freeing buffers.

Thanks, @arjan-bal. I wasn't aware of this. I think C++ sets 1MB right now and Java uses 128KB. So I was planning to make it all consistent with C++. Given the offline discussion with @matthewstevenson88, I thought we should populate this field nevertheless. Given the concern here, I wonder if we should do 128KB as Java or just 16KB which keeps the existing behavior.

[kevinGC]

The memory issue can be worked around with buffer pooling, no? A simple solution is to call conn.Read with a 1 byte buffer. Once it returns, grab a buffer from a pool and read the rest.

The most obvious downside is an increase in syscalls. A read that took 1-3 syscalls (IIRC the Go runtime will do an "optimistic" nonblocking read, and then if necessary an epoll and read) would take +1 syscall. Maybe this matters for small RPC performance. For large messages it matters less.

[arjan-bal]

A simple solution is to call conn.Read with a 1 byte buffer. Once it returns, grab a buffer from a pool and read the rest.

I think we would need to read the frame header to know the frame length. The header length is 4 bytes.

The benefit of using a larger frame size may help offset the extra syscall, but I'm not 100% sure. I think we should benchmark it using a GCS client to be sure.