Repository navigation
Conversation
Signed-off-by: Lukas Hering <lukasheringb@gmail.com>
4f03c73 to
db0c97c
Compare
krisztianfekete
left a comment
krisztianfekete
left a comment
There was a problem hiding this comment.
Thanks, the perf diff looks great here!
Since only encode_histogram uses the cache and never nests, could we replace ScratchBuf, Offset, Span with a reused String for the duration of the histogram? See my inline comments/questions around this.
| //! A LIFO string scratch buffer for caching intermediate encoder output. | ||
| //! | ||
| //! Encoders write pieces that repeat across many lines, such as a metric name | ||
| //! or a rendered label set, into a [`ScratchBuf`] once. They then refer to | ||
| //! those pieces by [`Span`] and copy them into the output as often as needed. | ||
| //! The buffer is used as a stack: record an [`Offset`] before pushing and | ||
| //! [`ScratchBuf::rewind`] to it once the pushed pieces are no longer needed. |
There was a problem hiding this comment.
The only caller pushes once and rewinds once per histogram and never nests. What's the stack semantics for?
| #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] | ||
| pub(crate) struct Offset(usize); |
There was a problem hiding this comment.
Is anything comparing Offsets, or can we remove PartialOrd/Ord?
| pub(crate) struct ScratchBuf { | ||
| buf: String, | ||
| /// Staging buffer for [`ScratchBuf::encode_text`]. It is always empty | ||
| /// between calls and only kept around to reuse its allocation. | ||
| staging: String, | ||
| } |
There was a problem hiding this comment.
Could the caller own the staging string, so ScratchBuf doesn't need to know about TextBuf?
| /// Pushes `s` onto the top of the buffer. | ||
| #[inline] | ||
| pub(crate) fn push_str(&mut self, s: &str) { | ||
| self.buf.push_str(s); | ||
| } |
There was a problem hiding this comment.
Apart from tests, this only runs through the fmt::Write impl, and that impl is only used by tests, right? Do we need this?
| /// Borrows the text covered by `span`. | ||
| /// | ||
| /// Returns `None` if the span is out of bounds or does not lie on `char` | ||
| /// boundaries, e.g. because the buffer was rewound below its end. | ||
| #[inline] | ||
| pub(crate) fn get(&self, span: Span) -> Option<&str> { | ||
| self.buf.get(span.start..span.end) | ||
| } |
There was a problem hiding this comment.
When would this return None in a correct program?
| // Encode the const and family labels once and reuse them for every | ||
| // line. | ||
| let start = self.scratch.offset(); | ||
| let result = self | ||
| .encode_cached_labels() | ||
| .and_then(|labels| self.write_histogram_lines(labels, sum, count, buckets, exemplars)); | ||
| self.scratch.rewind(start); | ||
| result |
There was a problem hiding this comment.
Since we never nest, would clear() at the start be enough instead of offset() and rewind()?
| fn encode_cached_labels(&mut self) -> Result<Span, std::fmt::Error> { | ||
| let mut seq = LabelSeqEncoder::new(self.scratch); | ||
| seq.encode(&self.const_labels)?; | ||
| if let Some(labels) = self.family_labels { | ||
| seq.encode(labels)?; | ||
| } | ||
| Ok(seq.finish()) | ||
| } |
There was a problem hiding this comment.
Could this return a borrowed &str, or fill a &mut String the caller passes?
| if let Some(labels) = self.family_labels { | ||
| list.encode(self.writer.reborrow(), labels)?; | ||
| } | ||
| if let Some(labels) = additional_labels { | ||
| list.encode(self.writer.reborrow(), labels)?; | ||
| } | ||
| if let Some(labels) = self.family_labels { | ||
| list.finish(self.writer.reborrow(), "}") |
There was a problem hiding this comment.
This changes OpenMetrics output for Info and histogram le, which will break tests I guess. Can it go in its own commit, or at least the PR title? I agree with the new order, as it matches the Go and Python clients.
| cached: Span, | ||
| additional_labels: Option<&S>, | ||
| ) -> Result<(), std::fmt::Error> { | ||
| let cached = self.scratch.get(cached).ok_or(std::fmt::Error)?; |
There was a problem hiding this comment.
If this get() ever fails, it's our bug, but users only see a fmt::Error. It's also an extra check on every line. Could we pass a &str here so it can't fail?
| /// Append `encoded`, a `k="v",...` sequence previously produced by a | ||
| /// `LabelList::new("")`, e.g. via [`LabelSeqEncoder`]. | ||
| pub(crate) fn encode_raw( | ||
| &mut self, | ||
| mut writer: TextBuf<'_>, | ||
| encoded: &str, | ||
| ) -> Result<(), std::fmt::Error> { |
There was a problem hiding this comment.
Wdyt about a newtype that only the encoder can construct to make abusing this impossible?
Summary
This PR optimizes Histogram encoding for both text formats by encoding cost and family labels into a reusable scratch buffer, reusing the results for encoding each line.
Changes
ScratchBuf(src/encoding/scratch.rs): a reusable string buffer used as a stack.LabelList::encode_raw: splices an already encodedk="v",...sequence into a label list with the same separator and brace handling asencode. An empty sequence is a no-op.ScratchBufperencodecall is passed down throughDescriptorEncoderandMetricEncoder.encode_histogramcaches the const and family labels and writes every line viawrite_labels_raw.{const, family, additional}, matching the Prometheus text encoder.Benchmarks
cargo bench --bench textApple M5 Pro, rustc 1.94.1. Baseline ismasterencodeencode_realistic_string_labelsencode_escaped_label_valuesprometheus_text_encodeprometheus_text_encode_realistic_string_labelsprometheus_text_encode_escaped_label_values