CircularBuffer: Add benchmarks and improve performance #641
Conversation
| return ifelse(idx > n, idx - n, idx) | ||
| return idx > n ? idx - n : idx |
There was a problem hiding this comment.
Based on testing, using the ternary operator here rather than the branchless conditional helps the compiler optimize away an allocation when iterating through Iterators.Reverse(cb). This is used for example in foldr.
| @@ -18,6 +18,8 @@ end | |||
|
|
|||
| CircularBuffer(capacity) = CircularBuffer{Any}(capacity) | |||
|
|
|||
| Base.IndexStyle(::Type{<:CircularBuffer}) = IndexLinear() | |||
There was a problem hiding this comment.
IndexLinear (as opposed to the default IndexCartesian for AbstractArrays) allows functions to use an optimized Base._mapreduce. This, for example, speeds up sum.
| Base.@propagate_inbounds function _buffer_index_checked(cb::CircularBuffer, i::Int) | ||
| @boundscheck if i < 1 || i > cb.length | ||
| throw(BoundsError(cb, i)) | ||
| end | ||
| _buffer_index(cb, i) | ||
| end |
There was a problem hiding this comment.
This can be collapsed into _buffer_index, which can be simplified without loss of performance. Callers can use @inbounds to enable or disable bounds checking.
| @inline Base.@propagate_inbounds function Base.getindex(cb::CircularBuffer, i::Int) | ||
| cb.buffer[_buffer_index_checked(cb, i)] | ||
| Base.@propagate_inbounds function Base.getindex(cb::CircularBuffer, i::Int) | ||
| j = _buffer_index(cb, i) | ||
| @inbounds return cb.buffer[j] | ||
| end |
There was a problem hiding this comment.
If i is inbounds for cb, then _buffer_index(cb, i) is guaranteed inbounds for cb.buffer. We can separate these into two lines and annotate the second with @inbounds.
Also, @inline here is redundant because @propagate_inbounds implies @inline.
| @@ -154,25 +154,21 @@ end | |||
|
|
|||
| Return the number of elements currently in the buffer. | |||
| """ | |||
| Base.length(cb::CircularBuffer) = cb.length | |||
|
|
|||
| Base.eltype(::Type{CircularBuffer{T}}) where T = T | |||
There was a problem hiding this comment.
This is already defined for the AbstractArray{T} supertype.
| @@ -154,25 +154,21 @@ end | |||
|
|
|||
| Return the number of elements currently in the buffer. | |||
| """ | |||
| Base.length(cb::CircularBuffer) = cb.length | |||
There was a problem hiding this comment.
Base.length(::AbstractArray) is already a generic function that calls size. For consistency with Julia Base, we can define just size instead.
|
|
||
| """ | ||
| size(cb::CircularBuffer) | ||
|
|
||
| Return a tuple with the size of the buffer. | ||
| """ | ||
| Base.size(cb::CircularBuffer) = (length(cb),) | ||
|
|
||
| Base.convert(::Type{Array}, cb::CircularBuffer{T}) where {T} = T[x for x in cb] |
There was a problem hiding this comment.
This convert was slower than the generic convert.
| Base.@propagate_inbounds function Base.last(cb::CircularBuffer) | ||
| @boundscheck (cb.length == 0) && throw(BoundsError(cb, 1)) | ||
| return cb.buffer[_buffer_index(cb, cb.length)] | ||
| end |
There was a problem hiding this comment.
This was no faster than the generic last function. The situation is different for first because the generic first does not know about cb.first, so the specialized first here is a shortcut. There is no such shortcut for last because we don't track the last index directly.
There was a problem hiding this comment.
Reverted the complete removal and instead replaced with a shorter definition. I was mistaken. Even though benchmarks showed no benefit, it might still be helpful on some architectures or code because the specialized method allows the bounds check to be elided by @inbounds, whereas the generic last does not.
benchmark/bench_circular_buffer.jl
Outdated
| cap = capacity(cb) | ||
| cb.length = cap | ||
| total = 0 | ||
| for _ in 1:cap |
There was a problem hiding this comment.
In this codebase, we use the convention for i = 1:num for iterating over ranges. Also, I know it's trivial, but consider replacing _. It's there in few other places too.
benchmark/bench_circular_buffer.jl
Outdated
| cap = capacity(cb) | ||
| cb.length = cap | ||
| total = 0 | ||
| for _ in 1:cap |
|
Thanks! I've modified lines with range iteration to follow your suggestion. |
|
I just noticed that |
Previous removal was in error because adding this definition allows the boundscheck to be elided by `@inbounds`.
This PR adds a set of benchmarks for
CircularBufferand also improves its performance.Here are some
PkgBenchmarkreports using the benchmarks and comparing the new performance:The benchmark file can be called directly by
PkgBenchmarkusing thescriptkeyword. For example,PkgBenchmark.benchmarkpkg("DataStructures"; script="benchmark/bench_circular_buffer.jl").I've added review comments to explain the rationale behind some of the changes.
There was some inconsistency in the code over whether to use accessor functions (
capacity(cb)) or dot notation (cb.capacity) to read field values. For consistency within the file, I've modified them to use accessor functions. They generate identical code since the functions get inlined.I haven't worked on
append!andfill!yet because improving those is more complicated. I might get to them in the future if this PR goes through.Thanks!