HdrHistogram_rust icon indicating copy to clipboard operation
HdrHistogram_rust copied to clipboard

Feature request: an unrecord() function

Open noguxun opened this issue 1 year 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