anthropics / anthropics/buffa

Arc<DescriptorPool> refcount shares a cache line with the pool's hot vectors; per-node pool clones contend across threads

Aperta
#434 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub
Lingua principale
Rust
Stelle
883
Fork
88
Merge medio
3g 19h
PR unite (30g)
42

Descrizione

Cross-thread scaling issue in `buffa-descriptor`'s dynamic messages: the `Arc` reference count and the pool's hottest read-only data appear to share a cache line, and every `DynamicMessage` in a tree holds its own clone of that `Arc`.

`DynamicMessage` is `{ pool: Arc, msg_idx, fields, unknown }`, and nested message values are `DynamicMessage`s too, so `DynamicMessage::new`, `clone`, `decode` (once per nested message decoded), `default_value_ref` for an unset message field, and `drop` all write the pool's strong count — one write per message node, per request, from whichever thread is handling that request. Meanwhile `DescriptorPool::message(idx)` — called on essentially every reflective field access — reads `self.messages`' pointer and length. `DescriptorPool` is a plain (non-`repr(C)`) struct whose first fields are `files: Vec<_>` and `messages: Vec<_>`; inside `ArcInner` the strong/weak counts occupy the first 16 bytes, so with the layout rustc currently picks the `messages` header lands within the same 64-byte line as the counts. The effect in a multi-threaded benchmark (N threads, one shared pool, each thread decoding and reading its own messages) is that per-message throughput degrades with thread count even though no thread touches another's message: the count writes invalidate the line the descriptor reads need.

Two possible remedies, not exclusive:

- Layout: keep the reference counts off the line the read path uses — e.g. `#[repr(C)]` on `DescriptorPool` with a leading `[u8; 48]`/`CachePadded` spacer or the cold fields (`files`, `by_name`, `symbols`) first and the hot `messages`/`enums`/`extensions` vectors after the first 64 bytes; cheap, no API change, and worth a `static_assertions`-style layout test so a field reorder does not silently undo it.
- Ownership: let a message tree share one pool handle — nested `DynamicMessage`s inside a `Value::Message` could hold the index only and borrow the root's pool, or the pool clone could be deferred until a nested message is detached from its parent (`take`/`into`). Larger change; removes the per-node count traffic entirely rather than moving it to a quieter line.

Happy to supply the benchmark shape if useful: one `DescriptorPool` in an `Arc`, T threads each looping decode → read a few fields → drop over their own byte buffers, T from 1 to 16, time per iteration on the slowest thread.

Guida per i contributori

Apri la guida per i contributori

Valutazione

Questa issue non è ancora stata valutata.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.