-
-
Notifications
You must be signed in to change notification settings - Fork 158
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
Speeding up Scala 3 Writing macros #440
Conversation
w: Any, | ||
value: 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.
Had to make these un-typed because I couldn't figure out how to make the Scala 3 macro quotes/splices play nicely with the path dependent prefix.Writer
type. A bit gross but shouldn't affect perf or correctness
mappedArgsI: String, | ||
w: Writer[V], | ||
value: V) = { | ||
def writeSnippet[R, V](objectAttributeKeyWriteMap: CharSequence => CharSequence, |
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 couldn't figure out how to subclass the path dependent prefix.CaseW
type in the macro, so I had to instantiate it in the inline part first and then call into the macro after. That means the call to writeSnippet
is no longer from the subclass, and it thus can no longer be protected
case m: Mirror.SumOf[T] => | ||
inline compiletime.erasedValue[T] match { | ||
case _: scala.reflect.Enum => | ||
val valueOf = macros.enumValueOf[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.
this won't work for ADT enums, the solution here is to assert all cases are singleton:
summonAll[Tuple.Map[m.MirroredElemTypes, [S] =>> S <:< Singleton]] // assert all singleton
maybe there is a more erased way to do this
I'll leave this over the weekend, and if there aren't any more comments I'll merge it then. All the existing/new/newly-enabled tests pass, and the benchmarks demonstrate the non-functional improvements, so I can't have broken anything too badly The issue noticed by @bishabosha is a limitation in existing code, which is why it was not picked up by any tests (since it never worked). Fixing that can come in a follow up |
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 looks good to me, although I'm myself not experienced in Scala 3 Macros. But tests pass and benchmark looks promising.
After we merge this, I'd like to cut a next milestone release 3.0.0-M2
to get this changed version tested in as many places as possible.
There seem to be issues with this change. E.g. in Ammonite, the integration tests don't succeed. |
Fixes #389 and #388
This PR moves a bunch of logic from runtime to compile time, and optimizes whatever runtime logic remains. It also consolidates the logic with the Scala 2 logic as much as possible, e.g. re-using
writeSnippet
to manage writes andCaseObjectContext
/HugeCaseObjectContext
to manage validation and error reporting during a readRunning some ad-hoc benchmarks on my laptop,
./mill bench.jvm.run
, it gives a significant speedup on reads and writes, and brings it close to the Scala 2.13 numbers (higher is better):Note that the generated code, especially for reads, is still not as optimized as the Scala 2 versions:
Array[Any]
match
statements, so I generated aMap[K, V]
and look it up at runtime.Fixing this issues and moving the reading logic into the macro should also be possible, but can happen in a separate PR
All existing tests pass. Added a regression test for the recursive Scala 3 scenario that hangs on master. Also moved a bunch of
AdvancedTests
into the shared folder now that they work