-
Notifications
You must be signed in to change notification settings - Fork 39
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
(almost) allocationless tags ⚡ #348
Conversation
@@ -102,7 +102,6 @@ lazy val `kyo-core` = | |||
.in(file("kyo-core")) | |||
.settings( | |||
`kyo-settings`, | |||
libraryDependencies += "dev.zio" %%% "izumi-reflect" % "2.3.9", |
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.
Izumi dependency only for benchmarks now.
@@ -38,7 +38,7 @@ class coreBytecodeSizeTest extends KyoTest: | |||
assert(map == Map( | |||
"test" -> 28, | |||
"resultLoop" -> 94, | |||
"handleLoop" -> 280, | |||
"handleLoop" -> 257, |
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.
fewer bytecodes to call =:=
inline expected: Tag[T] | ||
inline def failTag( | ||
inline actual: Tag[?], | ||
inline expected: Tag[?]* |
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.
Since the new Tag
doesn't support intersection types, I'm simulating them here for error reporting via a vararg param.
import scala.quoted.* | ||
|
||
opaque type Tag[T] = String | ||
case class Tag[T](tpe: String) extends AnyVal |
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.
Using AnyVal
solved some issues I was having with type inference.
|
||
extension [T](t1: Tag[T]) | ||
|
||
def show = t1.tpe.takeWhile(_ != ';') |
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.
We should eventually improve this to show the entire type signature. This code only prints the outer class name, no type parameters. It's enough for the current error reporting use case, though.
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.
Perhaps worth a pending test!
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.
Makes sense! I've just added it.
() | ||
} | ||
"intersection type env" in { | ||
assertDoesNotCompile("Envs.get[Int & Double]") |
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.
Doesn't compile anymore since tags don't allow intersection types.
@@ -9,7 +8,7 @@ import sttp.tapir.server.ServerEndpoint | |||
import sttp.tapir.server.netty.NettyKyoServer | |||
import sttp.tapir.server.netty.NettyKyoServerBinding | |||
|
|||
type Route = ServerEndpoint[Any, KyoSttpMonad.M] | |||
case class Route(endpoint: ServerEndpoint[Any, KyoSttpMonad.M]) extends AnyVal |
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 change this since KyoSttpMonad.M
isn't a concrete type.
} | ||
end run | ||
|
||
def add[A: Tag, I, E: Tag: ClassTag, O: Flat](e: Endpoint[A, I, E, O, Any])( | ||
f: I => O < (Fibers & Envs[A] & Aborts[E]) | ||
): Unit < Routes = | ||
Sums.add(List( |
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.
There was a bug here introduced by #267. The new Tag
transformed it into a compilation error! \o/
// | ||
// Tag[Test[Param1, Param2]] | ||
// | ||
// Test;Super;java.lang.Object;scala.Matchable;scala.Any;[-Param1;Super;java.lang.Object;scala.Matchable;scala.Any;,+Param2;Super;java.lang.Object;scala.Matchable;scala.Any;] |
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 format has some opportunity for optimization. We might be able to omit java.lang.Object;scala.Matchable;scala.Any
and assume they're always present but I'm not sure about Matcheable
.
end if | ||
end checkParams | ||
|
||
def nextParam(tag: String, idx: Int): Int = |
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.
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.
Very impressive performance improvement!
The change looks pretty good overall. Will take a closer look at the tests later.
Does izumi have tests we can copy? It would be good to highlight the overlap in functionality, as well as where Kyo tags may won't work (should be rare in practice).
I wasn't able to tell, but can you still invoke ==
on two tags? Can we somehow disallow this?
|
||
def nextParam(tag: String, idx: Int): Int = | ||
@tailrec def loop(opens: Int, idx: Int): Int = | ||
tag.charAt(idx) match |
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.
Add a @switch annotation 🙏🏻
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.
Thanks! I need to get used to this annotation. I had to avoid the pattern guards to add the annotation but it should still improve performance. I added it in a couple of other places as well.
given canEqual[T, U]: CanEqual[Tag[T], Tag[U]] = CanEqual.derived | ||
import internal.* | ||
|
||
inline given apply[T >: Nothing]: Tag[T] = ${ tagImpl[T] } |
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.
Will the >: Nothing
be problematic for Aborts? (Or any effect)
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.
Good question! I think we actually shouldn't allow Tag[Nothing]
. In Kyo's usage of tags, it's essentially an inconsistent state since tags are meant to represent concrete types, and Nothing
, although it has a class symbol, has no runtime representation. I've changed the macro to fail for Tag[Nothing]
and added a test for it. It caught an inssue in streamsTest
! I've removed the type constraint since it isn't strict, and the macro will detect it anyway. Nothing
can still appear in nested type parameters. I'm not sure if we should constraint it too.
def show = t1.tpe.takeWhile(_ != ';') | ||
|
||
def <:<[U](t2: Tag[U]): Boolean = | ||
t1.tpe == t2.tpe || isSubtype(t1, t2) |
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.
Do we need to check reference equality? Or does the default string ==
do so?
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.
Nice catch! It doesn't, fixed.
def =!=[U](t2: Tag[U]): Boolean = | ||
t1.tpe != t2.tpe | ||
|
||
def >:>[U](t2: Tag[U]): Boolean = |
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.
Can't this be defined in terms of the other direction subtype method? Do we need this method?
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 prefer the freedom to order operands in both directions, it sometimes make the code more linear.
() | ||
} | ||
"method inference" in pendingUntilFixed { | ||
def test[T](v: T < (Envs[Int] & Envs[Double])) = |
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.
Isn't this still relevant?
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.
Both scenarios fail because of the intersection tag and scenarios that test compilation failures are relatively fragile since they can break for different reasons as the codebase evolves. I don't see much value in keeping them. I'd say this is more of a wontfix
for now but we can review if we explore supporting intersection and union types in tags.
Thanks for the review! 🙏
Izumi can represent many more kinds types than Kyo's tags but I think it's actually a good thing to have something more constrained. There are a few tests for unsupported types.
It's only possible to restrict it if the compiler is configured with strict equality enabled, which is the case in Kyo's codebase. If the flag is enabled, |
case _ => | ||
report.errorAndAbort(s"Unsupported Tag type ${tpe.show}") |
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.
Can we make the error messages unique to assist with debugging issues? When users submit tickets it may be useful to know which case they are hitting
@@ -321,7 +321,7 @@ class streamsTest extends KyoTest: | |||
"none" in { | |||
assert( | |||
Streams.initSeq(Seq(1, 2, 3)).collect { | |||
case v if false => ??? | |||
case v if false => (): Unit < Streams[Int] |
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.
Do we need a generalized unit method to improve inference?
object Kyo:
def unit[S]: Unit < 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.
I'd say this is an edge case for testing. I think this compilation error would happen most often when users make mistakes for example not composing effects correctly.
|
||
extension [T](t1: Tag[T]) | ||
|
||
def show = t1.tpe.takeWhile(_ != ';') |
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.
Perhaps worth a pending test!
Thanks for the reviews @hearnadam! I'll merge this and work on support for union and intersection types in a separate PR. |
Fixes #339.
Kyo uses tags for effect handling. Izumi's performance has been an important limitation, especially to provide sub-type checking it hot paths. This PR introduces a new
Tag
implementation that bundles the type information in a string format amenable to type comparisons without requiring allocations. Since the string is generated at compile-time, it becomes a constant in the class pool and doesn't require allocations.The only scenario where this new
Tag
can allocate is when one of the type parameters has an implicit tag. For example:In this case, the macro generates a tree that will concatenate the string representing
Tag[T]
intoTag[List[T]]
. JIT compilers can many times avoid such allocation, though.This change introduces an important restriction: only types with concrete class symbols (traits, classes, objects) are supported. We could extend the implementation to support intersection and union types as well but I think allowing these types can be confusing to users since they can be produced by inference when a user makes a mistake.
The performance is ~10x in comparison to Izumi:
The main reason is zero allocation:
https://jmh.morethan.io/?source=https://gist.githubusercontent.com/fwbrasil/7010fc054376ce94e3497de2fbeb3030/raw/0ba5e81e932185ef4b10c75201e0ace98aac4a33/jmh-result.json#details