weaveworks / common

Libraries used in multiple Weave projects
Other
129 stars 92 forks source link

httpgrpc: reuse response body buffers #260

Closed ortuman closed 7 months ago

ortuman commented 1 year ago

Signed-off-by: Miguel Ángel Ortuño ortuman@gmail.com

This PR adapts httpgrpc server to recycle recording buffers after they've been used for copying HTTP responses, aiming to reduce GC pressure.

To safely return body buffers back to the pool, we resorted to gRPC stats.Handler to determine whether responses have been effectively serialized and sent over the wire.

An extra benchmark named BenchmarkServer_ServeHTTP has been included as part of this PR to test the change.

Comparision:

name                                   old time/op    new time/op    delta
Server_ServeHTTP/1k_response_size-8      43.8µs ± 0%    44.5µs ± 0%   +1.54%  (p=0.000 n=9+10)
Server_ServeHTTP/10k_response_size-8     53.2µs ± 0%    52.6µs ± 0%   -1.10%  (p=0.000 n=8+8)
Server_ServeHTTP/100k_response_size-8     117µs ±24%     107µs ±23%     ~     (p=0.113 n=10+9)
Server_ServeHTTP/256k_response_size-8     234µs ±31%     207µs ±23%     ~     (p=0.105 n=10+10)
Server_ServeHTTP/512k_response_size-8     412µs ±31%     347µs ±15%  -15.77%  (p=0.015 n=10+10)
Server_ServeHTTP/1M_response_size-8       751µs ±15%     669µs ±10%     ~     (p=0.089 n=10+10)
Server_ServeHTTP/10M_response_size-8     2.02ms ± 1%    2.02ms ± 1%     ~     (p=0.481 n=10+10)

name                                   old alloc/op   new alloc/op   delta
Server_ServeHTTP/1k_response_size-8      19.1kB ± 0%    19.3kB ± 0%   +0.81%  (p=0.000 n=8+9)
Server_ServeHTTP/10k_response_size-8     84.5kB ± 0%    75.1kB ± 0%  -11.21%  (p=0.000 n=9+8)
Server_ServeHTTP/100k_response_size-8     724kB ± 1%     630kB ±18%  -12.92%  (p=0.003 n=8+10)
Server_ServeHTTP/256k_response_size-8    1.90MB ±11%    1.57MB ±12%  -17.47%  (p=0.000 n=10+10)
Server_ServeHTTP/512k_response_size-8    3.58MB ± 6%    2.89MB ± 7%  -19.19%  (p=0.000 n=9+10)
Server_ServeHTTP/1M_response_size-8      6.56MB ± 1%    5.44MB ± 2%  -17.08%  (p=0.000 n=10+10)
Server_ServeHTTP/10M_response_size-8     21.2MB ± 0%    21.2MB ± 0%     ~     (p=0.573 n=8+10)

name                                   old allocs/op  new allocs/op  delta
Server_ServeHTTP/1k_response_size-8         213 ± 0%       229 ± 0%   +7.51%  (p=0.000 n=10+10)
Server_ServeHTTP/10k_response_size-8        213 ± 0%       229 ± 0%   +7.51%  (p=0.000 n=10+10)
Server_ServeHTTP/100k_response_size-8       227 ± 0%       243 ± 0%   +7.05%  (p=0.000 n=10+10)
Server_ServeHTTP/256k_response_size-8       242 ± 0%       255 ± 3%   +5.39%  (p=0.000 n=9+10)
Server_ServeHTTP/512k_response_size-8       244 ± 3%       262 ± 2%   +7.58%  (p=0.000 n=10+10)
Server_ServeHTTP/1M_response_size-8         266 ± 4%       284 ± 1%   +6.65%  (p=0.000 n=10+10)
Server_ServeHTTP/10M_response_size-8        294 ± 1%       312 ± 0%   +5.94%  (p=0.000 n=10+10)

Here I attach the full benchmark results for the old and the new implementations.

bboreham commented 1 year ago

Thanks, this is a clever idea.

To safely return body buffers back to the pool, we resorted to gRPC stats.Handler to determine whether responses have been effectively serialized and sent over the wire.

Do you have a reference for what makes this safe? E.g. is it stated in the gRPC documentation?

ortuman commented 1 year ago

Do you have a reference for what makes this safe? E.g. is it stated in the gRPC documentation?

There's no explicit statement about it (haven't found it at least). However, the stats.OutPayload has two additional fields that indicate the length of the content that was sent over the wire as well as the timestamp at which the sent took place (which BTW, leads me to think that we could validate if SentTime field is not zero before recycling the body buffer, just in case).

bboreham commented 1 year ago

By my reading of the code, this only applies to server responses. Are there many cases where responses sent by httpgrpc are a sizeable portion of garbage?

Is it possible to apply the same technique to requests, on client or server side? I believe those are much more of a problem.