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

Optimise Kleisli with specialized Function1 implementation #4211

Merged
merged 12 commits into from
May 30, 2022

Conversation

bplommer
Copy link
Contributor

@bplommer bplommer commented May 23, 2022

Adds an implementation of Kleisli that directly wraps a given F[B] rather than lifting it into a function. Adds a StrictConstFunction1 type to optimise Kleisli lifted from effectful values. Intended to improve performance of tagless final algebras running in Kleisli. This is borne out by results of cats-effect deep bind benchmark, lifted to Kleisli in the obvious way - throughput on async increases by about 65%, on delay by about 54%, and on pure by about 12%.

cats-effect deep bind: IO

jmh:run -i 10 -wi 10 -f 2 -t 1 cats.effect.benchmarks.DeepBindBenchmark

Benchmark                (size)   Mode  Cnt      Score     Error  Units
DeepBindBenchmark.async   10000  thrpt   20   2961.801 ±  35.939  ops/s
DeepBindBenchmark.delay   10000  thrpt   20  11702.678 ± 429.010  ops/s
DeepBindBenchmark.pure    10000  thrpt   20  14763.333 ± 119.990  ops/s

cats-effect deep bind: Kleisli (before):

jmh:run -i 5 -wi 5 -f 1 -t 1 cats.effect.benchmarks.DeepBindBenchmark

Benchmark                (size)   Mode  Cnt     Score    Error  Units
DeepBindBenchmark.async   10000  thrpt    5  1144.643 ±  5.384  ops/s
DeepBindBenchmark.delay   10000  thrpt    5  2844.455 ± 46.374  ops/s
DeepBindBenchmark.pure    10000  thrpt    5  4395.099 ± 12.977  ops/s

cats-effect deep bind: Kleisli (first implementation in this PR):

jmh:run -i 5 -wi 5 -f 1 -t 1 cats.effect.benchmarks.DeepBindBenchmark

Benchmark                (size)   Mode  Cnt     Score    Error  Units
DeepBindBenchmark.async   10000  thrpt    5  1825.018 ± 21.151  ops/s
DeepBindBenchmark.delay   10000  thrpt    5  4857.402 ± 26.344  ops/s
DeepBindBenchmark.pure    10000  thrpt    5  5459.568 ± 59.821  ops/s

cats-effect deep bind: Kleisli (current implementation in this PR, with StrictConstFunction1):

jmh:run -i 5 -wi 5 -f 1 -t 1 cats.effect.benchmarks.DeepBindBenchmark

Benchmark                (size)   Mode  Cnt     Score     Error  Units
DeepBindBenchmark.async   10000  thrpt    5  1892.768 ±  28.334  ops/s
DeepBindBenchmark.delay   10000  thrpt    5  4397.772 ± 254.866  ops/s
DeepBindBenchmark.pure    10000  thrpt    5  4908.705 ±  28.870  ops/s

@bplommer bplommer marked this pull request as ready for review May 23, 2022 13:05
@bplommer bplommer marked this pull request as draft May 23, 2022 13:40
@bplommer
Copy link
Contributor Author

Thought of a better way to do this, by specializing Function1 rather than Kleisli - converting back to draft.

@armanbilge armanbilge added this to the 2.8.0 milestone May 23, 2022
@@ -204,7 +209,7 @@ sealed private[data] trait KleisliFunctions {
* }}}
*/
def liftF[F[_], A, B](x: F[B]): Kleisli[F, A, B] =
Kleisli(_ => x)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can find other places in cats where we have _ => x and replace with StrictConstFunction1 profitably.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe! But see #4211 (comment)

core/src/main/scala/cats/data/StrictConstFunction1.scala Outdated Show resolved Hide resolved
@bplommer
Copy link
Contributor Author

bplommer commented May 24, 2022

The benchmarks for this implementation (added to the bottom of the PR description) aren't actually as good as the one from b576136 that specialised Kleisli rather than Function1, which surprised me a bit (though they're still a big improvement) - I changed a few things though, so I need to try changing things more incrementally to see what's making the difference.

@bplommer
Copy link
Contributor Author

bplommer commented May 24, 2022

On further thought, I don't think we can so blithely assume referential transparency as this PR does - for example we provide a MonadThrow instance for Future that can be lifted into Kleisli, which I'm fairly sure this will break. Maybe instead of making this a drop-in change we need to provide new constructors with stronger constraints - possibly Defer?

Unfortunately I don't have time to think this through just now. Any thoughts welcomed!

Edit: Defer isn't going to give the needed guarantee - it still allows for non-RT constructors, e.g. in Eval. It seems like we'd need a marker trait that guarantees lack of non-RT constructors.

@bplommer
Copy link
Contributor Author

Discussion on referential transparency taking place on Discord at https://discord.com/channels/632277896739946517/633329569402978313/978609454465564752 - the consensus seems to be that we can assume RT and there are no gurantees about what happens when that assumption is violated.

@bplommer
Copy link
Contributor Author

Re-opening this for review - there's probably scope for some further optimisation but the benefits as they stand are pretty clear.

@bplommer bplommer marked this pull request as ready for review May 25, 2022 12:07
@bplommer bplommer changed the title Add specialised Kleisli implementation for Kleisli.liftF Optimise Kleisli with specialized Function1 implementation May 25, 2022
johnynek
johnynek previously approved these changes May 28, 2022
@johnynek johnynek merged commit 0fd2fdb into typelevel:main May 30, 2022
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.

3 participants