Scala 3 serialization macros are very inefficient compared to Scala 2 #389
Comments
|
I am happy to take a look at it. It would be a good opportunity to also check #386. |
|
Thanks @jodersky! I was planning on releasing 2.0.0 since the other PRs have merged, but if you're going to take a look at this I can hold off a week to see if we can include your work. The proposed changes will affect binary compatibility for Scala 3 if I'm not mistake |
are you talking about binary compatibility of 2.0 or macros? In case you mean the latter, I don't think that anything would change. The macros are a black box, right? They will generate instances of the ReadWriters, but beyond that they are totally opaque to user programs. What I'm saying is that while I appreciate you willing to hold off on 2.0 for a little, I don't think you have to :) We could release fixes to macros in a 2.1 or other patch release. |
|
@jodersky I'm imagining a case where someone compiles against scala3/upickle2 with the current macros+runtimesupport, we release a new scala3/upickle2+ without the runtime support, and then someone using their their earlier-compiled macro serializers against the newer version of upickle hits NoSuchMethod errors. The earlier-compiled code would still have callsites to the runtime support code which gets removed in upickle 2+ On the other hand, I would guess that anyone dealing with Scala3 would be OK with some amount of instability, so I'll probably release upickle 2 as is and the scala 3 users can deal with the upickle 2+ breakage by upgrading when the time comes |
|
You could also keep the runtime support without using it, just to support old versions. |
|
@russellremple was able to reduce quite a bit of the penalty when he ported the upickle 3 macros to weepickle in rallyhealth/weePickle#81, mostly by removing those runtime Maps. Even so, they're still slower than the scala 2 macros. These benchmark results show the change in the case class macros, i.e. no JSON parsing, between scala 3.1.0 and 2.13.8 for both weepickle and upickle 2.0.0. |
|
Yep, we added a lot of caching and other fine tunings to avoid many of the unnecessary Map builds, etc. But underlying macros are still slower. Note there were other Scala 3 enhancements in rallyhealth/weePickle#94 to support auto-derivation for Scala 3 enums, but that didn't really affect performance. (But it is really cool IMO.) |
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 and `CaseObjectContext`/`HugeCaseObjectContext` to manage validation and error reporting during a read Running 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): | Benchmark (5000ms) | Scala 3 Before | Scala 3 After | Scala 2 | |-------------------------------------|----------------|---------------|---------| | upickleDefault Read | 637 | 1065 | 1403 | | upickleDefault Write | 839 | 1452 | 1549 | | upickleDefaultByteArray Read | 582 | 1172 | 1126 | | upickleDefaultByteArray Write | 847 | 1218 | 1277 | | upickleDefaultBinary Read | 925 | 3853 | 3844 | | upickleDefaultBinary Write | 1252 | 3117 | 3666 | | upickleDefaultCached Read | 620 | 1300 | 1412 | | upickleDefaultCached Write | 829 | 1555 | 1588 | | upickleDefaultByteArrayCached Read | 575 | 1182 | 1095 | | upickleDefaultByteArrayCached Write | 838 | 1223 | 1297 | | upickleDefaultBinaryCached Read | 928 | 3825 | 3885 | | upickleDefaultBinaryCached Write | 1266 | 2907 | 3674 | Note that the generated code, especially for reads, is still not as optimized as the Scala 2 versions: 1. I couldn't figure out how to generate an anonymous class with typed fields in Scala 3, so I'm putting things in an `Array[Any]` 2. I couldn't figure out how to generate `match` statements, so I generated a `Map[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
Compared Scala 2 (https://github.com/com-lihaoyi/upickle/tree/master/implicits/src-2/upickle/implicits) and Scala 3 (https://github.com/com-lihaoyi/upickle/tree/master/implicits/src-3/upickle/implicits) implementations.
The Scala 2 implementation does not contain any runtime logic at all, and generates a single class at compile time for each case class in question. During reading, temporary values are stored unboxed in typed case class fields, accessed as fields, and put directly into class constructor. During writing, the case class fields are read and put directly into the visitor. That is all that happens at runtime.
The Scala 3 implementation does a bunch of runtime reflection to get fields, puts values boxed in temporary lists and
Map[String, Any]s, does dictionary lookups, and all sorts of other things like that. While it passes tests, it is likely an order of magnitude slower than the Scala 2 implementation, and really should be re-written to be in the same style: do all reflection at compile-time, generating lean and efficient code to do minimal work at runtimeThe text was updated successfully, but these errors were encountered: