Remove use of Option<T> inside of RingBuf #18170
Conversation
|
|
|
Amazing! Thanks so much for doing this! 😍 |
| elts: Vec<Option<T>> | ||
| head: uint, | ||
| tail: uint, | ||
| len: uint, |
There was a problem hiding this comment.
Should this maybe be called cap instead?
|
I've gone over all of the changes now. A great first-draft, I look forward to the next one! |
performance depends on how big the ring buffer grows during the test. This is determined not by the test, but by how many iterations the Bencher chooses to run. This change should make the benchmarks more reliable.
-Require the buffer to be a power of 2, and use that knowledge to allow for cheap index calculations. -Remove the use of a Vec<Option<T>> for storage and move to a unmanaged buffer.
-Add is_full() and use it to check push push_back -use is_empty() for pop/pop_back -read get and make it return Option<&T> -change get_mut to return to Option<&mut T>
|
@gankro updated numbers Before Rework Head/Tail |
|
@csherratt Sorry, what commits are "rework" and "head/tail"? |
|
@gankro rework is what I have now. Which is just the minimum rework to drop |
|
@csherratt Ah, I see. I'm uncertain about the value of this half-measure refactor, considering you had a fully-reviewed patch with much better perf ready to go? Admittedly it is much simpler... |
|
@gankro I just want to get the change into master. I'd rather do smaller iterative changes since they are easier to review and measure their impact on the code base. I will do a follow-up change that will remove some of the needless bounds checking that you suggested as part of the first review. |
|
Would the follow-up be the old set of patches? Because that would basically overwrite all these changes, right? |
|
I was hoping someone would give a strong option on how to best attack this. The two approaches offer a bit of a tradeoff. One allows for the entire buffer to be used, and none-power-of-two buffers. The other allows for faster index calculations. It's a trade off and one that I don't actually know which one is better overall. The other change is still here: If you think we should go forward with the other set of changes, then ok we can do that. If you think the advantages of having flexible buffer sizes is worth it, then we should use this change and back port all the cleanup we did in the other change in a follow-up PR. |
|
I'm also willing to punt this issue to a more experienced developer. I'm never certain what tradeoffs we consider acceptable (this seems to be a moving target). |
|
Personally, I think the tradeoff should be towards speed here. Noone wants a slow ringbuf, even if it "wastes" some space. |
|
I agree with @cmr, a few extra slots is a small price to pay for more speed in this case. |
-Use unsafe for get()/get_mut() -Correct some comment language. -Rearrange the internal functions from external functions
| Some(self.elts[raw_index].as_ref().unwrap()) | ||
| let tail = self.tail; | ||
| self.tail = wrap_index(self.tail + 1, self.ring.len()); | ||
| unsafe { Some(self.ring.unsafe_get(tail)) } |
There was a problem hiding this comment.
N.B.: Iteration could use the branchless pointer wrapping like HashMap does in table::Bucket::next. I'd like to see benchmarks if you choose to do it.
There was a problem hiding this comment.
I gave the technique used in table::Bucket::next a try and was surprised to find it is significantly slower. There could be a flaw in how I implemented it.
Using the wrap_index function returns a result of 634ns, where the technique used in table::Bucket::next return a result of 1082ns.
pub struct MutItems<'a, T:'a> {
headptr: *mut T,
tailptr: *mut T,
tail: uint,
head: uint,
cap: uint,
marker: marker::ContravariantLifetime<'a>,
marker2: marker::NoCopy
}#[inline]
fn next(&mut self) -> Option<&'a mut T> {
if self.headptr == self.tailptr {
return None;
}
let maybe_wraparound_dist = (self.tail ^ (self.tail + 1)) & self.cap;
// Finally, we obtain the offset 1 or the offset -cap + 1.
let dist = 1i - (maybe_wraparound_dist as int);
self.tail += 1;
let ptr = self.tailptr;
unsafe {
self.tailptr = self.tailptr.offset(dist);
Some(&mut *ptr)
}
}
|
Ack! So many pending collections PRs! @csherratt Collections Reform just got approved, so everything's getting renamed and kicked around by like 4 people in parallel. You've already got some merge conflicts, and they're only going to get worse and worse. I'm really sorry this happened! Mostly you'll just have to move trait impls to inherent impls, and tweak names. It might be easier to make the API refactors for DList part of this PR. Does that work for you? |
ghost
closed this
|
I'd rather not mangle the commits. This is not a priority change so I can rebase it after you guys rework everything. |
|
This work should be safe to start up again. RingBuf shouldn't get poked again. Or rather, it should be poked precisely for the changes you're making. |
ghost
mentioned this pull request
| self.elts.reserve(n); | ||
| // +1 since the buffer needs one more space then the expected size. | ||
| let count = num::next_power_of_two(n + 1); | ||
| if count > self.cap { |
There was a problem hiding this comment.
Ah, yep you're right, this isn't what we want. We want
if self.len() + n > self.cap {
let count = num::next_power_of_two(n + 1);
self.grow(count);
}
There was a problem hiding this comment.
No wait,
let new_len = self.len() + n;
if new_len > self.capacity() {
assert!(new_len + 1 <= int::MAX, "capacity overflow"); // eh there's probably an off-by-one here
let count = num::next_power_of_two(new_len + 1);
self.grow(count);
}
There was a problem hiding this comment.
Whoops, I entered the wrong PR 🐫
Fix for task in Metabug #18009 (Rebased version of #18170) This changes much of about how RingBuf functions. `lo`, `nelts` are replaced by a more traditional `head` and`tail`. The `Vec<Option<T>>` is replaced by a bare pointer that is managed by the `RingBuf` itself. This also expects the ring buffer to always be size that is a power of 2. This change also includes a number of new tests to cover the some areas that could be of concern with manual memory management. The benchmarks have been reworked since the old ones were benchmarking of the Ring buffers growth rather then the actual test. The unit test suite have been expanded, and exposed some bugs in `fn get()` and `fn get_mut()` ## Benchmark **Before:** ``` test ring_buf::tests::bench_grow_1025 ... bench: 8919 ns/iter (+/- 87) test ring_buf::tests::bench_iter_1000 ... bench: 924 ns/iter (+/- 28) test ring_buf::tests::bench_mut_iter_1000 ... bench: 918 ns/iter (+/- 6) test ring_buf::tests::bench_new ... bench: 15 ns/iter (+/- 0) test ring_buf::tests::bench_pop_100 ... bench: 294 ns/iter (+/- 9) test ring_buf::tests::bench_pop_front_100 ... bench: 948 ns/iter (+/- 32) test ring_buf::tests::bench_push_back_100 ... bench: 291 ns/iter (+/- 16) test ring_buf::tests::bench_push_front_100 ... bench: 311 ns/iter (+/- 27 ``` **After:** ``` test ring_buf::tests::bench_grow_1025 ... bench: 2209 ns/iter (+/- 169) test ring_buf::tests::bench_iter_1000 ... bench: 534 ns/iter (+/- 27) test ring_buf::tests::bench_mut_iter_1000 ... bench: 515 ns/iter (+/- 28) test ring_buf::tests::bench_new ... bench: 11 ns/iter (+/- 0) test ring_buf::tests::bench_pop_100 ... bench: 170 ns/iter (+/- 5) test ring_buf::tests::bench_pop_front_100 ... bench: 171 ns/iter (+/- 11) test ring_buf::tests::bench_push_back_100 ... bench: 172 ns/iter (+/- 13) test ring_buf::tests::bench_push_front_100 ... bench: 158 ns/iter (+/- 12) ```
…eases, r=lnicola minor: Revert "internal: Disable GitHub releases for now" Turns out, this wasn't needed for long.
Fix for task in Metabug #18009
This changes much of about how RingBuf functions.
lo,neltsare replaced by a more traditionalheadandtail. TheVec<Option<T>>is replaced by a bare pointer that is managed by theRingBufitself. This also expects the ring buffer to always be size that is a power of 2.This change also includes a number of new tests to cover the some areas that could be of concern with manual memory management.
The benchmarks have been reworked since the old ones were benchmarking of the Ring buffers growth rather then the actual test.
Benchmark
Before:
After: