WIP: RingBuffer #7250
WIP: RingBuffer #7250
Conversation
|
CC @dabrahams |
|
Thanks @bitjammer :) |
|
@airspeedswift we should re-evaluate the role of the experimental directory now that Swift is open-source. It served a purpose for us once upon a time, but I'm not sure it does anymore. I may not make sense to accept pull requests on it. |
|
@dabrahams how about This PR does not expose any new public interfaces, it does fix a This PR currently relies on Is it worth continuing to work on this PR? |
|
@therealbnut Thanks for the contribution; I need to consult with others to make a determination. |
|
@dabrahams no problem, thanks for your time |
| return AnySequence([s0, s1].joined()) | ||
| } | ||
| let capacity = Swift.min(maxLength, underestimatedCount) | ||
| var ringBuffer = RingBuffer<Iterator.Element>(capacity: capacity) |
There was a problem hiding this comment.
Seems to me this can't possibly build, since the standard library doesn't define RingBuffer anywhere?
There was a problem hiding this comment.
Yeah, I was intending that it's internal in the stdlib, for now - I've only just moved it over from a separate project so I haven't worked out all the details of integrating it into the stdlib yet. I'm not going to do that until I know it's a welcome contribution though :)
There was a problem hiding this comment.
If the PR is just meant to fix the FIXME, it's definitely welcome! However, you'll have to get it integrated so we can run the tests and benchmarks. And if there's no benchmark that will be affected by your improvement, I'll ask you to please write one ;-)
|
@swift-ci Please smoke test |
|
@swift-ci Please benchmark |
|
@dabrahams this is very much WIP - I expect it's not ready for review, or tests :) Now that I know it's welcome I can do those necessary steps. I'll add "benchmark" to my TODO list. I'm happy to close the PR until it's ready. |
| result.append(ringBuffer[i]) | ||
| ringBuffer[i] = element | ||
| i = ringBuffer.index(after: i) % n | ||
| if ringBuffer.isFull, let first = ringBuffer.first { |
There was a problem hiding this comment.
Can we use popFirst here?
There was a problem hiding this comment.
The ringBuffer.append(element) later will be equivalent to popFirst().
I expect that popFirst is less performant on the current ring buffer implementation. It could use a startIndex and an endIndex to change that.
There was a problem hiding this comment.
I was going to implement it like that, but I needed to investigate existing array-like structures more first. I'd like a structure that's not reallocated, and can have its elements initialised and de-initialized out-of-order as needed. I think _ContiguousArrayBuffer is probably that, but I haven't looked into it enough yet.
There was a problem hiding this comment.
You want ManagedBuffer. Cheers!
|
Should also have a test in validation-test/stdlib that uses StdlibCollectionUnittest |
|
Closing until this is ready for review |
Build comment file:Build failed before running benchmark. |
This introduces an experimental internal collection type,
RingBuffer<T>and resolves rdar://problem/21885650 (presumably).This needs more work, but I thought I'd check it's wanted before I do that.
Remaining work:
validation-test/stdlibthat usesStdlibCollectionUnittestRingBuffershould use_ContiguousArrayBuffer?RingBufferneeds to trap on invalid accessRingBuffershould implement more defaulted methodsArrays.swift.gyb? (behaviour differs probably)RangeReplaceableCollectiononinit()andreserveCapacity(Int)