feat: share a channel descriptor's tag map instead of copying it per channel - #323
Merged
Conversation
alxhill
reviewed
Aug 26, 2026
alxhill
approved these changes
Aug 26, 2026
alxhill
left a comment
Contributor
There was a problem hiding this comment.
technically a break but given we're 0.x and it's very minor, no concerns merging this (we had a similar break going from &ChannelDescriptor to ChannelDescriptor with no major issues)
… channel A wide record is thousands of channels written at one timestamp, all carrying the same tags. Because `ChannelDescriptor` owned its tags, each of those channels copied the map: once building the descriptor, once more when the buffer first saw that channel. Making `tags` an `Arc<BTreeMap<..>>` turns both copies into a refcount bump. The protobuf `Series` still owns its tags, so the map is copied out at flush time -- but once per channel per flush rather than once per channel per write. Measured on a tagged 6,480-channel record: against staging, CPU per million points 0.49s -> 0.32s ~35% less against staging, throughput 2.08 -> 2.41 Mp/s, noisy; the network binds here against a local file target (CPU-bound) 3,852us -> 1,029us per call The staging throughput number is the honest one for a network-bound writer: this buys CPU, not bandwidth. The local figure shows what it is worth when the SDK is the bottleneck. BREAKING CHANGE: `ChannelDescriptor::tags` is now `Option<Arc<BTreeMap<String, String>>>` rather than `Option<BTreeMap<String, String>>`. `ChannelDescriptor::new` and `::with_tags` are unaffected; only code reading or assigning the field directly needs to change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
drake-nominal
force-pushed
the
perf/shared-tags
branch
from
August 27, 2026 01:17
e865c2e to
4abb02d
Compare
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Core-crate change, and it is BREAKING. Rebased onto
mainnow that #320 has landed; onecommit. Independent of #322 — they touch different parts of
stream.rsand can land in eitherorder.
The numbers below were measured with #320 as the baseline, which is now
main.What it does
A wide record is thousands of channels written at one timestamp, all carrying the same tags.
Because
ChannelDescriptorowned its tags, every one of those channels copied the map — once whenthe descriptor was built, and again when the buffer first saw that channel. Making
tagsanArc<BTreeMap<..>>turns both copies into a refcount bump.The protobuf
Seriesstill owns its tags, so the map is copied out at flush time. That is once perchannel per flush rather than once per channel per write.
Results — read the caveat
Tagged 6,480-channel record, interleaved A/B over three rounds:
The staging throughput figure is the honest one for a network-bound writer: this buys CPU, not
bandwidth. The 3.7x local figure is what it is worth when the SDK itself is the bottleneck — a
host running many streams, or doing real work alongside streaming.
Whether a 35% CPU reduction on tagged wide records justifies a breaking change to a public field
is a judgement call, and I would rather state it plainly than bury it. If the answer is no, the
non-breaking alternative is a separate
with_shared_tagsconstructor and leaving the field owned:uglier API, same speed, no break.
Breaking change
ChannelDescriptor::tagsbecomesOption<Arc<BTreeMap<String, String>>>fromOption<BTreeMap<String, String>>.ChannelDescriptor::newand::with_tagsare unaffected; onlycode reading or assigning the field directly needs a deref. Within this repo that was two call
sites.
Note it also needs a major version bump for the
nominal-streamingcrate — release-plz will pickthat up from the
perf!:prefix and the BREAKING CHANGE trailer.🤖 Generated with Claude Code