HdrHistogram_rust
HdrHistogram_rust copied to clipboard
Feature request: an unrecord() function
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
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.
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.
If you have time to write up a PR, then yes, I'd love to take a look!