-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Force inlining on indices(A, d) #18014
Conversation
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels |
That was anticlimactic. |
Might just reflect the state of our performance tests. But since it tends to be something you call at the beginning of a loop (once), it shouldn't affect performance that much. (Bounds-checking was once implemented using |
Despite the lack of improvement on our benchmarks, I suspect this is still worth doing, but I'll leave this open for further comments. |
@@ -51,7 +51,11 @@ size{N}(x, d1::Integer, d2::Integer, dx::Vararg{Integer, N}) = (size(x, d1), siz | |||
|
|||
Returns the valid range of indices for array `A` along dimension `d`. | |||
""" | |||
indices{T,N}(A::AbstractArray{T,N}, d) = d <= N ? indices(A)[d] : OneTo(1) | |||
function indices{T,N}(A::AbstractArray{T,N}, d) | |||
@_inline_meta |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whats wrong with @inline
?
I notice Base has a lot of @_inline_meta
but I don't understand why
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed before @inline
is usable, typically before some array functions are defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To expand a bit: since macros run at code-loading time (not compilation time), julia has to be capable of doing everything that the @inline
macro calls in its internal implementation. At this point, array.jl
hasn't even been loaded yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks!
There don't seem to be any objections, so let's do this. |
If you think this should be backported, do speak up. |
woof woof (I don't think this is very important, but there's also no reason not to have it, and it might save someone from a #17126-like annoyance.) |
Addresses f9fb4da#commitcomment-18621877.
@nanosoldier
runbenchmarks(ALL, vs=":master")