mimir icon indicating copy to clipboard operation
mimir copied to clipboard

Reduce memory allocations in functions used to propagate contextual information between gRPC calls

Open pracucci opened this issue 5 months ago • 2 comments

What this PR does

In Mimir ingesters, 10% of object (and memory) allocations come from gRPC metadata.FromIncomingContext(). metadata.FromIncomingContext() is called in few places. The problem of metadata.FromIncomingContext() is that it creates a copy of all metadata map, when we only want to lookup a single key.

Two years ago, metadata.ValueFromIncomingContext() was introduced in gRPC golang library exactly for this reason. The function is still marked as experimental but it was left untouched since was introduced 2 years ago. I propose to use it. If it will get removed, we can always revert back to metadata.FromIncomingContext().

I've done the same in https://github.com/grafana/dskit/pull/502, which I'm vendoring in this PR.

Which issue(s) this PR fixes or relates to

N/A

Checklist

  • [ ] Tests updated.
  • [ ] Documentation added.
  • [x] CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • [ ] about-versioning.md updated with experimental features.

pracucci avatar Mar 04 '24 15:03 pracucci

i was going to rebase and merge this, but Charles' comment makes sense, i'll leave this open

dimitarvdimitrov avatar Mar 11 '24 12:03 dimitarvdimitrov

The CHANGELOG has just been cut to prepare for the next Mimir release. Please rebase main and eventually move the CHANGELOG entry added / updated in this PR to the top of the CHANGELOG document. Thanks!

duricanikolic avatar Mar 12 '24 09:03 duricanikolic

Is there ever a case where we'd want to use metadata.FromIncomingContext()? I'm wondering if we should add a linting rule to avoid re-introducing this in the future.

Good idea. Done in https://github.com/grafana/mimir/pull/7529/commits/b6b64c64a195f08a79ab1e03a4993c8be47de397, where I've spot another place where we can use the metadata.ValueFromIncomingContext().

pracucci avatar May 06 '24 08:05 pracucci

where I've spot another place where we can use the metadata.ValueFromIncomingContext()

Actually that function is unused and will get removed in #7530, which I will merge after this PR.

pracucci avatar May 06 '24 08:05 pracucci