grpc-go icon indicating copy to clipboard operation
grpc-go copied to clipboard

Implement new Codec that uses `mem.BufferSlice` instead of `[]byte`

Open PapaCharlie opened this issue 1 year ago • 4 comments

This new codec, as outlined in https://github.com/grpc/grpc-go/issues/6619 will allow the reuse of buffers to their fullest extent. Note that this deliberately does not (yet) implement support for stathandlers, however all the relevant APIs have been updated and both old and new Codec implementations are supported.

RELEASE NOTES:

  • TBD

PapaCharlie avatar Jun 26 '24 19:06 PapaCharlie

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: PapaCharlie / name: Paul Chesnais (f6f8b4834af6c7dc776352eb45d6aa3e183f18c7, 80eab7c9cb5d9c4daa6f84ec8d5433f32aa2db52, 3807e4b5fc9c5a8003bf486f400c4576ef72f301, abb49934647037906becce55f5706d8b6a96dec2, a2a6add47649779a9c814f9c8e7a0d1a2ca015b9, d3863ecee8a328e4a09e4a3896a6bbd949f3e502, 13efcff72258ca73c997dc544452c44bc83587a3, 5624e37e7128f1a95a72199a213e2b7b10a1cd87, ab24c1869948e988afea237acc2cde181bf73886, 5ee8565ae0dd53fa62a6e9bb378a959aa5f9285a, 74ffdf4e2d30270e0d0bf2045f4080760c7904ac, 28f56510d60f9df85b83b822af55c917224b2338, eb442ed75956e0301354d3cc353d9e95a96d5c48, 25adf8aaef62bd5dff656ccc86af8ee43121f0f9, 9962795676e5dad209f7374b249b97f9cc27eb77, 017b72fe45c6a1960468887642a0fc2f22771430, 89ca8a9ca4c0829855fccf05b6d0dc475bcd5210, 2ee31ca049a9f78217460f0db16e88cf1fecf257, af4644b2bcff5d65bd784e324011bbfca65053ab, 37f1c30a22c09208f6d54887aa69eb95d8cee950, 5575dbc7e9e488291a78cabe21f3a6cb8d4dea86, df282ce1df6b4dbd96a85b225e38777c208fb2b3, cf49cd110a7c02ec47a8a9bb59a0af60603456b1, b6894dde5b0a9f627c6e16d724ac4bea9ada49b5, 9be25eb04b9401e6ab939e8cf3b32f8f511c4f45, 978d44897d8a9a570d24868dbeade9ab5f2b5e30, d9f1aa7cd4ecfedcd2791d372453d1f4cb73287e, af0a194a6daae86a04ea2e4b64d52717db636297, 462e0315ddec4075d223ff505a5bb9c3454597fd, 5a1e22ed9276f7457e5502290d854766ebea99b9, 3e7f3d4c72f32467a6e9e18e3f7b5bf9a5523247, 14c288c0e4d0daf12e5aa304071a60cfd1cf1a33, e9deef0096d6b59551a954b8858f196e8c4a69bd, b4b467fa852873e3be3ea4c2592696941110397f, c029ea2c29d7c6fa8ed0f472f7fcd2246bbcbd91, 284c19c544e197a0c9a6b37e0373459558d8c589, 3a8cac5849361accf6189af5a07755d18a7aa6c2, 12bf91d427a2e01fc13bd353d7036393810a0175, 3ef511e5ad830c7504ddc7e89c148fd132864d55, 981a5dc17c070a44451221cf562daab0d6834052, 842d3fc08e21215dae92d0419ed8a65124213a4c, 527b6447c6e19df97e9cec35571f2dae32508c70, 5cbb1d772f861c3c63346397ed59556c0ad2002e, 23975fc60fe6d681065a42e2206e8554aab8ac5c, 1ba78946db3bcc6eabe0b3cd3e141dfebae13985, 466a0ceabc5db0ff20eb68dd2adb6047925373de, 9a877dd61edfd45cc1c43944129622fafb7100e9, c2d9987e6367784de8d9b88560fb6b734604ed37, 0889be47db02012eab21b53f63d6e2908e75f77c, aaca19593fc56359638935d2af839562f495ad71, 84c58176d62131f0437644898bc0a2325b1872e5, c5ed95110455eab79945ef8d0e3d20794729cf60, be4d4ab8d90af3d547096fd09e7b910d0326cc3e, 152db3d573d87f3a6582ff821e27a894e4fd996a, 710ee3d223ff7b13498e5a756f55313af272c612, 30f191bc3333b44a43294949167c641412dd0d85, dace88fa4ca8135f92ecf873d76493a03084f08e, 8c401327477edc8920f72f85800e18cbf830f7ed, 87df011dab423c0e1eb8543ec4144feb2c93f592, aa33829409a62eb8e942df828e1f39b6f9182f8e, 19cc6b3df706a56b95dae362cc9c2130531bbf24, b82bf1f4a84ea57c3c1ff29bc722599d8b2c1419, 6e2146e7be6bea06f23e1c1cba4d8b9a7245be09, 507471550fbd751a9dad928b28f2cb2a760abb43, 35783823f74bda31d8ef419acd269993fcb93d13, 5aa0784350f056977ebfee54ecc15ff78ef97823, afcd86a1f0f9974a2ad5d41432c9ba74acd3e182, 4949fc486dd03817e5e72e2d3ca3b173fdf21013, c87426e0c7b50fd3d01254781ab55a2de6dfd99e, d4b487a151bd8b4bdb8e730b90dd11f035353952, 189e6205ba3ee4fcfa565564be65c54818ed59d2, ebffde337b93df239baff11d91a227124f64f083, 35f35e4dce92dcbca4c6eb8b802add087d38f422, 6c38c43d928dcd50420eefedfdb76cf9e0d356a3, 0d790994e80128a4d345cb3cadf8c3faf4ff712b, 606b9f29bf4c165c6f0423f92cbe255fe15eabaf, 2efbe0d47072fb69c706f680da4ab5b88a8f47e0, 8898f139487a7a22cf29c55da38c2ee723cfd60b, 7ff751bb373b0e0ed6133d0ba9a89f244ae08485, aaa3f1690abdf828a5d1dcf77bb5a1decd6d8b38, 4b0c81e840e1fa6037efef6457f80ecc622755e5, d557400b3597f061ab04455922fa404b3d0dfdd7, dab5e1e7152dd2e9a631e5402aa893d33db60151, 83d0322265d74d3f478aca5018f7e776efa97c74, 299260c48d3c78cff3ad915a76a9bbf3cd0c582d, 5672329d3974ca765585ec6ddd9fb28723fa1148, 6ad47931c722f9834a22c4690117b06ef4f6dd37, e6c67317584ffcba32d00cfc7cea0e57907ecf87, 87a408c58b5a4bd53cf3312ca0d0dbb37276b603, bcb1b414ba679e672745f0bdd6fb08f3a73fe081, 04691bebe2f035e28e73711426b734fabb8e183c, a7fb1d6968c3e4308c161b14f4d07be10b315aa4, 44376be9bdbf31aca57b33cc69f4ef22e6aa8903, 27f2187e524593114401a84e4e6e931c4f97d8c9)
  • :white_check_mark: login: easwars / name: Easwar Swaminathan (50bda245ffa55af7fbdfd6e5cb2fb10d80973248, 4d0eaa505ee918c69066941d53da2497cdb5e05c, ae308d40d46bc604c597ab6d5434273ac8cfc8fa, 169406f203c37f70a6a8ece33dde1ed7d54e7f2b, 01ac2c61800b097cd4c39ed3edfe0f94d2e23618, 1261d3efb3e4e42996fca5087393e02e62e6c5ac)

