-
Notifications
You must be signed in to change notification settings - Fork 50
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
prelude: revert pending type encoding to an opaque type #563
Conversation
@@ -1,5 +1,6 @@ | |||
package kyo2 | |||
|
|||
import Flat.unsafe.bypass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for a wrapper class anymore but we need to bypass the Flat
check. It's safe safe since zio-laws
can only compare two monad instances by calling the provided Equal
and should never rely on the regular ==
, even if the monad instance is nested into another. In other words, the A
provided to the Equal
can't be nested.
import kyo.Tag | ||
import scala.quoted.* | ||
|
||
opaque type Flat[T] = Null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copied from kyo-core
. I'll make sure to restore the git history in next PRs.
@@ -7,15 +7,12 @@ import scala.annotation.tailrec | |||
import scala.language.implicitConversions | |||
import scala.util.NotGiven | |||
|
|||
// TODO Constructor should be private but there's an issue with inlining | |||
case class <[+A, -S](private val curr: A | Kyo[A, S]) extends AnyVal | |||
opaque type <[+A, -S] = A | Kyo[A, S] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is declared as opaque type <[+A, -S] >: A = A | Kyo[A, S]
in kyo-core
. Note that I didn't add the >: A
back, which provides automatic lifting via the type system. The approach using implicit conversion has better usability because it can avoid unexpected nesting of computations (see NotGiven
in lift
bellow).
@@ -6,8 +6,12 @@ import kyo2.kernel.Safepoint | |||
private[kyo2] type Frame = kernel.Frame | |||
private[kyo2] inline def Frame = kernel.Frame | |||
|
|||
export kernel.< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to deal with a few cryptic compiler crashes while working on this change that went away once I avoided export
s here.
8e45b65
to
138e2e0
Compare
) | ||
) | ||
).eval._2 | ||
|
||
run(l).equals(run(r)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was incorrect since it wasn't using the provided Equal[A]
.
I'm porting the other modules to the new design in
kyo-prelude
but there's a performance regression that I can't see how to avoid with the new encoding in the current version of the Scala 3 compiler:Given the lack of specialization, the generated bytecode contains a significant amount of unexpected
<
boxing. Fortunately, the JIT is able to avoid most of these allocations but profiling sessions show<
allocations in several benchmarks. It isn't a bottleneck in the benchmarks I analyzed but introduces some overhead.The compiler doesn't allow implementing interfaces with
AnyVal
generic type parameters. As a workaround, I was planning to use a wrapper class like inMonadLawsTest
but the approach is too expensive in integrations like inkyo-sttp
. The wrapper class essentially introduces a new allocation to all methods in KyoSttpMonad, which are heavily used by sttp's transformations. Given how critical the integrations with sttp and tapir are, this regression doesn't seem ideal.As described in #531, the purpose of the new
AnyVal
encoding was to allow arbitrary nesting of Kyo computations. Although it's a nice usability improvement, the current limitations of the Scala 3 compiler doesn't seem to make it worth it and finalizing the migration to the new design seems a much higher priority. I can create a ticket so we can try again later.Related compiler tickets: scala/scala3#11264 scala/scala3#15532