Ringbuf remove option<T> #18747
Ringbuf remove option<T> #18747
Conversation
|
|
|
ping @gankro |
|
❤️ ❤️ ❤️ Thanks so much for persisting! Will review/trick-others-into-reviewing ASAP. |
| pub fn reserve_exact(&mut self, additional: uint) { | ||
| // FIXME(Gankro): this is just wrong. The ringbuf won't actually use this space | ||
| self.elts.reserve_exact(additional); | ||
| #[deprecated = "use reserve, Ringbuf can no longer be an exact size."] |
There was a problem hiding this comment.
I think this is the right call, but we need to work out if we want to expose this level of implementation detail in the public APIs (that is, this isn't fundamental to a RingBuf). CC @aturon interested in thoughts. Applies to HashMap as well.
|
Using |
|
Once #17665 is solved, those problems will go away. We should optimise for the language we are aiming to have, not the one we currently have. It is ridiculously easy to manually do #17665 (i.e. |
|
@gankro both of the oom cases should be covered by csherratt@3905a39 |
|
Ah, that it is! (I was going off of clobbered comments) Just need to figure out if we want the debug_asserts, then. I leave you in the strong, comforting, arms of @huonw then. |
|
I added some extra debug_asserts. I was careful in where I placed them to avoid doing the same check twice. The only state that is invalid for the head and tail pointer to be in is if they are greater then or equal to the capacity of ring buffer. Since The other suspect code is the logic in Performance wise the extra checks are not free. The largest changes are in the iteration rate. Before After |
These benchmarks are still useless because they can be optimized away. |
|
Ideally the push and pop benchmarks should handle their inverse with unsafe code so that it doesn't take any time. |
|
@mahkoh that's not a bad idea. I'll give it a try. |
| self.elts[self.lo] = Some(t); | ||
| self.nelts += 1u; | ||
|
|
||
| self.tail = wrap_index(self.tail - 1, self.cap); |
There was a problem hiding this comment.
Most of these function calls can be replaced by
self.tail -= 1;The only place where you might have to wrap indices is in the reallocating function. By removing these operations you get measurably better performance. Furthermore, if you don't wrap indices you can reduce memory usage by up to 2x because head == tail if and only if the container is empty and therefore you don't need an extra space.
There was a problem hiding this comment.
This optimisation can be done in a follow-up PR. Also, what happens here when self.tail = 0?
There was a problem hiding this comment.
The only thing that matters is head - tail.
There was a problem hiding this comment.
The actual number does matter, e.g. the self.buffer_write(tail, t) below is writing t to self.ptr.offset(tail), where self.ptr points to the start of the allocated buffer (so tail better be positive). As a random other example (that's not reallocation), wrapping also matters in get,
fn get(&self, i: uint) -> Option<&T> {
if i < self.len() {
unsafe { Some(&mut *self.ptr.offset((self.tail + i) as int)) }
} else {
None
}
}is incorrect if tail can be "negative".
There was a problem hiding this comment.
You have to wrap when you actually access the memory, that's clear.
There was a problem hiding this comment.
So
The only place where you might have to wrap indices is in the reallocating function
was not correct?
|
I'm happy to r+ this after feedback on #18747 (comment) (everything else can be done later). |
|
@huonw I think I have satisfied the asserts you wanted. |
|
|
||
| #[test] | ||
| fn test_drop() { | ||
| static mut drops: uint = 0; |
There was a problem hiding this comment.
General point: this could be a non-mut static storing an AtomicUint which would avoid the need for unsafety.
There was a problem hiding this comment.
I believe modifying an non-mut static is UB even if it is an Atomic*.
There was a problem hiding this comment.
No, the whole purpose of introducing const was to allow non-unsafe mutation of statics by building on top of the Sync built-in trait and only allowing & references to be taken.
(The compiler allows it without unsafe, which is a pretty good indication that something is not UB.)
-Adds unit tests for fn get() and fn get_mut() which are currently untested -Adds unit tests to verify growth of the ringbuffer when reserve is called. -Adds unit tests to confirm that dropping of items is correct Move ringbuf to use a raw buffer instead of Option<T>
Use is_some() in clear to simplify the clear loop.
|
Can I get a retry? The first build failed because I got unlucky and this got hit by a breaking change #18827 |
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) ```
|
Yessss eat @bors it meeeeerged! 🎉 |
|
@gankro 4th times the charm. |
Fix for task in Metabug #18009 (Rebased version of #18170)
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.
The unit test suite have been expanded, and exposed some bugs in
fn get()andfn get_mut()Benchmark
Before:
After: