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 a missing propagate_inbounds to a getindex method #40281

Merged
merged 2 commits into from Apr 1, 2021

Conversation

KristofferC
Copy link
Sponsor Member

@KristofferC KristofferC commented Mar 31, 2021

In #36427, many changes were made to reduce the number of invalidations during Julia bootstrap. One of them was the addition of this method:

julia/base/abstractarray.jl

Lines 1172 to 1173 in c0f8aef

# To avoid invalidations from multidimensional.jl: getindex(A::Array, i1::Union{Integer, CartesianIndex}, I::Union{Integer, CartesianIndex}...)
getindex(A::Array, i1::Integer, I::Integer...) = A[to_indices(A, (i1, I...))...]

This method lacks a @propagate_inbounds which leads to some performance degradation. This was for example reported in https://discourse.julialang.org/t/drop-of-performances-with-julia-1-6-0-for-interpolationkernels/58085/33?u=kristoffer.carlsson.

@KristofferC KristofferC added the backport 1.6 Change should be backported to release-1.6 label Mar 31, 2021
@KristofferC KristofferC added arrays [a, r, r, a, y, s] performance Must go faster labels Mar 31, 2021
@KristofferC KristofferC changed the title remove getindex method that only exists to reduce invalidation remove slower multiarg getindex method on arrays that only exists to reduce invalidations Mar 31, 2021
@KristofferC KristofferC changed the title remove slower multiarg getindex method on arrays that only exists to reduce invalidations add a missing propagate_inbounds to a getindex method Mar 31, 2021
@giordano
Copy link
Contributor

Does this fix #40276?

@KristofferC
Copy link
Sponsor Member Author

KristofferC commented Mar 31, 2021

Doesn't look like it.

@KristofferC KristofferC merged commit 80002db into master Apr 1, 2021
@KristofferC KristofferC deleted the kc/revert_getindex_change branch April 1, 2021 13:01
KristofferC added a commit that referenced this pull request Apr 4, 2021
* add a missing propagate_inbounds

(cherry picked from commit 80002db)
@KristofferC KristofferC mentioned this pull request Apr 4, 2021
33 tasks
KristofferC added a commit that referenced this pull request Apr 4, 2021
* add a missing propagate_inbounds

(cherry picked from commit 80002db)
KristofferC added a commit that referenced this pull request Apr 4, 2021
* add a missing propagate_inbounds

(cherry picked from commit 80002db)
@KristofferC KristofferC removed the backport 1.6 Change should be backported to release-1.6 label May 4, 2021
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
staticfloat pushed a commit that referenced this pull request Dec 23, 2022
* add a missing propagate_inbounds

(cherry picked from commit 80002db)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s] performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants