HdrHistogram_rust icon indicating copy to clipboard operation
HdrHistogram_rust copied to clipboard

Feature request: an unrecord() function

Open noguxun opened this issue 2 years ago • 3 comments

Can we implemented an interface to remove what we have Histogram::record() ? Like this decrement interface in another histgram crate https://docs.rs/histogram/0.6.9/histogram/struct.Histogram.html#method.decrement

noguxun avatar Sep 12 '22 08:09 noguxun

Ah, interesting, yeah, we don't get that "for free" like the Go version does because they use signed integers for the counts: https://github.com/HdrHistogram/hdrhistogram-go/blob/494271c4c016b36c8cee88480288f33b419cf7b0/hdr.go#L306-L309

I think the best way to go about this would be to add a sub: bool argument to record_n_inner here: https://github.com/HdrHistogram/HdrHistogram_rust/blob/7c4a5fa1771d361aa728fc2f8d4d519e8ce11ef8/src/lib.rs#L868

And if it's true do saturating_sub instead of saturating_add in the three places that's called in that function. Then, add two interface methods unrecord(value: u64) and unrecord_n(value: u64, n: T) that call record_n_inner with the arg being set to true.

I'd be curious on the impact of this change on the benchmarks. It could be that it's worth marking record_n_inner as #[inline] so that record can remain optimized for the addition case without the extra branch, given how it's often used in hot loops. @marshallpierce may also have thoughts about this.

jonhoo avatar Sep 17 '22 22:09 jonhoo

This feature is important to me... is there willingness to integrate it in if I PR this feature? - I should be able to figure it out based off of @jonhoo 's description.

The only alternative I can really currently see (if I'd like to use this library and I do!!) is to create a new histogram with the entries I desire to remove then to subtract it from the original histogram... a cumbersome and seemingly inefficient way to do it.

bogzbonny avatar Apr 05 '23 20:04 bogzbonny

If you have time to write up a PR, then yes, I'd love to take a look!

jonhoo avatar Apr 08 '23 17:04 jonhoo