Skip to content

feat: share a channel descriptor's tag map instead of copying it per channel - #323

Merged
drake-nominal merged 1 commit into
mainfrom
perf/shared-tags
Aug 27, 2026
Merged

feat: share a channel descriptor's tag map instead of copying it per channel#323
drake-nominal merged 1 commit into
mainfrom
perf/shared-tags

Conversation

@drake-nominal

@drake-nominal drake-nominal commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Core-crate change, and it is BREAKING. Rebased onto main now that #320 has landed; one
commit. Independent of #322 — they touch different parts of stream.rs and can land in either
order.

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 ChannelDescriptor owned its tags, every one of those channels copied the map — once when
the descriptor was built, and again 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. That is once per
channel per flush rather than once per channel per write.

Results — read the caveat

Tagged 6,480-channel record, interleaved A/B over three rounds:

main (post-#320) + this PR
staging, CPU per million points 0.49 s 0.32 s ~35% less
staging, throughput 2.08 Mp/s 2.41 Mp/s ~10%, noisy
local file target, per call 3,852 us 1,029 us 3.7x

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_tags constructor and leaving the field owned:
uglier API, same speed, no break.

Breaking change

ChannelDescriptor::tags becomes Option<Arc<BTreeMap<String, String>>> from
Option<BTreeMap<String, String>>. ChannelDescriptor::new and ::with_tags are unaffected; only
code 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-streaming crate — release-plz will pick
that up from the perf!: prefix and the BREAKING CHANGE trailer.

🤖 Generated with Claude Code

Comment thread nominal-streaming/src/types.rs

@alxhill alxhill left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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)

Base automatically changed from perf/py-only to main August 27, 2026 01:07
@drake-nominal drake-nominal changed the title perf!: share a channel descriptor's tag map instead of copying it per channel feat: share a channel descriptor's tag map instead of copying it per channel Aug 27, 2026
… 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
drake-nominal merged commit 771c76f into main Aug 27, 2026
10 checks passed
@drake-nominal
drake-nominal deleted the perf/shared-tags branch August 27, 2026 02:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants