Stack-safe FreeAp by edmundnoble · Pull Request #1748 · typelevel/cats

edmundnoble · 2017-06-28T05:19:10Z

Port from JS, from @safareli's implementation. It's stack-safe, I just need to benchmark and comment it.

codecov-io · 2017-06-28T07:11:09Z

Codecov Report

Merging #1748 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1748      +/-   ##
==========================================
+ Coverage   94.81%   94.87%   +0.05%     
==========================================
  Files         241      241              
  Lines        4092     4139      +47     
  Branches       95      103       +8     
==========================================
+ Hits         3880     3927      +47     
  Misses        212      212

Impacted Files	Coverage Δ
...rc/main/scala/cats/laws/discipline/Arbitrary.scala	`92.18% <ø> (ø)`	⬆️
...ree/src/main/scala/cats/free/FreeApplicative.scala	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6fefa30...1d32d09. Read the comment docs.

kailuowang · 2017-07-06T13:23:01Z

docs/src/main/tut/datatypes/freeapplicative.md

+### The way FreeApplicative#foldMap works
+Despite being an imperative loop, there is a functional intuition behind `FreeApplicative#foldMap`.
+
+The new `FreeAp`'s `foldMap` is a sort of mutually-recursive function that operates on an argument stack and a 


quick nitpick question: shall we use FreeAp or FreeApplicative in the documentation?

Good point, I'll try to use FreeApplicative. My bad.

looks like you forgot about this one?

kailuowang · 2017-07-19T13:53:22Z

free/src/main/scala/cats/free/FreeApplicative.scala

-   */
-  final def fold(implicit F: Applicative[F]): F[A] =
+    * Tail recursive only if `F` provides tail recursive interpretation.
+    */


another nitpick here, the scala doc indentation is changed away from our standard.

I don't think I know what tail recursive implementation means for Applicative. Can we be more concrete or link somewhere in this comment?

It's stack-safe. Edit: I see what you mean now. I'll remove this comment, it doesn't make sense to me :P

kailuowang · 2017-07-19T13:56:01Z

free/src/main/scala/cats/free/FreeApplicative.scala

+    */
+  final def fold(implicit F: Applicative[F]): F[A] = {
    foldMap(FunctionK.id[F])
+  }


yet another superficial comment, what about all these addition of {. If we want to change the formating rule, shall we use another separate PR?

IntelliJ is conspiring against me 😛

kailuowang · 2017-07-19T14:00:52Z

free/src/test/scala/cats/free/FreeApplicativeTests.scala

    r1.foldMap(nt) should === (r2.foldMap(nt))
  }

+  test("FreeApplicative#flatCompile") {


How about making this test a scalacheck? In fact, to gain more confidence against the new implementation, how about we convert the existing related tests to scalacheck tests?

I'll do that, and add an Arbitrary which is more exhaustive.

Doing this has exposed a ClassCastException bug in the implementation, which I'm working through now.

kailuowang · 2017-07-19T14:06:38Z

I left a couple of superficial comments. To me, it's a bit too magical to validate eyeball, I think making all relevant tests more robust (by converting them to scalacheck) would help. I am also curious about the benchmarks.

edmundnoble · 2017-07-21T02:31:55Z

@kailuowang I did some basic benchmarks. A right-associated call tree with 50000 Ap calls takes as long to construct and evaluate now as one with 1000 leaves did previously. We're at 50x faster, presumably mostly because Ap used to take O(n) time and now takes O(1). I tried evaluating the same 50000 Ap tree using the old implementation, and my system ran out of RAM 😛

I also converted the tests I thought were appropriate to scalacheck.

safareli · 2017-07-21T08:40:24Z

@kailuowang I did some basic benchmarks. A right-associated call tree with 50000 Ap calls takes as long to construct and evaluate now as one with 1000 leaves did previously. We're at 50x faster, presumably mostly because Ap used to take O(n) time and now takes O(1). I tried evaluating the same 50000 Ap tree using the old implementation, and my system ran out of RAM 😛

Perfect 🎉 💃

kailuowang · 2017-07-21T14:22:24Z

laws/src/main/scala/cats/laws/discipline/Arbitrary.scala

  implicit def catsLawsArbitraryForReaderWriterStateT[F[_]: Applicative, E, L, S, A](implicit F: Arbitrary[(E, S) => F[(L, S, A)]]): Arbitrary[ReaderWriterStateT[F, E, L, S, A]] =
    Arbitrary(F.arbitrary.map(ReaderWriterStateT(_)))
+
+  implicit def catsLawsArbitraryForListNatTrans: Arbitrary[List ~> List] =


These two arbitrary instances seems a bit too, well, arbitrary and limited to be included in discipline for general uses. (actually the second one might be unused according to the codcov), how about we move them into the tests?

I'll put the used one into FreeApplicativeTests.

kailuowang · 2017-07-21T17:12:19Z

Thanks. The only minor comment left from my side is the FreeAp wording in the documentation.

edmundnoble · 2017-07-21T18:18:34Z

Oh wow, thought I fixed that a while ago. Done.

kailuowang · 2017-07-21T20:04:58Z

free/src/test/scala/cats/free/FreeApplicativeTests.scala

+  }
+
+  implicit def freeArbitrary[F[_], A](implicit F: Arbitrary[F[A]], FF: Arbitrary[(A, A) => A], A: Arbitrary[A]): Arbitrary[FreeApplicative[F, A]] =
+    Arbitrary(freeGen[F, A](2))


