Skip to content

Make reshape(::BitArray,...) type stable #21670

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 3, 2017
Merged

Make reshape(::BitArray,...) type stable #21670

merged 1 commit into from
May 3, 2017

Conversation

@mbauman
Copy link
Member

@mbauman mbauman commented May 2, 2017 •
edited
Loading

This works around some of the performance regressions flagged in #20993. The instability itself is not new nor is it the cause of the regression. The trouble is just that there are a few more inlined functions now, each of which needs a dynamic dispatch.

It is worth noting that 0.6 (with this patch) is significantly faster than 0.5 (with this patch). I measure an improvement of ~30%.

@mbauman
Make reshape(::Bitarray,...) type stable
a6241dc 
This works around some of the performance regressions flagged in #20993. The instability itself is not new nor is it the cause of the regression. The trouble is just that there are a few more inlined functions now, each of which needs a dynamic dispatch.

It is worth noting that 0.6 (with this patch) is significantly faster than 0.5 (with this patch). I measure an improvement of ~30%.
@mbauman
Copy link
Member Author

mbauman commented May 2, 2017

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

@mbauman mbauman changed the title Make reshape(::Bitarray,...) type stable Make reshape(::BitArray,...) type stable May 2, 2017
@ararslan ararslan added the arrays [a, r, r, a, y, s] label May 2, 2017
@nanosoldier
Copy link
Collaborator

nanosoldier commented May 2, 2017

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

Sacha0
Sacha0 approved these changes May 2, 2017
View reviewed changes
Copy link
Member

@Sacha0 Sacha0 left a comment

Choose a reason for hiding this comment

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

lgtm! :)

@Sacha0
Copy link
Member

Sacha0 commented May 2, 2017

@nanosoldier runbenchmarks(("array" && "bool") || "simd", vs = ":master")

@nanosoldier
Copy link
Collaborator

nanosoldier commented May 2, 2017

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against primary commit: failed process: Process(`sudo cset shield -e su nanosoldier -- -c ./benchscript.sh`, ProcessExited(1)) [1]

Logs and partial data can be found here
cc @jrevels

@jrevels
Copy link
Member

jrevels commented May 2, 2017

Just deployed JuliaCI/Nanosoldier.jl#33, let's see if Nanosoldier can run things on Julia v0.7- now:

@nanosoldier runbenchmarks(("array" && "bool") || "simd", vs = ":master")

@nanosoldier
Copy link
Collaborator

nanosoldier commented May 2, 2017

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against primary commit: failed process: Process(`/home/nanosoldier/workdir/tmp430hB7/julia -e 'Pkg.add("BaseBenchmarks")'`, ProcessExited(1)) [1]

Logs and partial data can be found here
cc @jrevels

@jrevels
Copy link
Member

jrevels commented May 2, 2017

Alright, fixed a silly bug in that Nanosoldier patch, let's see how we do now:

@nanosoldier runbenchmarks(("array" && "bool") || "simd", vs = ":master")

@nanosoldier
Copy link
Collaborator

nanosoldier commented May 2, 2017

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

@Sacha0
Copy link
Member

Sacha0 commented May 3, 2017

The ["array","bool","boolarray_true_load!"] regression seems real (or at least notably consistent across nanosoldier runs)?

@mbauman
Copy link
Member Author

mbauman commented May 3, 2017

I can't reproduce it, and I have absolutely zero hypotheses on how this change might affect perf_true_load!($boolarr) negatively. There are no reshapes there, and note that it's the Vector{Bool}, not the BitArray.

@Sacha0
Copy link
Member

Sacha0 commented May 3, 2017

I can't reproduce it, and I have absolutely zero hypotheses on how this change might affect perf_true_load!($boolarr) negatively. There are no reshapes there, and note that it's the Vector{Bool}, not the BitArray.

lgtm then! :)

@Sacha0
Copy link
Member

Sacha0 commented May 3, 2017

(That benchmark popped up in #21677 (comment) with an equivalent improvement, further supporting the noise hypothesis.)

@mbauman
Copy link
Member Author

mbauman commented May 3, 2017

Yeah, it seems to frequently appear ±40%. https://github.com/JuliaCI/BaseBenchmarkReports/search?utf8=✓&q=boolarray_true_load+language%3Amd&type=

@KristofferC KristofferC merged commit 253fa74 into master May 3, 2017
@mbauman mbauman deleted the mb/reshaped-bits-stability branch May 3, 2017 19:38
tkelman pushed a commit that referenced this pull request May 4, 2017
@mbauman @tkelman
Make reshape(::Bitarray,...) type stable (#21670)
7b07975 
This works around some of the performance regressions flagged in #20993. The instability itself is not new nor is it the cause of the regression. The trouble is just that there are a few more inlined functions now, each of which needs a dynamic dispatch.

It is worth noting that 0.6 (with this patch) is significantly faster than 0.5 (with this patch). I measure an improvement of ~30%.
(cherry picked from commit 253fa74)
tkelman pushed a commit that referenced this pull request May 4, 2017
@mbauman @tkelman
Make reshape(::Bitarray,...) type stable (#21670)
be46bc8 
This works around some of the performance regressions flagged in #20993. The instability itself is not new nor is it the cause of the regression. The trouble is just that there are a few more inlined functions now, each of which needs a dynamic dispatch.

It is worth noting that 0.6 (with this patch) is significantly faster than 0.5 (with this patch). I measure an improvement of ~30%.
(cherry picked from commit 253fa74)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@Sacha0 Sacha0 Sacha0 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

arrays [a, r, r, a, y, s] performance Must go faster

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@mbauman @nanosoldier @Sacha0 @jrevels @KristofferC @tkelman @ararslan