Skip to content

Improve performance of foreach for tuples #31901

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

Merged
merged 1 commit into from
Jan 16, 2022

Conversation

danielmatz
Copy link
Contributor

Fixes #31869

@danielmatz
Copy link
Contributor Author

Are these CI errors expected? They don't seem related to my changes...

@StefanKarpinski
Copy link
Member

Perhaps rebase and force push to rerun the CI?

@danielmatz danielmatz force-pushed the foreach-tuples branch 3 times, most recently from d9758e6 to 1a7c72a Compare June 7, 2019 16:01
@danielmatz
Copy link
Contributor Author

Hmm, that cleared a few up, but I can't seem to be able to get rid of that last failure. The build keeps timing out.

@danielmatz danielmatz force-pushed the foreach-tuples branch 3 times, most recently from 3a07e80 to 103520b Compare June 14, 2019 20:25
@tkf
Copy link
Member

tkf commented Jul 10, 2019

foreach can be implemented using foldl:

foreach(f, xs) = foldl((_, x) -> (f(x); nothing), xs; init=nothing)

It will then automatically provide specializations for foreach for any container types defining specializations for mapfoldl. Maybe that's a better approach as it's more generic?

@danielmatz
Copy link
Contributor Author

Good idea! I was just mimicking the map code nearby in tuple.jl, but I like your idea more.

Isn't it possible to implement many of the higher order functions using foldl? Your argument seems to be more general than just my foreach change. Should the other higher order functions be implemented in terms of mapfoldl?

Also, I just noticed that there's no method for filter on a tuple. If filter had been implemented in terms of mapfoldl, then the tuple implementation of mapfoldl would have given us filter automatically...

@tkf
Copy link
Member

tkf commented Jul 10, 2019

Isn't it possible to implement many of the higher order functions using foldl?