Sorry I meant to ask this last time but somehow lost it. Did you try maxDepth larger than 2? The main reason I ask here is that it seemed to me that you were preparing for testing maxDepth more than 2.

Yeah I was, I forgot to raise it again after lowering.

kailuowang

👍 It's a bit risky but the gain is substantial.

kailuowang · 2017-07-21T20:13:28Z

@peterneyens @johnynek since you two are the only other maintains left around, got some time to review this one? It would nice to be included in the MF release.

peterneyens · 2017-07-26T14:04:06Z

This indeed feels a bit magical.

I played around a bit to get a feel what is going on and ended up refactoring foldMap a bit. I haven't done any benchmarks to compare it, but it seems to pass the tests.
It is probably a bit slower, but I think it might be easier to comprehend than the more imperative implementation.

See edmundnoble/cats@stacksafefreeap...peterneyens:stacksafefreeap-peter

edmundnoble · 2017-07-26T18:00:35Z

@peterneyens Wow, thank you, this is really readable. It looks like it has to be noticeably less performant than the imperative implementation (extra intermediate list which is appended to the right and extra tuple allocation introduced while reassociating an Ap chain) but I think I can use this to improve the comments a lot.

peterneyens · 2017-07-27T13:19:54Z

Thanks for the improved documentation @edmundnoble.

Given the considerable performance improvements and the added stack safety, lets merge this as is, so we don't have to hold the 1.0.0-MF release any longer 👍 (Looks like we need to fix some conflicts first though).

We can look if we can clean up the foldMap implementation later on.

kailuowang · 2017-07-27T15:38:51Z

merging with 2 sign-offs

edmundnoble force-pushed the stacksafefreeap branch 4 times, most recently from 5e6adae to 10c6516 Compare July 5, 2017 01:06

edmundnoble changed the title ~~Stack-safe FreeAp (WIP, #1151)~~ Stack-safe FreeAp (#1151) Jul 5, 2017

edmundnoble assigned djspiewak Jul 5, 2017

edmundnoble force-pushed the stacksafefreeap branch from 10c6516 to 94159b9 Compare July 5, 2017 02:14

edmundnoble changed the title ~~Stack-safe FreeAp (#1151)~~ Stack-safe FreeAp Jul 5, 2017

edmundnoble requested a review from djspiewak July 5, 2017 05:20

edmundnoble unassigned djspiewak Jul 5, 2017

This was referenced Jul 5, 2017

Alternative formulation of Free Applicative ekmett/free#158

Open

consider another implementation of FreeAp scalaz/scalaz#1301

Open

kailuowang reviewed Jul 6, 2017

View reviewed changes

edmundnoble force-pushed the stacksafefreeap branch from 74554b5 to bcf954d Compare July 9, 2017 07:51

kailuowang added this to the 1.0.0-MF milestone Jul 19, 2017

kailuowang mentioned this pull request Jul 19, 2017

Prepare for the 1.0.0-MF release #1702

Merged

26 tasks

kailuowang reviewed Jul 19, 2017

View reviewed changes

edmundnoble force-pushed the stacksafefreeap branch 2 times, most recently from 1afce03 to c428c45 Compare July 20, 2017 05:38

Stack-safe FreeApplicative

92b6edc

edmundnoble force-pushed the stacksafefreeap branch from c428c45 to 92b6edc Compare July 21, 2017 01:54

kailuowang reviewed Jul 21, 2017

View reviewed changes

Move Arbitrary FunctionK instance

0f52dda

Last FreeAp mention gone

bb3b0bc

kailuowang reviewed Jul 21, 2017

View reviewed changes

kailuowang approved these changes Jul 21, 2017

View reviewed changes

Increase test tree size

f6ccdec

Better docs with examples

6454b4f

Merge branch 'master' into stacksafefreeap

1d32d09

kailuowang merged commit 693f456 into typelevel:master Jul 27, 2017

pakoito mentioned this pull request Aug 1, 2017

Free Applicative arrow-kt/arrow#164

Closed

peterneyens mentioned this pull request Sep 20, 2017

Optimize FreeApplicative.product #1921

Merged

safareli mentioned this pull request May 4, 2020

Stack safety gcanti/fp-ts#1194

Open

Uh oh!

Conversation

edmundnoble commented Jun 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jun 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edmundnoble Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kailuowang Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kailuowang commented Jul 19, 2017

Uh oh!

edmundnoble commented Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

safareli commented Jul 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kailuowang commented Jul 21, 2017

Uh oh!

edmundnoble commented Jul 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kailuowang left a comment

Choose a reason for hiding this comment

Uh oh!

kailuowang commented Jul 21, 2017

Uh oh!

peterneyens commented Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edmundnoble commented Jul 26, 2017

Uh oh!

peterneyens commented Jul 27, 2017

Uh oh!

kailuowang commented Jul 27, 2017

Uh oh!

Reviewers

Assignees

edmundnoble commented Jun 28, 2017 •

edited

Loading

codecov-io commented Jun 28, 2017 •

edited

Loading

edmundnoble Jul 19, 2017 •

edited

Loading

kailuowang Jul 19, 2017 •

edited

Loading

edmundnoble commented Jul 21, 2017 •

edited

Loading

peterneyens commented Jul 26, 2017 •

edited

Loading