Fix #124 Record iteration when recording only zeros #125
Conversation
|
I would have preferred a fix that make the initial value of -- BTW |
|
Thanks for the thorough explanation! I think you're right that |
Fixes HdrHistogram#124 As pointed in the comment in the corresponding issues, we want to ensure we trigger the picker logic at least once to keep the invariant of the loop, that is to say that the picking logic has been executed at least once for last_picked_index, which is not the case for zero when there are only zeros. To do so we use and Option, and initialize to None, to make sure the pick logic is ran at least once. I added a test that collect the record and check for their count. Would it be better to check for the following: ```rust assert_eq!(h.iter_recorded().collect::<Vec<_>>(), vec![IterationValue::<u64>::new(0, 1.0, 1.0, 1, 1)]); ``` It is easy to assume that `h.iter_recorded().collect::<Vec<_>>().len() == h.len()` and simplify the added test, and I'm unsure how to convey that we _really_ want to collect and check the length of all the record.
Oh, yeah, thanks I thought about it, but still sometime forget that structs fields are private by default, and wasn't sure about the performance implications. |
|
Ubuntu beta failed, I can't rerun a single test, so closing and reopening. |
|
Going through my list of opened PRs, is there anything more I can do to help here ? Thanks ! |
|
Released in 7.5.3 🎉 |
In HdrHistogram#125 (comment) a simplification was made without discussion: ```diff - h.iter_recorded().collect::<Vec<_>>().len() + h.iter_recorded().count() ``` This is incorrect as what this test actually tests was that the iterator has the correct length. In a perfect world both should be identical but the PR fix a case where this was not the case as some items would be skipped over. In this I revert the change, add a comment and a second (redundant) check to make sure this is not re-simplified later.
Fixes #124
As pointed in the comment in the corresponding issues, we want to ensure we trigger the picker logic at least once to keep the invariant of the loop, that is to say that the picking logic has been executed for last_picked_index, which is not the case for zero when there are only zeros.
I added a test that collect the record and check for their count.
Would it be better to check for the following:
It is easy to assume that
h.iter_recorded().collect::<Vec<_>>().len() == h.len()and simplify the added test, and I'm unsure how to convey that we really want to collect and check the length of all the record.