Could you please sign the CLA, and also fix the merge conflicts. Thanks.

easwars avatar Jun 26 '24 19:06 easwars

FYI: @printchard

easwars avatar Jun 26 '24 19:06 easwars

@PapaCharlie : Did you get a chance to sign the CLA?

easwars avatar Jul 08 '24 17:07 easwars

Would you mind splitting the mem package out to a separate PR?

We have some ongoing work that requires it, and it might be good to get that in before the rest to unblock that work. Thanks.

easwars avatar Jul 18 '24 19:07 easwars

Codecov Report

Attention: Patch coverage is 86.82386% with 95 lines in your changes missing coverage. Please review.

Project coverage is 81.94%. Comparing base (7e12068) to head (27f2187). Report is 1 commits behind head on master.

Files Patch % Lines
rpc_util.go 79.77% 15 Missing and 3 partials :warning:
mem/buffers.go 82.75% 13 Missing and 2 partials :warning:
internal/leakcheck/leakcheck.go 86.04% 8 Missing and 4 partials :warning:
encoding/proto/proto_v2.go 68.96% 5 Missing and 4 partials :warning:
internal/transport/transport.go 91.91% 4 Missing and 4 partials :warning:
server.go 78.78% 7 Missing :warning:
stream.go 91.02% 4 Missing and 3 partials :warning:
codec.go 82.60% 3 Missing and 1 partial :warning:
encoding/encoding_v2.go 55.55% 2 Missing and 2 partials :warning:
experimental/experimental.go 0.00% 4 Missing :warning:
... and 3 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7356      +/-   ##
==========================================
- Coverage   81.95%   81.94%   -0.02%     
==========================================
  Files         360      362       +2     
  Lines       27508    27831     +323     
==========================================
+ Hits        22545    22805     +260     
- Misses       3778     3831      +53     
- Partials     1185     1195      +10     
Files Coverage Δ
internal/transport/controlbuf.go 89.93% <100.00%> (-0.76%) :arrow_down:
internal/transport/grpchttp2/http2bridge.go 98.31% <100.00%> (ø)
internal/transport/handler_server.go 89.69% <100.00%> (+0.36%) :arrow_up:
internal/transport/http2_client.go 91.92% <100.00%> (+0.05%) :arrow_up:
internal/transport/http2_server.go 90.69% <100.00%> (-0.41%) :arrow_down:
mem/buffer_pool.go 100.00% <100.00%> (+9.43%) :arrow_up:
preloader.go 60.00% <100.00%> (+15.55%) :arrow_up:
stats/stats.go 71.42% <ø> (ø)
dialoptions.go 89.02% <50.00%> (-0.93%) :arrow_down:
internal/grpctest/grpctest.go 73.52% <71.42%> (+3.52%) :arrow_up:
... and 11 more

... and 15 files with indirect coverage changes

codecov[bot] avatar Jul 22 '24 17:07 codecov[bot]

I think that this change should be more explicitly called out that the memory is being reused by default on the release notes as it can break some edge cases where the memory from the request is still being used even after the request finish.

The only think i can see on the release notes about this is:

codec: Implement a new Codec which uses buffer recycling for encoded message (https://github.com/grpc/grpc-go/pull/7356)
introduce a mem package to facilitate buffer reuse (https://github.com/grpc/grpc-go/pull/7432)
Special Thanks: @PapaCharlie

Which does not call out that the feature is enabled by default now.

alanprot avatar Oct 23 '24 17:10 alanprot