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

Feature request: an unrecord() function #114

Open
noguxun opened this issue Sep 12, 2022 · 3 comments
Open

Feature request: an unrecord() function #114

noguxun opened this issue Sep 12, 2022 · 3 comments

Comments

@noguxun
Copy link

noguxun commented Sep 12, 2022

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

@jonhoo
Copy link
Collaborator

jonhoo commented Sep 17, 2022

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:

fn record_n_inner(&mut self, mut value: u64, count: T, clamp: bool) -> Result<(), RecordError> {

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.

@bogzbonny
Copy link

bogzbonny commented Apr 5, 2023

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.

@jonhoo
Copy link
Collaborator

jonhoo commented Apr 8, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants