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 optimized findall(::AbstractArray{Bool}) method #25879

Merged
merged 1 commit into from
Feb 6, 2018
Merged

Conversation

nalimilan
Copy link
Member

Allocating result upfront is faster, but it is possible only when collection can be iterated twice.
This fixes a regression introduced when rewriting the generic method.

Fixes #25489.

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

Allocating result upfront is faster, but it is possible only when collection can be iterated twice.
This fixes a regression introduced when rewriting the generic method.
@nalimilan
Copy link
Member Author

Let's try again:
@nanosoldier runbenchmarks(ALL, vs=":master")

@nalimilan nalimilan added performance Must go faster search & find The find* family of functions labels Feb 4, 2018
@nanosoldier
Copy link
Collaborator

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

@nalimilan
Copy link
Member Author

Hmm, there are (probably spurious) improvements all over the place, but the relevant one (["find", "(\"Array{Bool,1}\", \"50-50\")]) is only 9% faster, with 22% less allocated memory, and others do not even show up. I wonder whether the deprecation of find may make the timings completely meaningless.

@ararslan
Copy link
Member

ararslan commented Feb 6, 2018

The find stuff has been fixed in BaseBenchmarks (thanks Milan!!), so maybe this will be a little more representative.

@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

@nalimilan
Copy link
Member Author

nalimilan commented Feb 6, 2018

OK, with the new run ["find", "(\"Array{Bool,1}\", \"50-50\")] is three times faster and allocates half as much memory, which cancels the regression due to #24774. Local benchmarking shows that it's also the case with non-50/50 distributions, not sure why it's not visible (EDIT: that's because the benchmarks did not test them, see JuliaCI/BaseBenchmarks.jl#182). BitVector doesn't show up because it uses a specific method (I hadn't realized that).

@oscardssmith
Copy link
Member

Does this method remove the need for specialized BitVector one?

@nalimilan
Copy link
Member Author

No, I don't think so. The method this PR reintroduces used to exist already, both are useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster search & find The find* family of functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix performance regression of some find() methods
5 participants