ZLayer.make fails with ClassTooLarge #8855
Comments
|
/bounty $100 for reproducer and fix. |
💎 $100 bounty • ZIOSteps to solve:
Thank you for contributing to zio/zio! Add a bounty • Share on socials
|
|
I took a look. It seems to me, that the code generated by the Scala 3 macro hits a limit of max identifiers for a class. But my gut feeling says, that you might have hit a hard limitation of the Scala 3 compiler. Because the generated code I was looking at looked like it could not work with less identifiers. It might be possible to generate the code in a way that these identifiers are not all in the same class, but this seems complicated and maybe not worth it? My guess is, that this only happens when the layer is build via the macro. So you could maybe wire it by hand as a workaround. Or have maybe a split of smaller argument lists for I would suggest, that you also open an issue for the Scala compiler after you have a branch with a reproducer. |
|
Here's a tree with the bug: https://github.com/dasch-swiss/dsp-api/tree/bug/class-too-large. |
|
It sounds like a macro bug rather than a compiler bug, because it probably just hit a limit as you said. What could be improved in the message? I'm not familiar enough with macro/compiler interaction to decide whether making a compiler bug ticket is warranted. (Should the output of the macro be dumped? No idea.) Indeed, splitting it it into smaller chunks did help as a workaround. |
|
I don't think we can detect if the class is too big during macro expansion - first of all we don't know what what's beside our macro. The only thing I see it done is writing some heuristic that will show warning/error when we generate too much stuff (whatever that would mean).
The only thing Scala compiler may do is catch that exception and print that class is too large in nicer way? |
|
@andrzejressel might be true. But at the end, we can only optimize how many identifiers are generated by our macro. And I think that this is either optimal or close to it. What we could do though is, to have something like a identifier, that is a new instance of something, that holds a number of references to layer so that we reduce the number of identifiers on the top level, which I guess is the issue here. @jdegoes any opinion? |
|
I wouldn't go that far - instead I would calculate gas (I saw this name in blockchain and WASM interpreters) of macro - However I need to check what that would be (number of methods, method length - I haven't run the example code myself) - in any case after some threshhold is passed (3/4 of JVM limit) I would show warning to the user that this method has to be split to separate classes. Zio-intellij could have an action to do it automatically - it would just split the list/graph in the middle and calculate layer inputs and outputs. |
|
I wouldn't say won't fix, but no giant motivation to fix. Instead of generating identifiers, we could create an untyped array and access different parts of it (e.g.). So there are ways to fix it but it is more complicated. |
|
I don't think passing everything by name is close to optimal (also generating tags and traces): final def ++[E1 >: E, RIn2, ROut1 >: ROut, ROut2](
that: => ZLayer[RIn2, E1, ROut2]
)(implicit tag: EnvironmentTag[ROut2]): ZLayer[RIn with RIn2, E1, ROut1 with ROut2] =
self.zipWithPar(that)(_.union[ROut2](_))
final def to[E1 >: E, ROut2](that: => ZLayer[ROut, E1, ROut2])(implicit
trace: Trace
): ZLayer[RIn, E1, ROut2] =
self >>> that |
|
Here is example that throws the same error: https://scastie.scala-lang.org/2Sy2v9mtQiyd0UxRR4mgAQ |
It compiles if I change it to: final def andEager[E1 >: E, RIn2, ROut1 >: ROut, ROut2](
that: ZLayer[RIn2, E1, ROut2]
): ZLayer[RIn with RIn2, E1, ROut1 with ROut2] =
self.zipWithPar(that)(_.unionAll[ROut2](_))
final def toEager[E1 >: E, ROut2](that: ZLayer[ROut, E1, ROut2]): ZLayer[RIn, E1, ROut2] = {
implicit val trace: Trace = Trace.empty
self >>> that
} |
|
@joroKr21 good catch I was focusing on the generated layers. We can't change the signature of the existing methods, but we can add new (package private?) methods to be used in the macro. |
The problem with this is that it used |
Why is it a problem? |
|
Because And to prune the environment we need the |
|
Perhaps the issue is not the Also I'm not even sure there is much benefit in excluding the final def andEager[E1 >: E, RIn2, ROut1 >: ROut, ROut2](
that: ZLayer[RIn2, E1, ROut2]
): ZLayer[RIn with RIn2, E1, ROut1 with ROut2] = {
val that0 = that.map(_.prune[ROut2])
self.zipWithPar(that0)(_.unionAll[ROut2](_))
} |
|
I would expect |
|
If you insist on pruning, you could prune all input layers once and be done with it. No need to prune when combining. |
I don't think it's as simple as you describe it. Here's an example where this will be problematic: import zio.*
trait Foo
final class Foo1 extends Foo
final class Foo2 extends Foo
final class Bar
val fooBarLayer: ULayer[Foo & Bar] = ZLayer.succeed(new Foo1) ++ ZLayer.succeed(new Bar)
val barLayer: ULayer[Bar] = fooBarLayer // Environment contains both Foo1 and Bar!
val foo2Layer: ULayer[Foo] = ZLayer.succeed(new Foo2)
// barLayer isn't pruned, so we end up with Foo1 and Bar instead of Foo2 and Bar
val combined: ULayer[Foo & Bar] = foo2Layer ++ barLayerBased on how we currently do things, we'll have |
|
Concatenation does pruning (by "default") because otherwise of surprises: val clock: ZLayer[Any, Nothing, Clock] = ???
val logging: ZLayer[Any, Nothing, Logging] = ???
clock ++ logging : ZLayer[Any, Nothing, Clock & Logging]Makes sense, right? Until you see: val clock: ZLayer[Any, Nothing, Clock] = ???
val loggingAndStompingClock: ZLayer[Any, Nothing, Logging] = ???
val logging: ZLayer[Any, Nothing, Logging] = loggingAndStompingClock
clock ++ logging : ZLayer[Any, Nothing, Clock & Logging]The issue is that due to covariance, Scala lets you forget part or all of the output of a layer. Then when you combine them, you look at the types to gain some intuition of where your services are coming from. Which, in the absence of pruning, can be completely wrong and difficult to diagnose. |
How do you know if the code generated by |
|
Let me summarize:
And a question that came to my mind: Is it actually necessary to generate all of the identifiers that the macro creates into the parent class? These things can never be accessed publicly by design. Should this issue be solved on the side of the compiler? |
We don't know that - in fact, we don't care about the order. That's the point of pruning; by keeping only the services that are statically known to be combined within the layer we remove any other services that might be there. And in the case of a user error, where more than 1 layer provide the same service, an "Ambiguous layer" compiling error is raised |
So if the order of operations is not well defined from the user's PoV, why would the right overwriting the left matter? It's an implementation detail. |
|
I just realised this compiles fine on Scala 2, so there must be some difference in the generated Scala code and/or bytecode. |
|
@joroKr21 see the test case here: #8901 (review) |
|
@joroKr21 I've checked it with some auto generated code: 400KB Scala 3 vs 300 KB Scala 2.13
|
|
💡 @joroKr21 submitted a pull request that claims the bounty. You can visit your bounty board to reward. |
We have recently migrated the dsp-api from Scala 2 to Scala 3. This is not a Scala 3 only problem I have seen the error with the dsp-api code base on Scala 2 already. |
|
That's interesting, I guess it's worth it to make the same changes in Scala 2. |
This is with
val ScalaVersion = "3.3.3"andval ZioVersion = "2.0.22"on an open source application, but I couldn't make a showcase within reasonable time, it is quite difficult. I couldn't figure out a way to make any failing cases from scratch. I managed to refactor it in a way that cancels the problem for now, but that also takes quite a bit of time, so I'm making this issue because maybe the bug is easier to test on your end. Our layer definitions look like this: LayersLive.scala, though I don't expect you to build this project yourself (although you can of course), but I would also have to publish the branch.The text was updated successfully, but these errors were encountered: