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

50x regression in copying large custom arrays. #52070

Closed
LilithHafner opened this issue Nov 7, 2023 · 5 comments
Closed

50x regression in copying large custom arrays. #52070

LilithHafner opened this issue Nov 7, 2023 · 5 comments
Labels
domain:arrays [a, r, r, a, y, s] kind:potential benchmark Could make a good benchmark in BaseBenchmarks kind:regression Regression in behavior compared to a previous version performance Must go faster

Comments

@LilithHafner
Copy link
Member

using Random, BenchmarkTools

struct MyArray{T, N} <: AbstractArray{T, N}
    data::Array{T, N}
end
Base.size(a::MyArray) = size(a.data)
Base.getindex(a::MyArray, i...) = getindex(a.data, i...)
Base.setindex!(a::MyArray, v, i...) = setindex!(a.data, v, i...)
Base.similar(a::MyArray) = MyArray(similar(a.data))
Base.IndexStyle(::Type{<:MyArray}) = IndexLinear()

x = MyArray(rand(40_000));
@btime copy($x);
#  13.084 μs (2 allocations: 312.55 KiB)  # 1.9
#  13.291 μs (2 allocations: 312.55 KiB)  # 1.10
#  459.667 μs (2 allocations: 312.55 KiB) # 58030da3bc4e6790d7bafe66d5f37b382dd6df3c (right before #50824)
#  813.792 μs (3 allocations: 312.56 KiB) # 140ea94f8e (current master)

versioninfo()
# Julia Version 1.11.0-DEV.857
# Commit 140ea94f8e (2023-11-07 18:58 UTC)
# Platform Info:
#   OS: macOS (arm64-apple-darwin22.4.0)
#   CPU: 8 × Apple M2
#   WORD_SIZE: 64
#   LLVM: libLLVM-15.0.7 (ORCJIT, apple-m1)
#   Threads: 1 on 4 virtual cores
# Environment:
#   JULIA_EDITOR = code

This regression was caused by multiple commits and I have not performed a bisection. However, I suspect that #50824 (cc @oscardssmith and @vtjnash) is involved (because it's involved in everything) and #49827 (cc @Tokazama) because that is the last commit that touched the isassigned code which profiling indicates is the bottleneck here.

@LilithHafner LilithHafner added performance Must go faster kind:regression Regression in behavior compared to a previous version domain:arrays [a, r, r, a, y, s] kind:potential benchmark Could make a good benchmark in BaseBenchmarks labels Nov 7, 2023
@vtjnash
Copy link
Sponsor Member

vtjnash commented Nov 7, 2023

Probably #51760

@nsajko
Copy link
Contributor

nsajko commented Apr 20, 2024

Profiling shows most of the time is spend in the Array setindex!, so I guess this is caused by bounds checks. Possibly related (?): #54116

@nsajko
Copy link
Contributor

nsajko commented Apr 20, 2024

Flamegraph:

flamegraph

@nsajko
Copy link
Contributor

nsajko commented Apr 22, 2024

A workaround (and probably a good practice anyway) here is to specialize copyto! for the custom array type. Example: after JuliaArrays/FixedSizeArrays.jl#23, copying FixedSizeArrays is as fast as copying Arrays.

KristofferC added a commit that referenced this issue May 13, 2024
This reverts commit f0a28e9.

This introduced in general a try catch inside the inner loop for
`copyto!` and it also has performance regression in other cases
#53430.

Since this was added without any tests and "is not-quite-public API" it
seems easiest to just revert it.
This was added for Memory-to-Array and vice versa but dedicated methods
could be added for that if it is desirable

Fixes #53430,
#52070
@oscardssmith
Copy link
Member

should be closed by #54332

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] kind:potential benchmark Could make a good benchmark in BaseBenchmarks kind:regression Regression in behavior compared to a previous version performance Must go faster
Projects
None yet
Development

No branches or pull requests

4 participants