Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add sizehint! for first and make append!/prepend! safer #51903

Merged
merged 1 commit into from
Nov 10, 2023

Conversation

vtjnash
Copy link
Sponsor Member

@vtjnash vtjnash commented Oct 27, 2023

First we add an optional API parameter for sizehint! that controls whether it is for push! (default) or pushfirst!. Secondly, we make the offset zero when using sizehint! to shrink an array from the end, or the trailing size zero when using it to shring from the beginning.

Then we replace the prior implementations of prepend! and append! with ones that are safe even if the iterator changes length during the operation or if convert fails. The result of prepend! may be in an undefined order (because of the reverse! call) in the presence of concurrent modifications or errors, but at least all of the elements will be present and valid afterwards.

Replaces and closes #49905
Replaces and closes #47391
Fixes #15868

Benchmarks show that repeated push! performance (with sizehint) is nearly equivalent to the old append performance:

julia> @benchmark append!(x, 1:1000) setup=x=Vector{Float64}(undef,0)
BenchmarkTools.Trial: 10000 samples with 10 evaluations.
 Range (min … max):  1.027 μs … 72.871 μs  ┊ GC (min … max): 0.00% … 94.57%
 Time  (median):     1.465 μs              ┊ GC (median):    0.00%
 Time  (mean ± σ):   1.663 μs ±  1.832 μs  ┊ GC (mean ± σ):  6.20% ±  5.67%

             ▂▃▅▆█▇▇▆▄▂▁                                      
  ▂▁▁▂▂▂▂▃▄▅▇█████████████▇▆▅▅▅▅▅▅▄▅▄▅▅▅▆▇███▆▅▄▃▃▂▂▂▂▂▂▂▂▂▂ ▄
  1.03 μs        Histogram: frequency by time        2.31 μs <

 Memory estimate: 19.69 KiB, allocs estimate: 0.

julia> @benchmark append!(x, 1:1000) setup=x=Vector{Int}(undef,0)
BenchmarkTools.Trial: 10000 samples with 10 evaluations.
 Range (min … max):  851.900 ns … 76.757 μs  ┊ GC (min … max): 0.00% … 91.59%
 Time  (median):       1.181 μs              ┊ GC (median):    0.00%
 Time  (mean ± σ):     1.543 μs ±  1.972 μs  ┊ GC (mean ± σ):  6.75% ±  5.75%

     ▆█▇▃                                                       
  ▂▃██████▇▅▅▄▅▅▃▂▂▂▃▃▃▂▃▃▃▂▂▂▂▂▁▂▁▂▁▂▂▂▁▁▂▂▁▁▁▁▁▁▁▂▂▂▃▃▃▃▂▂▂▂ ▃
  852 ns          Histogram: frequency by time         4.07 μs <

 Memory estimate: 19.69 KiB, allocs estimate: 0.

base/array.jl Outdated Show resolved Hide resolved
@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Oct 30, 2023

@nanosoldier runbenchmarks("array", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

base/array.jl Outdated Show resolved Hide resolved
First we add an optional API parameter for `sizehint!` that controls
whether it is for `push!` (default) or `pushfirst!`. Secondly, we make
the offset zero when using `sizehint!` to shrink an array from the end,
or the trailing size zero when using it to shring from the beginning.

Then we replace the prior implementations of `prepend!` and `append!`
with ones that are more safe, even if the iterator changes length during
the operation or if convert fails. The result of `prepend!` may be in an
undefined order (because of the `reverse!` call) in the presence of
concurrent modifications or errors, but at least all of the elements
will be present and all entries will be valid afterwards.

Replaces and closes #49905
Replaces and closes #47391
Fixes #15868

Co-authored-by: Sukera <[email protected]>
Co-authored-by: MasonProtter <[email protected]>
@vtjnash vtjnash added the status:merge me PR is reviewed. Merge when all tests are passing label Nov 10, 2023
@vtjnash vtjnash merged commit 9eb2770 into master Nov 10, 2023
8 checks passed
@vtjnash vtjnash deleted the jn/safer_preappend branch November 10, 2023 17:48
@oscardssmith oscardssmith added domain:arrays [a, r, r, a, y, s] and removed status:merge me PR is reviewed. Merge when all tests are passing labels Nov 10, 2023
oscardssmith pushed a commit that referenced this pull request Aug 12, 2024
…55470)

Fix #55459

In Julia 1.10, `push!` and `append!` would be functional for
`AbstractVector` implementations
if `resize!` and `setindex!` were defined.

As of #51903 by @vtjnash as in Julia 1.11.0-rc2, `append!` now depends
on an implementation of
`sizehint!` and `push!`. Since `push!` also depends on `append!`, a
stack
overflow situation can easily be created.

To avoid this, this pull request defines the following

* Add generic versions of `push!(a::AbstractVector, x)` which do not
depend on `append!`
* Add default implementation of `sizehint!` that is a no-op

The implementation of `push!(a::AbstractVector, x)` is a generic version
based on the implementation
of `push!(a::Vector, x)` without depending on internals.

