Add toRightAssociated to AndThen

[johnynek]

For use cases such as Scalding or fs2 or similar systems where you will build up an AST then run a lot of data through it, we will evaluate map functions many times.

AndThen is a good tool for stack safety, but it does the right-association as it evaluates and does not save work between calls. For monads such as IO, this is good, because in general the map function will be called at most once.

Also, the perf hit is mitigated by the fact that AndThen uses a 128 normal function calls before moving to a trampoline approach, so the graph of AndThen should be pretty shallow in the general case.

This PR adds a toRightAssociated method to convert an AndThen once to the right associated form, where the evaluation will only hit Single(_, _) or Concat(Single(_, _), _). This is impossible to do in an external function because Single and Concat are private.

There is one open question: should AndThen keep a lazy val of the right associated version and route def apply through it? This could be a performance win for anyone that call an AndThen more than once I think, but at the expense that everyone takes a size hit of keeping both representations in memory.

For now, we take the conservative approach of not changing anything but opening the API for use cases that know they will execute the function many times.

[johnynek]

also, @alexandru in this line:

cats/core/src/main/scala/cats/data/AndThen.scala

Line 74 in aac643f

case Single(f, index) if index != fusionMaxStackDepth =>

which was introduced in the original PR:
#2187

it seems that if you call Single(f, 0).andThen(Concat(g, h)) then you hide the Concat inside the Single(f.andThen(Concat(g, h), 1) which seems like you can cause a stack overflow since the stack depth is inaccurate.

Should we not change that method to test if the passed function is already an AndThen and if it is a Concat already, then allocate a new Concat on the front?

Is there something I'm missing as to why that's safe? If so, can we add a comment so future readers aren't confused?

[johnynek]

we could potentially combine adjacent Single if the sum of their stack depth is < fusionMaxStackDepth? Such a case might have been hidden by a non-right associated construction.

[codecov-commenter]

Codecov Report

Merging #3527 into master will decrease coverage by 0.02%.
The diff coverage is 70.00%.

@@            Coverage Diff             @@
##           master    #3527      +/-   ##
==========================================
- Coverage   91.29%   91.26%   -0.03%     
==========================================
  Files         386      386              
  Lines        8577     8587      +10     
  Branches      240      235       -5     
==========================================
+ Hits         7830     7837       +7     
- Misses        747      750       +3     

[travisbrown]

This looks good to me. I definitely don't think AndThen should maintain a lazy val with the right-associated instance, since if users want that it's easy enough for them to do it manually.

[johnynek]

merged with HEAD, can you take a look @travisbrown

[travisbrown]

👍 from me.