Add last / prepend to CircularBuffer #347

normanmaurer · 2018-04-23T10:30:02Z

Motivation:

Its often useful to be able to add / retrieve elements on both sides of a CircularBuffer.

Modifications:

Add last / prepend to be able to act on both ends.
Add tests

Result:

Fixes #279

normanmaurer · 2018-04-23T10:30:18Z

FYI @helje5

helje5 · 2018-04-23T10:34:58Z

Very nice! 👯 Should that also do the power-of-2 buffer size increases instead of doubling? For Redi/S to be fast, I had to do manual capacity increases.

helje5 · 2018-04-23T10:36:58Z

Oh, I just noticed you already did that in #313 .

Lukasa · 2018-04-23T10:45:13Z

Sources/NIO/CircularBuffer.swift

-        } else {
-            return self.buffer[self.headIdx]
-        }
+        // As self.buffer is filled with `nil` this will correctly return `nil` if the CircularBuffer is empty.


Is this change worth it? The path of checking isEmpty is going to be faster, because it saves us the additional level of indirection into the buffer.

@Lukasa not sure I think it is a nice change but I am not strong on it...

@weissi for the tie-break!

feels too clever to me, I'd leave the old code

@weissi achievement unlocked... @weissi called me clever =P

Let me revert this part

normanmaurer · 2018-04-23T11:50:42Z

@weissi @Lukasa PTAL again

weissi

looks really good but I think the naming of the functions should be improved. Also paged the stdlib guys so they can help us out here.

weissi · 2018-04-23T15:33:06Z

Sources/NIO/CircularBuffer.swift

+    /// Add an element to the end of the ring buffer.
+    ///
+    /// Amortized *O(1)*
+    public mutating func addLast(_ value: E) {


why do we have this method? We have append already which matches Array's append.

@weissi I added it for consistent naming ;)

weissi · 2018-04-23T15:36:33Z

Sources/NIO/CircularBuffer.swift

+    /// Add an element to the front of the ring buffer.
+    ///
+    /// Amortized *O(1)*
+    public mutating func addFirst(_ value: E) {


Swift doesn't really seem to use add.... I would suggest prepend maybe as it's the opposite of append? CC @lorentey, @moiseev, @airspeedswift

also: what protocols should this auto expanding ring buffer implement?

Appending to the front gets us to BidirectionalCollection. Now that we're at that, we can actually hop the next level up to RandomAccessCollection. Given the way this behaves, we could also give it RangeReplaceableCollection too, which is a pretty easy cousin of RandomAccessCollection.

So I'd recommend adding conformances to those two, though we may want to do them in a different PR.

@Lukasa isn't RangeReplaceableCollection a thing where you can replace any range by any other range? So for example put 5 elements in a space where there used to be 3 ?

Like thing[1...3] = ["foo", "bar", "buz", "qux", "quux"] ?

Yeah, and our buffer can do that. It's no less efficient at it than a regular array.

I guess that (startIndex=0) wouldn't matter much. (though Redi/S lists also have that getrange-family of operations, but that could be mapped in the frontend).

The thing I'm wondering is whether there should also be a prepend(contentsOf:) which reserves the proper capacity.

From what I can see, CircularBuffer does not have even an append(contentsOf:) right now.

I had this idea for a moment, but then I quickly hit a wall trying to name prepend(contentsOf:) correctly and specify the behavior when tailIdx reaches the headIdx, so I decided not to =)

Going back to indices. FWIW, in the collection types provided by the standard library there are no guarantees about the values of indices, so one should always use startIndex/endIndex combo and index family of methods, which might be annoying, but is the only guaranteed way of writing generic code over collections, even when Index: FixedWidthInteger or Index == Int.

@moiseev Many thanks! I'm not personally attached to the 0 based indexing and I think we can change it as SemVer patch I think. What do @normanmaurer & @Lukasa think?

and obvs I'm fine with prepend

@weissi sounds good to me... That said I think we can pull in the PR as it is now... WDYT ?

weissi · 2018-04-23T15:38:11Z

Sources/NIO/CircularBuffer.swift

        let value = self.buffer[self.headIdx]
-        self.buffer[headIdx] = nil
+
+        precondition(value != nil, "CircularBuffer is empty")


this should be the first instruction I think, just check isEmpty

@weissi sure I can do this but I would argue that this way we it may be a bit faster...

If you want a speedup, the better thing to do is to do this:

guard let value = self.buffer[headIdx] else { preconditionFailure("Circular buffer is empty") } self.buffer[headIdx] = nil self.headIdx = (self.headIdx + 1) & self.mask return value

This checks the optional only once, not twice.

fine with me as well...(would use fatalError tho) @weissi ?

Why use fatalError?

