Skip to content
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.

Sign up for GitHub

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

Jump to bottom

transport: when using write buffer pooling, use input size instead of size*2 #6983

Merged
merged 5 commits into from
Feb 15, 2024
Merged

transport: when using write buffer pooling, use input size instead of size*2 #6983

merged 5 commits into from
Feb 15, 2024

Conversation

raghav-stripe
Copy link
Contributor

@raghav-stripe raghav-stripe commented Feb 15, 2024 •
edited by dfawley
Loading

This is a minor change to fix an inconsistency in #6309

It seems in the past, for unknown reasons (see comment), the write buffer would be allocated to 2x the input size. It's been fixed since, but is still 2x the size for the shared buffer.

Edit: it seems like the reasons aren't really unknown, the buffer allocation was 2x the size to keep syscalls low. If syscalls become a performance issue, the calling code is able to specify a WriteBufferSize that is larger. We shouldn't be automatcally 2x-ing the buffer size callers pass in.

RELEASE NOTES:

  • Remove legacy behavior that allocates twice the buffer size in the shared write buffer.

Testing
Ran the same command from the original PR (minus -shareConnection=false; it's not a recognized parameter. Added -sharedWriteBuffer=on and -connections=100). There are no significant differences.

go run benchmark/benchmain/main.go -workloads all -kbps 0 -latency 0s -maxConcurrentCalls 1000 -reqSizeBytes 1 -respSizeBytes 1 -benchtime 1m -sleepBetweenRPCs=20s -sharedWriteBuffer=on -connections=100 -resultFile /tmp/master2 -memProfile /tmp/master2-mem

Before change (I just git revert):

➜  grpc-go go tool pprof -top -inuse_space /tmp/master2-mem
Type: inuse_space
Time: Feb 14, 2024 at 8:20pm (PST)
Showing nodes accounting for 545.04MB, 98.60% of 552.78MB total
Dropped 78 nodes (cum <= 2.76MB)
      flat  flat%   sum%        cum   cum%
  243.60MB 44.07% 44.07%   243.60MB 44.07%  runtime.malg
  132.56MB 23.98% 68.05%   186.56MB 33.75%  google.golang.org/grpc/internal/transport.(*http2Server).operateHeaders
      31MB  5.61% 73.66%    31.50MB  5.70%  google.golang.org/grpc/benchmark.(*testServer).UnconstrainedStreamingCall
   27.51MB  4.98% 78.63%    76.01MB 13.75%  google.golang.org/grpc.(*Server).processStreamingRPC
   18.50MB  3.35% 81.98%    18.50MB  3.35%  google.golang.org/grpc/internal/transport.newWriteQuota (inline)
   16.50MB  2.99% 84.96%    16.50MB  2.99%  context.WithValue
   15.50MB  2.80% 87.77%    15.50MB  2.80%  google.golang.org/grpc/internal/transport.newRecvBuffer (inline)
   10.63MB  1.92% 89.69%    10.63MB  1.92%  google.golang.org/grpc/internal/transport.(*loopyWriter).registerStreamHandler (inline)
    9.50MB  1.72% 91.41%     9.50MB  1.72%  google.golang.org/grpc/internal/status.New (inline)
       8MB  1.45% 92.86%        8MB  1.45%  time.Sleep
       8MB  1.45% 94.30%    11.50MB  2.08%  google.golang.org/grpc.newContextWithRPCInfo (inline)
    7.50MB  1.36% 95.66%     7.50MB  1.36%  context.(*cancelCtx).Done
       7MB  1.27% 96.93%        7MB  1.27%  context.newCancelCtx (inline)
    5.15MB  0.93% 97.86%     5.15MB  0.93%  runtime.allgadd
    3.09MB  0.56% 98.42%     3.09MB  0.56%  bufio.NewReaderSize (inline)
    0.50MB  0.09% 98.51%     7.50MB  1.36%  context.WithCancel
    0.50MB  0.09% 98.60%       32MB  5.79%  google.golang.org/grpc/interop/grpc_testing._BenchmarkService_StreamingCall_Handler
         0     0% 98.60%        7MB  1.27%  context.withCancel
         0     0% 98.60%     4.09MB  0.74%  google.golang.org/grpc.(*Server).Serve.func3
         0     0% 98.60%     4.09MB  0.74%  google.golang.org/grpc.(*Server).handleRawConn
         0     0% 98.60%   186.56MB 33.75%  google.golang.org/grpc.(*Server).handleRawConn.func1
         0     0% 98.60%    79.01MB 14.29%  google.golang.org/grpc.(*Server).handleStream
         0     0% 98.60%     4.09MB  0.74%  google.golang.org/grpc.(*Server).newHTTP2Transport
         0     0% 98.60%   186.56MB 33.75%  google.golang.org/grpc.(*Server).serveStreams
         0     0% 98.60%    79.01MB 14.29%  google.golang.org/grpc.(*Server).serveStreams.func2.1
         0     0% 98.60%       10MB  1.81%  google.golang.org/grpc.(*parser).recvMsg
         0     0% 98.60%    10.50MB  1.90%  google.golang.org/grpc.(*serverStream).RecvMsg
         0     0% 98.60%        5MB   0.9%  google.golang.org/grpc.NewContextWithServerTransportStream (inline)
         0     0% 98.60%        3MB  0.54%  google.golang.org/grpc.contextWithServer (inline)
         0     0% 98.60%    10.50MB  1.90%  google.golang.org/grpc.recv
         0     0% 98.60%       10MB  1.81%  google.golang.org/grpc.recvAndDecompress
         0     0% 98.60%    31.50MB  5.70%  google.golang.org/grpc/benchmark.(*testServer).StreamingCall
         0     0% 98.60%       10MB  1.81%  google.golang.org/grpc/benchmark.(*testServer).UnconstrainedStreamingCall.func1
         0     0% 98.60%     8.50MB  1.54%  google.golang.org/grpc/benchmark.(*testServer).UnconstrainedStreamingCall.func2
         0     0% 98.60%       10MB  1.81%  google.golang.org/grpc/internal/transport.(*Stream).Read
         0     0% 98.60%   186.56MB 33.75%  google.golang.org/grpc/internal/transport.(*http2Server).HandleStreams
         0     0% 98.60%    10.63MB  1.92%  google.golang.org/grpc/internal/transport.(*loopyWriter).handle
         0     0% 98.60%    12.19MB  2.21%  google.golang.org/grpc/internal/transport.(*loopyWriter).run
         0     0% 98.60%       10MB  1.81%  google.golang.org/grpc/internal/transport.(*recvBufferReader).Read
         0     0% 98.60%       10MB  1.81%  google.golang.org/grpc/internal/transport.(*recvBufferReader).read
         0     0% 98.60%       10MB  1.81%  google.golang.org/grpc/internal/transport.(*transportReader).Read
         0     0% 98.60%       10MB  1.81%  google.golang.org/grpc/internal/transport.ContextErr
         0     0% 98.60%     4.09MB  0.74%  google.golang.org/grpc/internal/transport.NewServerTransport
         0     0% 98.60%    11.66MB  2.11%  google.golang.org/grpc/internal/transport.NewServerTransport.func2
         0     0% 98.60%     3.59MB  0.65%  google.golang.org/grpc/internal/transport.newFramer
         0     0% 98.60%        5MB   0.9%  google.golang.org/grpc/metadata.NewIncomingContext (inline)
         0     0% 98.60%       10MB  1.81%  google.golang.org/grpc/status.Error (inline)
         0     0% 98.60%     9.50MB  1.72%  google.golang.org/grpc/status.New (inline)
         0     0% 98.60%       10MB  1.81%  io.ReadAtLeast
         0     0% 98.60%       10MB  1.81%  io.ReadFull (inline)
         0     0% 98.60%   248.75MB 45.00%  runtime.newproc.func1
         0     0% 98.60%   248.75MB 45.00%  runtime.newproc1
         0     0% 98.60%   248.75MB 45.00%  runtime.systemstack

After change:

➜  grpc-go go tool pprof -top -inuse_space /tmp/master3-mem
Type: inuse_space
Time: Feb 14, 2024 at 8:14pm (PST)
Showing nodes accounting for 539.80MB, 98.63% of 547.31MB total
Dropped 65 nodes (cum <= 2.74MB)
      flat  flat%   sum%        cum   cum%
  245.10MB 44.78% 44.78%   245.10MB 44.78%  runtime.malg
  110.05MB 20.11% 64.89%   164.55MB 30.07%  google.golang.org/grpc/internal/transport.(*http2Server).operateHeaders
   32.51MB  5.94% 70.83%    78.51MB 14.34%  google.golang.org/grpc.(*Server).processStreamingRPC
   27.50MB  5.03% 75.85%       28MB  5.12%  google.golang.org/grpc/benchmark.(*testServer).UnconstrainedStreamingCall
   19.50MB  3.56% 79.42%    19.50MB  3.56%  google.golang.org/grpc/internal/transport.newRecvBuffer (inline)
      18MB  3.29% 82.71%       18MB  3.29%  context.WithValue
   15.66MB  2.86% 85.57%    15.66MB  2.86%  google.golang.org/grpc/internal/transport.(*loopyWriter).registerStreamHandler (inline)
   14.50MB  2.65% 88.22%    14.50MB  2.65%  google.golang.org/grpc/internal/transport.newWriteQuota (inline)
   11.50MB  2.10% 90.32%    11.50MB  2.10%  context.(*cancelCtx).Done
       7MB  1.28% 91.60%       13MB  2.38%  google.golang.org/grpc.newContextWithRPCInfo (inline)
    6.50MB  1.19% 92.78%     6.50MB  1.19%  context.newCancelCtx (inline)
    6.50MB  1.19% 93.97%     6.50MB  1.19%  time.Sleep
    6.19MB  1.13% 95.10%     6.19MB  1.13%  google.golang.org/grpc/internal/transport.getWriteBufferPool.func1
    5.50MB  1.00% 96.11%     5.50MB  1.00%  google.golang.org/grpc/internal/status.New (inline)
    5.15MB  0.94% 97.05%     5.15MB  0.94%  runtime.allgadd
    4.13MB  0.75% 97.80%     4.13MB  0.75%  bufio.NewReaderSize (inline)
    3.52MB  0.64% 98.45%     3.52MB  0.64%  runtime.doaddtimer
       1MB  0.18% 98.63%       29MB  5.30%  google.golang.org/grpc/interop/grpc_testing._BenchmarkService_StreamingCall_Handler
         0     0% 98.63%     6.50MB  1.19%  context.WithCancel
         0     0% 98.63%     6.50MB  1.19%  context.withCancel
         0     0% 98.63%     4.13MB  0.75%  golang.org/x/net/http2.(*Framer).WriteData (inline)
         0     0% 98.63%     4.13MB  0.75%  golang.org/x/net/http2.(*Framer).WriteDataPadded
         0     0% 98.63%     6.19MB  1.13%  golang.org/x/net/http2.(*Framer).endWrite
         0     0% 98.63%     4.13MB  0.75%  google.golang.org/grpc.(*Server).Serve.func3
         0     0% 98.63%     4.13MB  0.75%  google.golang.org/grpc.(*Server).handleRawConn
         0     0% 98.63%   166.06MB 30.34%  google.golang.org/grpc.(*Server).handleRawConn.func1
         0     0% 98.63%    84.51MB 15.44%  google.golang.org/grpc.(*Server).handleStream
         0     0% 98.63%     4.13MB  0.75%  google.golang.org/grpc.(*Server).newHTTP2Transport
         0     0% 98.63%   166.06MB 30.34%  google.golang.org/grpc.(*Server).serveStreams
         0     0% 98.63%    84.51MB 15.44%  google.golang.org/grpc.(*Server).serveStreams.func2.1
         0     0% 98.63%        7MB  1.28%  google.golang.org/grpc.(*parser).recvMsg
         0     0% 98.63%        7MB  1.28%  google.golang.org/grpc.(*serverStream).RecvMsg
         0     0% 98.63%        4MB  0.73%  google.golang.org/grpc.NewContextWithServerTransportStream (inline)
         0     0% 98.63%     5.50MB  1.00%  google.golang.org/grpc.contextWithServer (inline)
         0     0% 98.63%        7MB  1.28%  google.golang.org/grpc.recv
         0     0% 98.63%        7MB  1.28%  google.golang.org/grpc.recvAndDecompress
         0     0% 98.63%       28MB  5.12%  google.golang.org/grpc/benchmark.(*testServer).StreamingCall
         0     0% 98.63%        7MB  1.28%  google.golang.org/grpc/benchmark.(*testServer).UnconstrainedStreamingCall.func1
         0     0% 98.63%     8.50MB  1.55%  google.golang.org/grpc/benchmark.(*testServer).UnconstrainedStreamingCall.func2
         0     0% 98.63%        7MB  1.28%  google.golang.org/grpc/internal/transport.(*Stream).Read
         0     0% 98.63%     6.19MB  1.13%  google.golang.org/grpc/internal/transport.(*bufWriter).Write
         0     0% 98.63%   166.06MB 30.34%  google.golang.org/grpc/internal/transport.(*http2Server).HandleStreams
         0     0% 98.63%    17.72MB  3.24%  google.golang.org/grpc/internal/transport.(*loopyWriter).handle
         0     0% 98.63%     4.13MB  0.75%  google.golang.org/grpc/internal/transport.(*loopyWriter).processData
         0     0% 98.63%    21.85MB  3.99%  google.golang.org/grpc/internal/transport.(*loopyWriter).run
         0     0% 98.63%        7MB  1.28%  google.golang.org/grpc/internal/transport.(*recvBufferReader).Read
         0     0% 98.63%        7MB  1.28%  google.golang.org/grpc/internal/transport.(*recvBufferReader).read
         0     0% 98.63%        7MB  1.28%  google.golang.org/grpc/internal/transport.(*transportReader).Read
         0     0% 98.63%        7MB  1.28%  google.golang.org/grpc/internal/transport.ContextErr
         0     0% 98.63%     4.13MB  0.75%  google.golang.org/grpc/internal/transport.NewServerTransport
         0     0% 98.63%    19.25MB  3.52%  google.golang.org/grpc/internal/transport.NewServerTransport.func2
         0     0% 98.63%     4.13MB  0.75%  google.golang.org/grpc/internal/transport.newFramer
         0     0% 98.63%     3.09MB  0.57%  google.golang.org/grpc/internal/transport.newHTTP2Client.func6
         0     0% 98.63%        7MB  1.28%  google.golang.org/grpc/status.Error (inline)
         0     0% 98.63%     5.50MB  1.00%  google.golang.org/grpc/status.New (inline)
         0     0% 98.63%     8.01MB  1.46%  io.ReadAtLeast
         0     0% 98.63%     8.01MB  1.46%  io.ReadFull (inline)
         0     0% 98.63%     3.52MB  0.64%  runtime.mcall
         0     0% 98.63%     3.52MB  0.64%  runtime.modtimer
         0     0% 98.63%   250.25MB 45.72%  runtime.newproc.func1
         0     0% 98.63%   250.25MB 45.72%  runtime.newproc1
         0     0% 98.63%     3.52MB  0.64%  runtime.park_m
         0     0% 98.63%     3.52MB  0.64%  runtime.resetForSleep
         0     0% 98.63%     3.52MB  0.64%  runtime.resettimer (inline)
         0     0% 98.63%   250.25MB 45.72%  runtime.systemstack
         0     0% 98.63%     6.19MB  1.13%  sync.(*Pool).Get

RELEASE NOTES:

  • transport: when using write buffer pooling, use input size instead of size*2

@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Feb 15, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@codecov Codecov
Copy link

codecov bot commented Feb 15, 2024 •
edited
Loading

Codecov Report

Merging #6983 (0a08b0e) into master (408139a) will increase coverage by 0.18%.
The diff coverage is 0.00%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #6983      +/-   ##
==========================================
+ Coverage   82.14%   82.32%   +0.18%     
==========================================
  Files         296      296              
  Lines       31452    31451       -1     
==========================================
+ Hits        25835    25891      +56     
+ Misses       4543     4493      -50     
+ Partials     1074     1067       -7
Files Coverage Δ
dialoptions.go 88.84% <ø> (ø)
server.go 81.27% <ø> (ø)
internal/transport/http_util.go 85.94% <0.00%> (+0.34%) ⬆️

... and 15 files with indirect coverage changes

@raghav-stripe raghav-stripe mentioned this pull request Feb 15, 2024
Merged
dfawley
dfawley reviewed Feb 15, 2024
View reviewed changes
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this change, but then we need to change the docstring in grpc.[With]WriteBufferSize to remove the bit about allocating double for historical reasons.

@dfawley dfawley added this to the 1.63 Release milestone Feb 15, 2024
@raghav-stripe raghav-stripe removed their assignment Feb 15, 2024
@raghav-stripe raghav-stripe marked this pull request as draft February 15, 2024 16:24
@raghav-stripe raghav-stripe marked this pull request as ready for review February 15, 2024 16:26
@dfawley dfawley changed the title Create write buffers of input size instead of size*2 transport: when using write buffer pooling, use input size instead of size*2 Feb 15, 2024
@dfawley dfawley requested a review from zasweq February 15, 2024 20:57
@dfawley
Copy link
Member

dfawley commented Feb 15, 2024 •
edited
Loading

+@zasweq for a second review.

zasweq
zasweq approved these changes Feb 15, 2024
View reviewed changes
Copy link
Contributor

@zasweq zasweq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zasweq zasweq merged commit 3c2a44d into grpc:master Feb 15, 2024
14 checks passed
@raghav-stripe raghav-stripe deleted the raghav-buffer-size branch February 16, 2024 23:16
@atollena atollena mentioned this pull request Mar 29, 2024
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@zasweq zasweq zasweq approved these changes

@dfawley dfawley dfawley approved these changes

Assignees

@zasweq zasweq

Labels
Type: Behavior Change Behavior changes not categorized as bugs
Projects
None yet
Milestone
1.63 Release
Development

Successfully merging this pull request may close these issues.
3 participants
@raghav-stripe @dfawley @zasweq