metrics-rs / metrics

A metrics ecosystem for Rust.
MIT License
1.08k stars 148 forks source link

Label Allocation Optimization #395

Open jwilm opened 11 months ago

jwilm commented 11 months ago

When using dynamic labels, it's necessary to either allocate a string each time or "forget" it to obtain a &'static str reference. The latter can in some cases (esp a naive solution) result in unbound memory growth, and the former results in a bunch of wasted time dealing with allocating and dropping Strings. For hot code paths, this can result in a lot of wasted CPU and degrade application performance.

A simple solution to this might be supporting Arc for labels, and a more sophisticated version might be some sort of label value registration / string interning managed by the metrics crate.

Not sure if this is a feature request exactly, but it's something I keep bumping into when working with the metrics crate, and I wanted to at least start a discussion about it. Is this something you've thought about at all? Is it a problem you're interested in addressing internally, or would you prefer users of the crate to handle it themselves?

Thanks for your time (and for the great package!)

tobz commented 11 months ago

This is definitely something I've thought about and even have an old, stale branch for trying to handle: https://github.com/metrics-rs/metrics/tree/cow-experiments

Essentially, I envisioned the copy-on-write type that we use to handle either borrowed, owned, or shared (Arc<T>) versions of the given type, as well as an optimization for small strings that could otherwise be inlined into the copy-on-write structure.

I just never got as far as I wanted with it: lack of time/motivation, more or less. Dropping the bit for doing small string optimization would make it trivial to include support for shared pointers, and could be a good incremental step to revive the effort.

jwilm commented 11 months ago

Oh that sounds like it would solve our problem! Sounds like a nice solution.

Re: small string optimization; are you saying you'd drop support for it?

tobz commented 11 months ago

I would only drop the requirement of getting it out at the same time as support for smart pointers, for the purpose of trying to incrementalize improve things rather than doing it in one fell swoop.

loyd commented 10 months ago

Related to https://github.com/metrics-rs/metrics/issues/273

Another problem with dynamic labels is Box::new:

let a = "some_value";
metrics::gauge!("some_metric", 42., "label" => a);

is expanded into

  static METRIC_NAME: &'static str = "some_metric";
  if let Some(recorder) = ::metrics::try_recorder() {
      let key = ::metrics::Key::from_parts(
          METRIC_NAME,
          (<[_]>::into_vec(
              #[rustc_box]
              $crate::boxed::Box::new([(::metrics::Label::new("label", a))]),
          )),
      );
      let handle = recorder.register_gauge(&key);
      handle.set(42.);
  }

It could be avoided by separating metadata (static usually) and values in the same way as the tracing crate does it.

tobz commented 10 months ago

Taking the approach that tracing takes would involve a lot more work, because it would completely change how we work with Key... but this is very different from basic problem above of not supporting Arc<str>.

I opened #405 as a placeholder to write down some thoughts on what your idea might look like.