Skip to content

Conversation

chethega
Copy link
Contributor

@chethega chethega commented Sep 2, 2018

As remarked on JuliaLang/julia#29000, accessing an MArray, MMatrix, MVector sometimes pulls a stack copy. This patch avoids the stack copy, by using the pointer for getindex as well, not just for setindex.

Copy link
Contributor

@KristofferC KristofferC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the thrown bounds errors be BoundsError(v, i) etc?

Copy link
Member

@andyferris andyferris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Thanks :)

Are the matrix and vector versions strictly necessary anymore? Perhaps we can delete those methods.

@chethega
Copy link
Contributor Author

chethega commented Sep 3, 2018

Yes (also for setindex!) and yes, removed the duplicates.

The boundschecks for cartesian index access are still broken, though.

src/MArray.jl Outdated
@propagate_inbounds function getindex(v::MArray, i::Int)
T = eltype(v)
if isbitstype(T)
@boundscheck if i < 1 || i > length(v)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to write this as @boundscheck checkbounds(v, i)?

@chethega
Copy link
Contributor Author

chethega commented Sep 3, 2018

Ok, this is fast. How do I fix the following?

julia> using StaticArrays
julia> M=MMatrix{2,2}(rand(2,2));
julia> M[3,1]
0.7134474216097795

@andyferris
Copy link
Member

OK what's there looks great now :)

For Cartesian indexing it's the ..._scalar functions at the start of indexing.jl

@chethega
Copy link
Contributor Author

chethega commented Sep 3, 2018

Ok, I think this can be merged (from my perspective). Can we run a benchmark to see whether the pointer access or the boundscheck confuse the optimizer? As far as I can see all cartesian accesses were effectively @inbounds up to now, so that may slow down some relying code :(

@andyferris
Copy link
Member

Let's roll with this.

@andyferris andyferris merged commit 6e41883 into JuliaArrays:master Sep 6, 2018
@c42f c42f mentioned this pull request Oct 22, 2018
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants