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

MonadRec instances for Eval and StateT. #1076

Merged
merged 1 commit into from
May 31, 2016

Conversation

TomasMikula
Copy link
Contributor

No description provided.

@codecov-io
Copy link

Current coverage is 88.34%

Merging #1076 into master will decrease coverage by <.01%

  1. 3 files (not in diff) in .../main/scala/cats/std were modified. more
  2. 2 files (not in diff) in ...n/scala/cats/functor were modified. more
  3. 3 files (not in diff) in ...main/scala/cats/data were modified. more
  4. 2 files (not in diff) in .../src/main/scala/cats were modified. more
    • Misses +1
    • Hits -1
@@             master      #1076   diff @@
==========================================
  Files           221        221          
  Lines          2798       2804     +6   
  Methods        2743       2749     +6   
  Messages          0          0          
  Branches         50         50          
==========================================
+ Hits           2472       2477     +5   
- Misses          326        327     +1   
  Partials          0          0          

Sunburst

Powered by Codecov. Last updated by d3c64d1...de5d911

@ceedubs
Copy link
Contributor

ceedubs commented May 31, 2016

👍 thanks!

@ceedubs
Copy link
Contributor

ceedubs commented May 31, 2016

@TomasMikula it's outside of the scope of this PR, but do you have an intuition for whether the introduction of MonadRec could allow us to change StateT from F[S => F[(S, A)]] to the more familiar S => F[(S, A)] version while retaining stack safety?

@non
Copy link
Contributor

non commented May 31, 2016

👍 Looks good, thanks!

@non non merged commit 6c6b2a8 into typelevel:master May 31, 2016
@TomasMikula TomasMikula deleted the state-monadrec branch May 31, 2016 17:29
@TomasMikula
Copy link
Contributor Author

@ceedubs MonadRec can help only if we use its tailRecM method. Thus, for example, foldMap-ing Free or foldM-ing a list/stream into StateT should be fine with the simpler representation, since those implementations would use tailRecM. Though I worry that there might be other use cases for arbitrarily long sequences of flatMaps where tailRecM is not applicable ¯_(ツ)_/¯

@TomasMikula
Copy link
Contributor Author

TomasMikula commented May 31, 2016

@ceedubs Here is an alternative representation of StateT that looks very much like S => F[(S, A)], except it is S -> F[(S, A)], where -> is a "free function" which allows composed functions to be evaluated in a stack-safe fashion.

As a bonus, it allows to weaken constraints on F for some operations, but that is not so important, since one will need F: Monad anyway to do the more interesting stuff.

Although this allowed to add one more stack-safety test which would fail before, in general, this new representation still guarantees stack-safety of long chains of flatMaps only for a lazy F, such as Eval or Free.

I haven't done any benchmarks.

The benefit of this is purely aesthetic/didactic.

@ceedubs
Copy link
Contributor

ceedubs commented Jun 3, 2016

@TomasMikula that's really interesting. I'd be interested in seeing how benchmarks compare (and I think @travisbrown would too if the benchmark results are good).

@TomasMikula
Copy link
Contributor Author

I'm actually curious, too, how this compares performance-wise. Do you know of any pre-existing StateT benchmarks that I could run?

@TomasMikula
Copy link
Contributor Author

@ceedubs Thanks, I will try to put something together.

@TomasMikula
Copy link
Contributor Author

So I created 2 tests ("iterated flatMap" and "recursive flatMap") and ran each with both strict and trampolined monad (Id vs. Eval), with a small (100) and large (100k) size. The benchmark source, and the results I got:

current cats
F[S => F[(S, A)]]
ops/s
experimental
S -> F[(S, A)]
ops/s
relative change
iterated Id 100 249952.905 163268.914 65.31% (😞)
iterated Id 100k stack overflow 132.055 ∞ (👍)
recursive Id 100 291328.689 193787.941 66.51% (😞)
recursive Id 100k stack overflow stack overflow
iterated Eval 100 92090.896 112858.675 122.55% (👍)
iterated Eval 100k 77.497 87.827 113.32% (👍)
recursive Eval 100 128470.948 198249.974 154.31% (👍)
recursive Eval 100k 132.774 199.594 150.53% (👍)

So the new representation performs worse for strict monads and better for trampolined monads.

@TomasMikula
Copy link
Contributor Author

TomasMikula commented Jun 4, 2016

I might be able to eliminate one allocation per step of evaluation by using asInstanceOf to convince scalac to eliminate tailcalls and give the benchmarks another run to see if it improves the results significantly. (If it does, that will likely mean that the change in #924 impairs performance, since that's the same kind of trade-off, allocation vs. asInstanceOf).

@TomasMikula
Copy link
Contributor Author

It did improve performance. I was actually able to get away with a single asInstanceOf per the whole evaluation (not one per each step), so my previous statement about relation to #924 doesn't apply. The updated results table:

current cats
F[S => F[(S, A)]]
ops/s
experimental
S -> F[(S, A)]
ops/s
relative change
iterated Id 100 249952.905 218172.202 87.28% (😞)
iterated Id 100k stack overflow 152.652 ∞ (👍)
recursive Id 100 291328.689 222709.763 76.64% (😞)
recursive Id 100k stack overflow stack overflow
iterated Eval 100 92090.896 131052.630 142.30% (👍)
iterated Eval 100k 77.497 93.276 120.36% (👍)
recursive Eval 100 128470.948 213118.856 165.88% (👍)
recursive Eval 100k 132.774 219.163 165.06% (👍)

This is the commit TomasMikula@ff7af34

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.

4 participants