mdedetrich opened a new pull request, #857: URL: https://github.com/apache/incubator-pekko/pull/857
This PR is a conservative continuation of https://github.com/apache/incubator-pekko/pull/305, specifically * All of the `@inline` annotations have been removed. As stated by @jrudolph , these annotations were added a long time ago when Akka was targeting older versions of Scala 2 backend which didn't produce optimal bytecode. Currently the `@inline` annotations are creating confusion as its giving the implication that this code should be inlined due to an impediment in the Scala 2 inliner (and to make things worse the Scala 2 inliner isn't even enabled). * Currently the `@inline` annotation doesn't do anything in the Pekko codebase because the Scala 2 inliner isn't enabled anyways (this PR enables that) * The Scala 2 inliner has gotten to a point where its heuristics are at the point that it should be inlining anything that needs to be inlined automatically without the `@inline` annotation. If this is not the case then it should be tackled explicitly * There is an exception for the various converter util methods in `org.apache.pekko.util.*`, this is explained in the next point * For the `org.apache.pekko.util.*` converter functions i.e. `OptionConverters`/`FutureConverters`/`FunctionConverters`/`PartialFunction` the `@inline` annotation has been kept in place and for Scala 3 specifically the `inline `keyword is used (which achieves the same effect). * This is because there is no reason **NOT** to inline these functions as they are just delegation wrappers * All this code will be dropped when Scala 2.12 support is removed anyways * The general idea is that once this is merged into Pekko 1.1.x series we can start enabling the inliner in the other Pekko modules. Of particular import, aside from the standard inline settings would be adding ```scala Seq( "-opt-inline-from:org.apache.pekko.util.OptionConverters**", "-opt-inline-from:org.apache.pekko.util.FutureConverters**", "-opt-inline-from:org.apache.pekko.util.FunctionConverters**", "-opt-inline-from:org.apache.pekko.util.PartialFunction**" ) ``` To the other Pekko modules in the 1.1.x series for Scala 2 inliner settings (with Scala 3 this is not necessary). What this will essentially do is that for Scala 2.12 any calls to the converters (i.e. lets say `org.apache.pekko.util.OptionConverters.toScala`) will directly call the method from [scala-collection-compat](https://github.com/scala/scala-collection-compat) and for Scala 2.13+ it will directly call the Scala stdlib version. This means that aside from a possible performance improvement, when Scala 2.12 support is dropped there won't be any difference in bytecode because `@inline final def toScala[A](o: Optional[A]): Option[A] = scala.jdk.javaapi.OptionConverters.toScala(o)`/`inline final def toScala[A](o: Optional[A]): Option[A] = scala.jdk.javaapi.OptionConverters.toScala(o)` will be inlined away. * It may be that for the Pekko module 1.1.x series they will have to build against Pekko core 1.1.x because while the `@inline` annotations existed in Pekko 1.0.x, the `inline` keyword for Scala 3 in various places was only added in Scala 3. This however shouldn't cause any issues because a Pekko module built against Pekko core 1.1.x would inline all relevant code, so even if a Pekko core 1.0.x is linked at runtime in a Pekko 1.1.x module, all it means is that the Pekko 1.0.x will bring in some unused code. * This is considered safe because all of this converter code won't be changed and is considered entirely stable and when Scala 2.12 is dropped its just going to be calling the exact same method from stdlib anyways. * `@noinline` had to be added in a few places because it caused a test regression, specifically there are tests which call [testNameFromCallStack](https://github.com/apache/incubator-pekko/blob/b0fdac259bd57fdd481483f3fe9a7aec6e1ff38a/actor-testkit-typed/src/main/scala/org/apache/pekko/actor/testkit/typed/internal/TestKitUtils.scala#L82-L83) which (as the name implies) inspects the call stack and the Scala 2 inlining will obviously effect the call stack if it inlines something (there is no Scala 3 inlining here because Scala 3 will only inline if you use the `inline` keyword). Generally speaking its a bit fishy to rely on the call stack for anything but this is testkit related so it gets a free pass. There is a low risk chance that similar issues may be revealed in downstream pekko modules but if it comes up its quite easy to fix this (i.e. need to sprinkle `@noinline` in a few more places). * There is an argument that the default of inlining (i.e. `pekko.no.inline`) should be off rather than on because it can mess with local incremental compilation (although in practice this rarely happens). The reason why I opted for it to be on by default (aside from other Scala projects having inlining on by default) is that since we are doing manual releasing, it would be very easy to forget the inliner flag when making a build. When we get to the point of being able to build release artifacts in CI then I think is a good time to revisit this (i.e. the github action to make a release build would have the inliner flag enabled and locally it will be disabled by default). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