# Example for SimpleArray

Consider the `SimpleArray` example from test/abstractarray.jl:

```julia
mutable struct SimpleArray{T} <: AbstractVector{T}
    els::Vector{T}
end
Base.size(sa::SimpleArray) = size(sa.els)
Base.getindex(sa::SimpleArray, idx...) = getindex(sa.els, idx...)
Base.setindex!(sa::SimpleArray, v, idx...) = setindex!(sa.els, v, idx...)
Base.resize!(sa::SimpleArray, n) = resize!(sa.els, n)
Base.copy(sa::SimpleArray) = SimpleArray(copy(sa.els))
```

Note that `setindex!` and `resize!` are implemented for `SimpleArray`.

## Julia 1.10.4: push! is functional

On Julia 1.10.4, `push!` has a functional implementation for
`SimpleArray`

```julia-repl
julia> push!(SimpleArray{Int}(zeros(Int,5)), 6)
6-element SimpleArray{Int64}:
 0
 0
 0
 0
 0
 6
```

## Julia 1.11.0-rc2 and nightly: push! requires sizehint! and is prone
to stack overflow

Before this pull request, on Julia 1.11.0-rc2 and nightly, `push!` fails
for want of `sizehint!`.

```julia-repl
julia> push!(SimpleArray{Int}(zeros(Int,5)), 6)
ERROR: MethodError: no method matching sizehint!(::SimpleArray{Int64}, ::Int64)
The function `sizehint!` exists, but no method is defined for this combination of argument types.
...
```

After implementing `sizehint!`, `push!` still fails with a stack
overflow.

```julia-repl
julia> Base.sizehint!(a::SimpleArray, x) = a

julia> push!(SimpleArray{Int}(zeros(Int, 5)), 6)
Warning: detected a stack overflow; program state may be corrupted, so further execution might be unreliable.
ERROR: StackOverflowError:
Stacktrace:
      [1] _append!
        @ ./array.jl:1344 [inlined]
      [2] append!
        @ ./array.jl:1335 [inlined]
      [3] push!(a::SimpleArray{Int64}, iter::Int64)
        @ Base ./array.jl:1336
--- the above 3 lines are repeated 79982 more times ---
 [239950] _append!
        @ ./array.jl:1344 [inlined]
 [239951] append!
        @ ./array.jl:1335 [inlined]
```

This is because the new implementation of `append!` depends on `push!`.

## After this pull request, push! is functional.

After this pull request, there is a functional `push!` for `SimpleArray`
again as in Julia 1.10.4:

```julia-repl
julia> push!(SimpleArray{Int}(zeros(Int, 5), 6)
6-element SimpleArray{Int64}:
 0
 0
 0
 0
 0
 6
```
KristofferC pushed a commit that referenced this pull request Aug 13, 2024
…55470)

Fix #55459

In Julia 1.10, `push!` and `append!` would be functional for
`AbstractVector` implementations
if `resize!` and `setindex!` were defined.

As of #51903 by @vtjnash as in Julia 1.11.0-rc2, `append!` now depends
on an implementation of
`sizehint!` and `push!`. Since `push!` also depends on `append!`, a
stack
overflow situation can easily be created.

To avoid this, this pull request defines the following

* Add generic versions of `push!(a::AbstractVector, x)` which do not
depend on `append!`
* Add default implementation of `sizehint!` that is a no-op

The implementation of `push!(a::AbstractVector, x)` is a generic version
based on the implementation
of `push!(a::Vector, x)` without depending on internals.

# Example for SimpleArray

Consider the `SimpleArray` example from test/abstractarray.jl:

```julia
mutable struct SimpleArray{T} <: AbstractVector{T}
    els::Vector{T}
end
Base.size(sa::SimpleArray) = size(sa.els)
Base.getindex(sa::SimpleArray, idx...) = getindex(sa.els, idx...)
Base.setindex!(sa::SimpleArray, v, idx...) = setindex!(sa.els, v, idx...)
Base.resize!(sa::SimpleArray, n) = resize!(sa.els, n)
Base.copy(sa::SimpleArray) = SimpleArray(copy(sa.els))
```

Note that `setindex!` and `resize!` are implemented for `SimpleArray`.

## Julia 1.10.4: push! is functional

On Julia 1.10.4, `push!` has a functional implementation for
`SimpleArray`

```julia-repl
julia> push!(SimpleArray{Int}(zeros(Int,5)), 6)
6-element SimpleArray{Int64}:
 0
 0
 0
 0
 0
 6
```

## Julia 1.11.0-rc2 and nightly: push! requires sizehint! and is prone
to stack overflow

Before this pull request, on Julia 1.11.0-rc2 and nightly, `push!` fails
for want of `sizehint!`.

```julia-repl
julia> push!(SimpleArray{Int}(zeros(Int,5)), 6)
ERROR: MethodError: no method matching sizehint!(::SimpleArray{Int64}, ::Int64)
The function `sizehint!` exists, but no method is defined for this combination of argument types.
...
```

