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

Faster multi-arg mapreduce #39053

Closed
wants to merge 9 commits into from
Closed

Conversation

oscardssmith
Copy link
Member

Partially addresses #38558
If there's an easy way to make this handle the kwarkgs, let me know.

@oscardssmith oscardssmith added fold sum, maximum, reduce, foldl, etc. performance Must go faster labels Dec 30, 2020
@oscardssmith oscardssmith requested a review from tkf December 30, 2020 23:41
@oscardssmith
Copy link
Member Author

Also does anyone have a good idea for how to test for O(1) allocations?

@Keno
Copy link
Member

Keno commented Dec 31, 2020

Also does anyone have a good idea for how to test for O(1) allocations?

Add is as a benchmark, so nanosoldier will track it?

base/reducedim.jl Outdated Show resolved Hide resolved
@simeonschaub
Copy link
Member

Also does anyone have a good idea for how to test for O(1) allocations?

I think in this case just adding an explicit @allocated test (after you call the function once before to precompile) should be fine.

Co-authored-by: Fredrik Bagge Carlson <baggepinnen@gmail.com>
base/reducedim.jl Outdated Show resolved Hide resolved
@oscardssmith
Copy link
Member Author

oscardssmith commented Jan 20, 2021

I'm reverting to the first version of this that I made. It doesn't handle keword arguments, but I think it's probably good to merge as is since it's a strict improvement compared to master.

base/reducedim.jl Outdated Show resolved Hide resolved
@StefanKarpinski
Copy link
Member

Bump?

@oscardssmith
Copy link
Member Author

good call. I'd forgotten about this PR.

mapreduce(f, op, A::AbstractArrayOrBroadcasted...; kw...) =
reduce(op, map(f, A...); kw...)
function mapreduce(f, op, A::AbstractArrayOrBroadcasted...; kw...)
isempty(kw) && return mapreduce(A->f(A...), op, zip(A...))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this allow mapreduce(+, +, -3:3, -3:3, init=1) to take the zip path?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it should. Is there an easy way to do that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the only two legal ones are dims, init, but am not sure if init = _InitialValue() is understood by all the relevant methods. If not, then this would work:

function mapreduce(f, op, A::AbstractArrayOrBroadcasted...; dims=:, kw...)
    dims==(:) && return mapreduce(A->f(A...), op, zip(A...); kw...)
    ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Last commit needs ; kw...) -> ; dims=:, kw...), I think.

base/reducedim.jl Outdated Show resolved Hide resolved
Copy link
Member

@simeonschaub simeonschaub left a comment

Choose a reason for hiding this comment

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

LGTM

@simeonschaub simeonschaub added the merge me PR is reviewed. Merge when all tests are passing label Apr 27, 2021
@KristofferC
Copy link
Member

As a note, I think in PRs like this it is great if the first post contains a benchmark that shows the before/after performance and can easily be run by someone looking at the PR.

@simeonschaub
Copy link
Member

The failure here is real. Locally, replacing A -> f(A...) with Base.splat(f) and then defining

mapreduce_empty(f::typeof(splat(+)).name.wrapper, op, ::Type{T}) where {T<:Tuple} =
    reduce_empty(op, Core.Compiler.return_type(op, T))

worked as a quick fix. I think for a proper solution, we should make Base.splat return a functor of type Base.Splat though.

@simeonschaub simeonschaub removed the merge me PR is reviewed. Merge when all tests are passing label Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fold sum, maximum, reduce, foldl, etc. performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants