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

Improve performance of Kleisli: make it a value class #3080

Open
wants to merge 1 commit into
base: master
from

Conversation

@svalaskevicius
Copy link
Contributor

commented Sep 22, 2019

Make Kleisli a value class, to avoid the runtime cost of new object instantiation.

sbt 'bench/jmh:run "cats.bench.KleisliBench.*" -f1 -t1 -wi 2 -i2'

before:

[info] Benchmark                      Mode  Cnt       Score   Error  Units
[info] KleisliBench.baselineFlatMap  thrpt    2  299375.289          ops/s
[info] KleisliBench.flatMap          thrpt    2  127897.572          ops/s

after:

[info] Benchmark                      Mode  Cnt       Score   Error  Units
[info] KleisliBench.baselineFlatMap  thrpt    2  294876.092          ops/s
[info] KleisliBench.flatMap          thrpt    2  155593.301          ops/s
@svalaskevicius

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2019

Hmm binary compatiblity fails - expectedly tbh as some functions are renamed.. (otherwise the compiler is not happy)

Could this be part of a major version change then? Or does anyone have any better ideas?

@djspiewak

This comment has been minimized.

Copy link
Member

commented Sep 23, 2019

Hmm, could you leave the old functions as private? The benchmarks are impressive. It is worth noting that in a few cases, this will actually make performance worse (for example, List[Kleisli[F, R, A]], which will result in twice as much boxing as without the value class), but I think those cases are relatively rare.

@kailuowang

This comment has been minimized.

Copy link
Member

commented Sep 23, 2019

I think it's not just a few renames. It seems to me that changing this to a value class itself breaks BC. We still don't have a plan on how we do a binary breaking version on cats-core yet but we are working on it.

@svalaskevicius

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2019

Thanks both! Yeah I don't think private will help - the error I got from compiler is that when it became a value class the functions were too similar for it to resolve. We can try though if you think it might help :) plus the reason for bin compat failure is likely to be the change to value class as @kailuowang wrote..

Out of curiosity - @djspiewak - why would a list box it twice? I've checked https://docs.scala-lang.org/overviews/core/value-classes.html and one case is actually a bit worrying - apparently even a call to identity will instantiate it (what about tagless final usage?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.