After implementing `sizehint!`, `push!` still fails with a stack
overflow.

```julia-repl
julia> Base.sizehint!(a::SimpleArray, x) = a

julia> push!(SimpleArray{Int}(zeros(Int, 5)), 6)
Warning: detected a stack overflow; program state may be corrupted, so further execution might be unreliable.
ERROR: StackOverflowError:
Stacktrace:
      [1] _append!
        @ ./array.jl:1344 [inlined]
      [2] append!
        @ ./array.jl:1335 [inlined]
      [3] push!(a::SimpleArray{Int64}, iter::Int64)
        @ Base ./array.jl:1336
--- the above 3 lines are repeated 79982 more times ---
 [239950] _append!
        @ ./array.jl:1344 [inlined]
 [239951] append!
        @ ./array.jl:1335 [inlined]
```

This is because the new implementation of `append!` depends on `push!`.

## After this pull request, push! is functional.

After this pull request, there is a functional `push!` for `SimpleArray`
again as in Julia 1.10.4:

```julia-repl
julia> push!(SimpleArray{Int}(zeros(Int, 5), 6)
6-element SimpleArray{Int64}:
 0
 0
 0
 0
 0
 6
```

(cherry picked from commit cf4c30a)
lazarusA pushed a commit to lazarusA/julia that referenced this pull request Aug 17, 2024
…uliaLang#55470)

Fix JuliaLang#55459

In Julia 1.10, `push!` and `append!` would be functional for
`AbstractVector` implementations
if `resize!` and `setindex!` were defined.

As of JuliaLang#51903 by @vtjnash as in Julia 1.11.0-rc2, `append!` now depends
on an implementation of
`sizehint!` and `push!`. Since `push!` also depends on `append!`, a
stack
overflow situation can easily be created.

To avoid this, this pull request defines the following

* Add generic versions of `push!(a::AbstractVector, x)` which do not
depend on `append!`
* Add default implementation of `sizehint!` that is a no-op

The implementation of `push!(a::AbstractVector, x)` is a generic version
based on the implementation
of `push!(a::Vector, x)` without depending on internals.

# Example for SimpleArray

Consider the `SimpleArray` example from test/abstractarray.jl:

```julia
mutable struct SimpleArray{T} <: AbstractVector{T}
    els::Vector{T}
end
Base.size(sa::SimpleArray) = size(sa.els)
Base.getindex(sa::SimpleArray, idx...) = getindex(sa.els, idx...)
Base.setindex!(sa::SimpleArray, v, idx...) = setindex!(sa.els, v, idx...)
Base.resize!(sa::SimpleArray, n) = resize!(sa.els, n)
Base.copy(sa::SimpleArray) = SimpleArray(copy(sa.els))
```

Note that `setindex!` and `resize!` are implemented for `SimpleArray`.

## Julia 1.10.4: push! is functional

On Julia 1.10.4, `push!` has a functional implementation for
`SimpleArray`

```julia-repl
julia> push!(SimpleArray{Int}(zeros(Int,5)), 6)
6-element SimpleArray{Int64}:
 0
 0
 0
 0
 0
 6
```

## Julia 1.11.0-rc2 and nightly: push! requires sizehint! and is prone
to stack overflow

Before this pull request, on Julia 1.11.0-rc2 and nightly, `push!` fails
for want of `sizehint!`.

```julia-repl
julia> push!(SimpleArray{Int}(zeros(Int,5)), 6)
ERROR: MethodError: no method matching sizehint!(::SimpleArray{Int64}, ::Int64)
The function `sizehint!` exists, but no method is defined for this combination of argument types.
...
```

After implementing `sizehint!`, `push!` still fails with a stack
overflow.

```julia-repl
julia> Base.sizehint!(a::SimpleArray, x) = a

julia> push!(SimpleArray{Int}(zeros(Int, 5)), 6)
Warning: detected a stack overflow; program state may be corrupted, so further execution might be unreliable.
ERROR: StackOverflowError:
Stacktrace:
      [1] _append!
        @ ./array.jl:1344 [inlined]
      [2] append!
        @ ./array.jl:1335 [inlined]
      [3] push!(a::SimpleArray{Int64}, iter::Int64)
        @ Base ./array.jl:1336
--- the above 3 lines are repeated 79982 more times ---
 [239950] _append!
        @ ./array.jl:1344 [inlined]
 [239951] append!
        @ ./array.jl:1335 [inlined]
```

This is because the new implementation of `append!` depends on `push!`.

## After this pull request, push! is functional.

After this pull request, there is a functional `push!` for `SimpleArray`
again as in Julia 1.10.4:

```julia-repl
julia> push!(SimpleArray{Int}(zeros(Int, 5), 6)
6-element SimpleArray{Int64}:
 0
 0
 0
 0
 0
 6
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays [a, r, r, a, y, s]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

append!(Int64[], [1.1, 0.1]) results in unwanted array growth
5 participants