Implement insert for RingBuf #19569

murphm8 · 2014-12-05T16:04:28Z

This is a first pass at insert on RingBuf. I tried to keep it as simple as possible. I'm not sure of the performance implications of doing one copy vs. copying multiple times but moving a smaller amount of memory. I chose to stick with one copy, even if the amount of memory I have to move is larger.

I believe this is part of #18424

@gankro mentioned this was missing.

Gankra · 2014-12-05T16:15:22Z

src/libcollections/ring_buf.rs

+    /// assert_eq!(Some(&11), buf.get(1));
+    /// ```
+    pub fn insert(&mut self, i: uint, t: T) {
+        assert!(i <= self.len());


This assert should have a message. e.g. assert!(whatever, "index out of bounds");

Gankra · 2014-12-05T16:16:59Z

src/libcollections/ring_buf.rs

+        match i {
+            0 => self.push_front(t),
+            _ => {
+                if i == self.len() {


You can replace the parent block with this if/else. e.g.

_ => if ... { ... } else { ... }

Also actually maybe just don't have the match at all? just an if/else-if/else?

I'm also not clear if the extra branches are worth it at all in the general case.

murphm8 · 2014-12-05T19:19:39Z

I added documentation. Do you think this algorithm is good enough? The worst case could be pretty bad. If it would be better to try to copy the smallest number of elements, even if it takes multiple copies, I can code that up.

Gankra · 2014-12-05T19:27:08Z

Sorry I've been in and out all day, was randomly scanning this on my phone for the most part. I now have a couple hours free, so I can actually review the real logic.

Gankra · 2014-12-05T19:34:03Z

src/libcollections/ring_buf.rs

+        //   [o A o o o o o o o o o o . . . . ]
+        //    T                         H
+        // 3 [o I A o o o o o o o o o o . . . ]
+        //        B        =          E


Yeah I don't think this will be acceptable. It would be very surprising if inserting at index 1 was this expensive in this data structure. Especially in such a "random" way from the user's perspective. It's especially bad because this cost isn't at all amortized, and pretty easy to trigger. That is, RingBuf::new(); push_back(1); insert(1,1); insert(1,1); ... insert(1,1); will take O(n^2) time.

There might be a magic ratio here. Something like the double copy is worth it if the single copy would move twice as much, but I dunno. For now I'd just pick the minimal, and we can dig into optimization later (unless you're into that sort of thing and want to nail it now).

murphm8 · 2014-12-06T22:48:41Z

The algorithm should have a worst case of moving len/2 - 1 elements. I did a bunch of debug printing to make sure my test hits all of the branches. I am going to look at this again later tonight to see if there's anywhere that I can make the code more concise.

Gankra · 2014-12-07T00:48:56Z

Awesome! I will try to review this in a couple days when I have the time. I'm super paranoid about collections code though, so I'd appreciate getting a second pair of eyes from @huonw @cgaebel or @csherratt on this.

ghost · 2014-12-07T04:44:34Z

src/libcollections/ring_buf.rs

+        let contiguous = self.tail < self.head;
+
+        if contiguous { // easy stuff
+            let (src, dst, count) = if distance_to_tail < distance_to_head {


I think you have missed an edge case here. There is a case where adding to the ring buffer breaks the buffer into none-contiguous memory. In that case a single memory copy is not sufficient.

Yep, you're correct.

In the case that head moves from being contiguous to non-contiguous, it will always only require one copy because where head is pointing does not contain an element.

In the case of tail moving from contiguous to non-contiguous, if the insert is not at 0 in the ringbuf then it should require two copies.

H I T [. . . . . o o o o o o o A o o o] T H [o . . . . o o o o o o o I A o o] M M M M

I will add a test case for this, and add the corner case.

murphm8 · 2014-12-07T15:51:14Z

I included commit e00a63d which has all of the prints I used when making sure I hit the right coverage. That should make it easier to review.

Gankra · 2014-12-08T16:56:59Z

src/libcollections/ring_buf.rs

+        let distance_to_head = self.len() - i;
+
+        // If the buffer is contiguous and the distance to head is less than the distance to
+        // tail then move head, and do one copy.


I would add a comma or colon after tail

cgaebel · 2014-12-08T21:13:25Z

src/libcollections/ring_buf.rs

@@ -649,6 +649,189 @@ impl<T> RingBuf<T> {
            unsafe { Some(self.buffer_read(head)) }
        }
    }
+
+    /// Inserts an element at position `i` within the ringbuf, shifting all
+    /// elements after position `i` one position to the right.


This is no longer true.

Well, it kinda is, but the subtlety should be documented.

I'm not sure what the subtlety is. To the user, it appears as if the element was inserted at 'i' (the index they passed in), and the element that was at 'i' is now at 'i+1'

The fact that elements to the left may be shifted could be surprising. There's no hint to the implementation's unique efficiency in the docs.

Gankra · 2014-12-09T21:37:49Z

This draft is so much nicer to read! \o/

cgaebel · 2014-12-09T21:56:47Z

src/libcollections/ring_buf.rs

+
+        let contiguous = self.tail < self.head;
+
+        if contiguous {


Instead of this nested if-else structure, is it possible to use a single match on a triple? That might help decrease the indentation here, and make it look really nice when you split up the docs case-by-case.

An if-elseif-else would be fine, no?

Yeah probably. The nesting is just a little overkill for what's happening.

Ohh sorry I get what you mean, now. Yeah that might be cool.

Although this design makes the case "nesting" more clear.

But they're not really nested. The outer and inner conditions could be inverted and there'd be no loss of readability.

Fair enough. Case... grouping?

Well matching on something like (contiguous, in_tail_half, distance_to_head < distance_to_tail) seems like it'd go a long way.

I'm planning on doing what @cgaebel suggested

cgaebel · 2014-12-09T21:57:12Z

Yeah it looks a LOT nicer now. I'm done my pass.

Gankra · 2014-12-09T23:59:27Z

One last thing: the comment about perf guarantee seems to have been lost as some point. Should specify that this does at most O(min(n, n - i)) work.

murphm8 · 2014-12-10T22:52:43Z

@gankro I think I covered everything. Not sure on the wording of the doc statement. Do you have some more comments?

Gankra · 2014-12-10T23:43:58Z

src/libcollections/ring_buf.rs

+                //      [o A o o o o o . . . . . . o o o]
+                //
+                //                     H         T
+                //      [I A o o o o o . . . . . o o o o]


Diagram still deceptive about work done.

Gankra · 2014-12-12T03:03:46Z

Awesome! I think this is good to go now! Just need to squash the commits into one and we can merge it. :D

cgaebel · 2014-12-12T03:19:23Z

Aww man that's some pretty code. Nicely done!

Gankra · 2014-12-12T03:34:51Z

Thanks a lot for persevering through our brutal review! 😄

Interested in implementing remove and shrink_to_fit? 😈

This is a first pass at insert on RingBuf. I tried to keep it as simple as possible. I'm not sure of the performance implications of doing one copy vs. copying multiple times but moving a smaller amount of memory. I chose to stick with one copy, even if the amount of memory I have to move is larger. I believe this is part of #18424 @gankro mentioned this was missing.

This is a first pass at insert on RingBuf. I tried to keep it as simple as possible. I'm not sure of the performance implications of doing one copy vs. copying multiple times but moving a smaller amount of memory. I chose to stick with one copy, even if the amount of memory I have to move is larger. I believe this is part of rust-lang#18424 @gankro mentioned this was missing.

murphm8 force-pushed the ring_buf_insert branch from e20a325 to 5932ac5 Compare December 5, 2014 16:13

Gankra reviewed Dec 5, 2014
View reviewed changes

murphm8 force-pushed the ring_buf_insert branch from 5932ac5 to e3e6e9a Compare December 5, 2014 16:15

Gankra reviewed Dec 5, 2014
View reviewed changes

ghost reviewed Dec 7, 2014
View reviewed changes

Gankra reviewed Dec 8, 2014
View reviewed changes

cgaebel reviewed Dec 8, 2014
View reviewed changes

cgaebel reviewed Dec 9, 2014
View reviewed changes

Gankra reviewed Dec 10, 2014
View reviewed changes

Implement insert for RingBuf

40f28c7

murphm8 force-pushed the ring_buf_insert branch from e490716 to 40f28c7 Compare December 12, 2014 03:15

bors closed this Dec 12, 2014

bors merged commit 40f28c7 into rust-lang:master Dec 12, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement insert for RingBuf #19569

Implement insert for RingBuf #19569

murphm8 commented Dec 5, 2014

Gankra Dec 5, 2014

Gankra Dec 5, 2014

Gankra Dec 5, 2014

murphm8 commented Dec 5, 2014

Gankra commented Dec 5, 2014

Gankra Dec 5, 2014

Gankra Dec 5, 2014

murphm8 commented Dec 6, 2014

Gankra commented Dec 7, 2014

ghost Dec 7, 2014

murphm8 Dec 7, 2014

murphm8 commented Dec 7, 2014

Gankra Dec 8, 2014

cgaebel Dec 8, 2014

murphm8 Dec 9, 2014

cgaebel Dec 9, 2014

Gankra commented Dec 9, 2014

cgaebel Dec 9, 2014

Gankra Dec 9, 2014

cgaebel Dec 9, 2014

Gankra Dec 9, 2014

Gankra Dec 9, 2014

cgaebel Dec 9, 2014

Gankra Dec 9, 2014

cgaebel Dec 9, 2014

murphm8 Dec 9, 2014

cgaebel commented Dec 9, 2014

Gankra commented Dec 9, 2014

murphm8 commented Dec 10, 2014

Gankra Dec 10, 2014

Gankra commented Dec 12, 2014

cgaebel commented Dec 12, 2014

Gankra commented Dec 12, 2014

Implement insert for RingBuf #19569

Implement insert for RingBuf #19569

Conversation

murphm8 commented Dec 5, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murphm8 commented Dec 5, 2014

Gankra commented Dec 5, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murphm8 commented Dec 6, 2014

Gankra commented Dec 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murphm8 commented Dec 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gankra commented Dec 9, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgaebel commented Dec 9, 2014

Gankra commented Dec 9, 2014

murphm8 commented Dec 10, 2014

Choose a reason for hiding this comment

Gankra commented Dec 12, 2014

cgaebel commented Dec 12, 2014

Gankra commented Dec 12, 2014