@Lukasa actually you are right preconditionFailure may be more correct.

happy with either

weissi · 2018-04-23T15:38:18Z

Sources/NIO/CircularBuffer.swift

+        let idx = (self.tailIdx - 1) & self.mask
+        let value = self.buffer[idx]
+
+        precondition(value != nil, "CircularBuffer is empty")


should be first

See #347 (comment)

Lukasa · 2018-04-24T07:19:49Z

Sources/NIO/CircularBuffer.swift

+    public mutating func removeLast() -> E {
+        let idx = (self.tailIdx - 1) & self.mask
+        guard let value = self.buffer[idx] else {
+            fatalError("CircularBuffer is empty")


I think preconditionFailure here is fine, right @weissi?

yep I think you are right...

sure, happy with either

weissi

🙌 thanks, looks great

Motivation: Its often useful to be able to add / retrieve elements on both sides of a CircularBuffer. Modifications: - Add last / prepend to be able to act on both ends. - Add tests Result: Fixes apple#279

normanmaurer · 2018-04-24T08:19:27Z

Will merge one the CI completes

normanmaurer · 2018-04-24T08:27:27Z

Created #350 as a followup.

Motivation: During the `PR` apple#347 there was a discussion around which protocols this autoexpanding buffer should conform; the following 3 being the winners: 1. BidirectionalCollection 2. RandomAccessCollection 3. RangeReplaceableCollection Modifications: Added conformance to the 3 protocolos and provided implementaiton for 4 methods: 1. replaceSubrange(:with:) 2. index(before:) 3. removeSubrange(:) 4. remove(at:) 5. removeLast(:) Result: With the conformance of this 3 protocols and the implementation of this 5 functions we get dropLast(), dropLast(:), removeFirst(), removeLast() & popLast() for free.

Motivation: During the `PR` #347 there was a discussion around which protocols this autoexpanding buffer should conform; the following 3 being the winners: 1. BidirectionalCollection 2. RandomAccessCollection 3. RangeReplaceableCollection Modifications: Added conformance to the 3 protocolos and provided implementaiton for 4 methods: 1. replaceSubrange(:with:) 2. index(before:) 3. removeSubrange(:) 4. remove(at:) 5. removeLast(:) Result: With the conformance of this 3 protocols and the implementation of this 5 functions we get dropLast(), dropLast(:), removeFirst(), removeLast() & popLast() for free.

normanmaurer requested review from Lukasa and weissi and removed request for Lukasa April 23, 2018 10:30

Lukasa reviewed Apr 23, 2018

View reviewed changes

normanmaurer force-pushed the deque branch from be447f9 to 1985284 Compare April 23, 2018 11:50

Lukasa approved these changes Apr 23, 2018

View reviewed changes

Lukasa added the 🆕 semver/minor Adds new public API. label Apr 23, 2018

Lukasa added this to the 1.6.0 milestone Apr 23, 2018

weissi requested changes Apr 23, 2018

View reviewed changes

Lukasa reviewed Apr 24, 2018

View reviewed changes

normanmaurer force-pushed the deque branch from 4bca2cd to 2b8a195 Compare April 24, 2018 07:22

weissi approved these changes Apr 24, 2018

View reviewed changes

Add last / prepend to CircularBuffer

26c2345

Motivation: Its often useful to be able to add / retrieve elements on both sides of a CircularBuffer. Modifications: - Add last / prepend to be able to act on both ends. - Add tests Result: Fixes apple#279

normanmaurer force-pushed the deque branch from 2b8a195 to 26c2345 Compare April 24, 2018 08:11

normanmaurer changed the title ~~Add last / addFirst to CircularBuffer~~ Add last / prepend to CircularBuffer Apr 24, 2018

normanmaurer merged commit 976074d into apple:master Apr 24, 2018

normanmaurer deleted the deque branch April 24, 2018 08:26

normanmaurer mentioned this pull request Apr 24, 2018

CircularBuffer should conform to more protocols #350

Closed

esttorhe mentioned this pull request Jun 4, 2018

Make CircularBuffer conform to more protocols [Fixes #350] #466

Merged

Add last / prepend to CircularBuffer #347

Add last / prepend to CircularBuffer #347

Conversation

normanmaurer commented Apr 23, 2018 • edited Loading

normanmaurer commented Apr 23, 2018

helje5 commented Apr 23, 2018

helje5 commented Apr 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weissi Apr 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

normanmaurer commented Apr 23, 2018

weissi left a 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

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

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

weissi left a comment

Choose a reason for hiding this comment

normanmaurer commented Apr 24, 2018

normanmaurer commented Apr 24, 2018

normanmaurer commented Apr 23, 2018 •

edited

Loading

weissi Apr 23, 2018 •

edited

Loading