Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Label Allocation Optimization #395

Open
jwilm opened this issue Oct 2, 2023 · 5 comments
Open

Label Allocation Optimization #395

jwilm opened this issue Oct 2, 2023 · 5 comments
Labels
C-core Component: core functionality such as traits, etc. E-intermediate Effort: intermediate. T-ergonomics Type: ergonomics. T-experiment Type: experiment.

Comments

@jwilm
Copy link

jwilm commented Oct 2, 2023

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
Copy link
Member

tobz commented Oct 3, 2023

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.

@tobz tobz added C-core Component: core functionality such as traits, etc. E-intermediate Effort: intermediate. T-experiment Type: experiment. T-ergonomics Type: ergonomics. labels Oct 3, 2023
@jwilm
Copy link
Author

jwilm commented Oct 4, 2023

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
Copy link
Member

tobz commented Oct 4, 2023

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
Copy link

loyd commented Oct 26, 2023

Related to #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
Copy link
Member

tobz commented Oct 29, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-core Component: core functionality such as traits, etc. E-intermediate Effort: intermediate. T-ergonomics Type: ergonomics. T-experiment Type: experiment.
Projects
None yet
Development

No branches or pull requests

3 participants