Yes! In fact, mapfoldl can also be implemented based on foldl. Other "iterator transforms" like filter and flatten as well. If you go into this route, you will eventually get transducers (my implementation here: https://github.com/tkf/Transducers.jl).

But I suppose core devs would think adding transducers to Base is too radical change ATM. Manually implementing foreach and filter in terms of foldl seems to be a good direction.

@danielmatz
Copy link
Contributor Author

danielmatz commented Jul 11, 2019

OK, I'll start small. : )

I'm hitting some snags with this approach right now because foldl isn't included in compiler/compiler.jl, so the build fails. If I simply include("reduce.jl"), then @simd isn't found, and so on. I'll work on resolving that and update my branch soon.

@danielmatz
Copy link
Contributor Author

OK, I just pushed my first cut at it.

I started chasing the things to include in the bootstrap process, but ended up just creating a dummy @simd macro that does nothing to break the chain. Is that acceptable?

I'll look at adding a filter implementation next. Or maybe that should be a separate PR...

And one more question. Does anyone know why the following method exists:

foreach(f) = (f(); nothing)

That seems really strange to me. It is especially strange that it calls f() at all.

@tkf
Copy link
Member

tkf commented Jul 11, 2019

FWIW I find foreach(f) = (f(); nothing) strange, too.

@danielmatz danielmatz changed the title Add methods to foreach for tuple arguments Implement foreach using foldl to support more container types Aug 30, 2019
@danielmatz danielmatz changed the title Implement foreach using foldl to support more container types WIP: Implement foreach using foldl to support more container types Sep 27, 2019
@StefanKarpinski
Copy link
Member

StefanKarpinski commented Oct 1, 2019

I have to say I'm puzzled by the motivation for this change. foreach is currently a dead simple function: foreach(f, itr) does for x in itr; f(x); end. Why would we complicate that?

@danielmatz
Copy link
Contributor Author

danielmatz commented Oct 1, 2019

My original motivation was to improve the performance of foreach for tuples. There's currently no specialization of foreach for tuple collections.

If you look at the map function, we do have a specialization that unrolls the function applications for small tuples. My first attempt at this PR mimicked the way map works in that case.

Then, @tkf suggested I could just use foldl instead. I think his idea is that if more higher-order functions were implemented on top of foldl, they would work for any container type that implements foldl efficiently.

If you dislike the foldl approach, I'm happy to revert to my older code that just specialized foreach on tuples directly.

@StefanKarpinski
Copy link
Member

How could more collection types could implement foldl than iteration? Iteration seems like the most basic thing for anything iterable to implement. I don't really have any strong opinions on how to specialize foreach on tuples but I don't think it should affect all other uses of foreach.

@tkf
Copy link
Member

tkf commented Oct 1, 2019

How could more collection types could implement foldl than iteration?

That's the whole point of my JuliaCon talk 😉. It's more natural and efficient to implement foldl directly especially when your collection has a complex structure (e.g., nesting and type-level shape information).

But I don't want to stall @danielmatz's PR; even supporting tuples for now is a nice improvement.

@danielmatz
Copy link
Contributor Author

It seems that I've conflated what should really be two separate PRs.

I've just reworked this PR to add a specialization of foreach for tuples directly, rather than change the fallback definition of foreach.

The separate issue that got mixed in here is whether we'd like to rework various higher-order functions on top of foldl instead of iteration, with the fallback foldl just using iteration. Perhaps @tkf and I can collaborate on that in a separate PR.

My only remaining question is whether folks are OK with me including the deletion of

foreach(f) = (f(); nothing)

in this PR, or if that needs further discussion and should be split out as a separate PR.

@danielmatz danielmatz changed the title WIP: Implement foreach using foldl to support more container types WIP: Improve performance of foreach for tuples Oct 2, 2019
@danielmatz danielmatz changed the title WIP: Improve performance of foreach for tuples Improve performance of foreach for tuples Oct 4, 2019
@StefanKarpinski
Copy link
Member

StefanKarpinski commented Oct 9, 2019

t's more natural and efficient to implement foldl directly especially when your collection has a complex structure (e.g., nesting and type-level shape information).

Yes, that would be fine if foldl was the most basic unit of iteration in Julia, but currently that's not the case. Rather, foldl is generally a fairly complex, higher order function that is almost always defined in terms of iteration which is the most basic protocol for collections and things that are iterable. I understand that you'd like to potentially make that different in Julia 2.0, and that's something I'm sympathetic to, but that's not the world we live in right now.

@tkf
Copy link
Member

tkf commented Oct 9, 2019

I understand that iterate has to stay as the foundation of iteration in Julia, at least within 1.x for sure and certainly not complaining about it. I also apologise if I slowed down this PR.

Having said that, I think it is conceivable to (re)structure Julia code base such that

  • foreach/map/filter/sum/prod/dot/etc. depending on...
    • foldl/reduce depending on...
      • iterate

With this scheme, it is OK to overload foldl or reduce as performance or other kind of optimization while the minimum requirement still is iterate.

This is partially already true. For example, reduce (or actually mapreduce) has specialized method for arrays which is used for pair-wise sum. There are also array types in the wild such as RecursiveArrayTools.ArrayPartition and CuArray which overload reduce for performance reason. If this becomes a trend in foldl and reduce for other collection types, it makes sense for foreach to use foldl. Or rather, more functions (including foreach) using foldl would help such a trend. (But as @danielmatz pointed out this is better be done in a separate PR)

@StefanKarpinski
Copy link
Member

That's entirely possible as an approach but I don't think this PR is the place to try to sneak that into one single function. Instead, what should happen is there should be a Julep-style issue to propose and discuss (can be roughly just what you wrote here), then if people agree to it, we can come up with a plan to change things around so that this is the arrangement.

@stevengj
Copy link
Member

stevengj commented Jan 15, 2022

👍 for fast foreach on tuples. I find myself wanting this a lot. e.g. here was another case today.

In the long run, it would be better to have faster (unrolled) generic iteration on tuples, but I feel like foreach is something we can do now.

@@ -1916,7 +1916,6 @@ julia> foreach(x -> println(x^2), a)
49
```
"""
foreach(f) = (f(); nothing)
Copy link
Member

@stevengj stevengj Jan 15, 2022

Choose a reason for hiding this comment

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

It seems like this method is necessary to (1) resolve the ambiguity between foreach(f, itrs...) and foreach(f, itrs::Tuple...) in the empty-iterator case and (2) be non-breaking (note that for z in zip() doesn't currently work: #43821).

If we want to change the foreach(f) behavior, that should be a separate PR.

@danielmatz
Copy link
Contributor Author

I'm sorry that I neglected this PR for so long. Thanks for motivating me to look at it again!

I've restored foreach(f) and rebased onto the latest master branch.

@tkf
Copy link
Member

tkf commented Jan 15, 2022

In the long run, it would be better to have faster (unrolled) generic iteration on tuples

It's called left fold.

@tkf tkf added the merge me PR is reviewed. Merge when all tests are passing label Jan 15, 2022
@stevengj
Copy link
Member

It would be nicer if

for x in sometuple
   ....do something...
end

were unrolled and fast, but currently Julia's compiler gets confused if the tuple elements are of different types.

@tkf
Copy link
Member

tkf commented Jan 16, 2022

We can lower a for loop to a left fold: https://github.com/JuliaFolds/FLoops.jl

@tkf tkf merged commit 3522661 into JuliaLang:master Jan 16, 2022
@tkf tkf removed the merge me PR is reviewed. Merge when all tests are passing label Jan 16, 2022
@stevengj
Copy link
Member

Yes, but in the 1.x time frame?

@danielmatz
Copy link
Contributor Author

I just noticed that this only improves the performance of foreach when it is given a single iterable. The varargs form is about the same as the generic foreach. I'm guessing it's because we have to zip up the tuples to pass them to foldl? Is there an improvement to be made to zip when given Tuple arguments? Or can foldl do something more clever when it is given a Zip?

@tkf
Copy link
Member

tkf commented Jan 16, 2022

Yes, but in the 1.x time frame?

For the expressibility of the protocol, I think I proved it's possible. A generalized version of foldl can supports the full semantics of our for loop. But the question is if we can live with the cost of extra work of inlinear. I'm trying to evaluate the other benefits of fold-based for-loop to assess the relative pros and cons of foldl vs iterate.

Is there an improvement to be made to zip when given Tuple arguments? Or can foldl do something more clever when it is given a Zip?

Yeah, we need specialization in foldl. Maybe a good approach is to have something like Base.maymaterialize(itr) to eagerly evaluate zip-of-tuples. It's a generalization of Base._reverse. We can then use it inside of foldl to turn zip-of-tuples to just a tuple-of-tuples (among other things).

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.

No foreach methods specifically for Tuple arguments
4 participants