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

Use propagate_inbounds in AbstractArray iterator #27535

Conversation

haampie
Copy link
Contributor

@haampie haampie commented Jun 11, 2018

I'm sure this was removed accidentally at some point.

julia> function simple_sum(V)
         s = 0.0
         @fastmath @inbounds for x in V
           s += x
         end
         s
       end

# Current master
julia> @btime simple_sum(v) setup = (v = rand(500))
  51.872 ns (0 allocations: 0 bytes)
251.22882057537214

# Current master
julia> @btime simple_sum(v) setup = (v = view(rand(500, 20), :, 1))
  553.941 ns (0 allocations: 0 bytes)
244.35868876917397

# This pr
julia> @btime simple_sum(v) setup = (v = view(rand(500, 20), :, 1))
  52.354 ns (0 allocations: 0 bytes)
254.29673869300711

@Keno
Copy link
Member

Keno commented Jun 12, 2018

This seems fine, but is a bit odd, since iterate is not getindex. Perhaps we're just missing something along the lines of #27386 for views?

@haampie
Copy link
Contributor Author

haampie commented Jun 12, 2018

I agree, but #27384 (comment) argues that something similar might be impossible. Also #25743 shows that a specialized iterator for views will probably not be safe.

@ararslan
Copy link
Member

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

@haampie
Copy link
Contributor Author

haampie commented Jun 12, 2018

Seems like nanosoldier is hanging?

@ararslan
Copy link
Member

Yeah it's definitely taking longer than normal, but the log suggests that it's actually still going. I'll give it a bit and kill it if doesn't respond in the next couple hours.

@nanosoldier
Copy link
Collaborator

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

@haampie
Copy link
Contributor Author

haampie commented Jun 13, 2018

Can't really explain this report 🔮

@KristofferC
Copy link
Member

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

@nanosoldier
Copy link
Collaborator

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

@haampie haampie closed this Oct 4, 2018
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.

5 participants