Enable inliner for Scala 2 and add Scala 3 inlining

[mdedetrich]

Currently Pekko uses the @inline annotation and while for earlier versions of Scala this may have done something, with the latest version of Scala 2.12/2.13 the @inline annotation only inlines if you actually enable the inliner via scalac flags, quoting from https://docs.scala-lang.org/overviews/compiler-options/optimizer.html

The @inline annotation only has an effect if the inliner is enabled. It tells the inliner to always try to inline the annotated method or callsite.

While this argument may be considered a bit of a stretch (I don't know the history of Akka here), on first glance it appears that in previous versions of Scala that had the older version of the inliner Akka actually expected @inline to work (we are dealing with performance critical sections of the code after all) and then when Scala was updated which brought in the new inliner written by Lukas Ritz all of the inlining capability via the @inline annotation was lost even because current versions of scalac won't warn if you use @inline without the inliner enabled.

Hence this PR enables inlining for Pekko. While it is true that the original inliner used in older versions of Scala (including in the early minor versions of Scala 2.12/2.13) had many issues which is why it was disabled in Akka, afaik the latest inliner is considered safe especially considering that even modules used within Scala have the inliner enabled (i.e. https://github.com/scala/sbt-scala-module/blob/main/src/main/scala/ScalaModulePlugin.scala#L57). This is also evidenced by the fact that when enabling the inliner, various changes had to be done because its actually a lot more comprehensive when doing checks, more specifically

• The latest version of the inliner actually cross checks between both the JVM bytecode and source to make sure code behaves the correctly. This can cause issues when mixing Java/Scala sources of which Akka/Pekko codebase is one. While this can be solved by changing the compiler order to CompileOrder.JavaThenScala unfortunately the problematic sources in question have cyclic dependencies between Scala and Java (I originally tried to move the problematic modules into a non published core sub project using dependsOn but this only worked with the non cyclic dependencies of which org.apache.pekko.util.Unsafe is the only one). Instead to solve this issue I opted to convert the problematic Java sources to Scala, thankfully the Java code that ended up being converted is trivial and hence even on bytecode level its basically equivalent.
• Some code which used to have @inline had the it removed because inlining wasn't possible (and in the old version of the inliner it would have either been unsafe or just silently not inline anything). An example of this is @inline final def head: Byte = array(from), i.e. @inline had to be removed because it was overriding a non final head method from outside of pekko (in this case head was occurring in Scala stdlib collections). Conversely this also means if we found an @inline annotation for a non final method defined within Pekko, the method was changed to be final (i.e. see https://github.com/apache/incubator-pekko/pull/305/files#diff-3e1ec66267cb7d4e96501b2f01cb462341571959c64387e74eea929d51ef593cR223). At least from what I could tell in such cases not having the method final was an oversight and there was zero expectation that users would override the method.
  • In other cases @inline was removed because there wasn't a proper way to cross-inline some code. This is due to the fact that the way cross Scala inlining (i.e. Scala 2 with @inline annotation and Scala 3 with inline keyword) is achieved by using traits however this can't cover every case. Note that this won't cause a performance regression because as stated earlier the @inline annotation didn't do anything. This can be remedied in future versions of Pekko, i.e. when either https://github.com/mdedetrich/get-inline or https://contributors.scala-lang.org/t/make-scala-2-inline-annotation-work-as-inline-keyword-for-scala-3-with-source-3-0-migration/6286/9?u=mdedetrich comes to fruition.
• Any source files that contained the @inline annotation and were contained within scala-2.13+ folders were copied over to scala-2.13 and scala-3 folder with the scala-3 variant changing @inline to inline (in Scala 3 inline was moved to a keyword and hence the @inline annotation doesn't do anything).
• @noinline had to be added in a few places because it caused a test regression, specifically there are tests which call testNameFromCallStack 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).
• 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

  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 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 is 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.
• 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).

[spangaer]

I guess you get away with this and possibly even want this this with the main pekko repo, but in all other case <sources> is probably the safer choice?

Well even here it's obviously the safer choice, but it would also inline fewer utilities than you aim to achieve?

[mdedetrich]

So one thing to be aware of is that <sources> only works with the sources within an sbt project, i.e.

-opt:inline:<sources>, where the pattern is the literal string <sources>, enables inlining from the set of source files being compiled in the current compiler invocation. This option can also be used for compiling libraries. If the source files of a library are split up across multiple sbt projects, inlining is only done within each project. Note that in an incremental compilation, inlining would only happen within the sources being re-compiled – but in any case, it is recommended to only enable the optimizer in CI and release builds (and to run clean before building).

While ordinarily this is exactly what you want even if you have a multi module sbt build, the reason I opted for org.apache.pekko.** instead is that we can abuse the fact that within context of akka/pekko core module you cannot mix different versions of different artifacts, even patch versions (i.e. you can't mix pekko-actor:1.0.0 with pekko-streams:1.0.1) and Pekko contains a run time class path check to ensure this doesn't happen which was carried over from Akka.

So in summary, since we are mandating that every artifact within the pekko core project has to have the SAME version, it is safe to inline as long as we stick within the org.apache.pekko package. One thing that does need to be confirmed is if every artifact that this sbt build provides follow these rules or if its only some of them (i.e. pekko-actor/pekko-streams vs other modules).

[mdedetrich]

So in summary, since we are mandating that every artifact within the pekko core project has to have the SAME version, it is safe to inline as long as we stick within the org.apache.pekko package. One thing that does need to be confirmed is if every artifact that this sbt build provides follow these rules or if its only some of them (i.e. pekko-actor/pekko-streams vs other modules).

@raboof @jrudolph @He-Pin Can you confirm this, I believe this is the case?

[He-Pin]

I mean this one. @mdedetrich

[mdedetrich]

Currently blocked by what appears to be the inliner being too aggressive, made an issue about it on scala contributors at https://contributors.scala-lang.org/t/scala-2-12-2-13-inliner-is-too-aggressive/6191

[mdedetrich]

Found a bug with the inliner in Scala 2.13, see scala/bug#12787

[mdedetrich]

A fix for the Scala 2.13 inline bug that I found has already been supplied (see scala/scala#10404). Hopefully it will get merged and then released for Scala 2.13.11

[mdedetrich]

I have added more info in the original post about the inliner.

[jxnu-liguobin]

Looks good, I can start testing Java11 this weekend. Is the scope of the test to add or remove inline methods, or is part of it just fine?

[mdedetrich]

Looks good, I can start testing Java11 this weekend. Is the scope of the test to add or remove inline methods, or is part of it just fine?

I would be more curious about the performance impacts rather than testing correctness, after all that is the whole point of the inliner.

[mdedetrich]

@He-Pin @jxnu-liguobin Did you manage to have a look into this? I was thinking of merging this so its ready for milestone 1 so i can get properly tested (@pjfanning what are your thoughts on this as well)?

[He-Pin]

@He-Pin @jxnu-liguobin Did you manage to have a look into this? I was thinking of merging this so its ready for milestone 1 so i can get properly tested (@pjfanning what are your thoughts on this as well)?

I will take another look at it on Sunday.

[pjfanning]

A test failed

2023-11-01T08:51:33.1333372Z [11-01 08:51:33.117] �[0m[�[0m�[0minfo�[0m] �[0m�[0m�[31m- must pass cancellation upstream across remoting before elements has been emitted *** FAILED *** (5 milliseconds)�[0m�[0m
2023-11-01T08:51:33.1338551Z [11-01 08:51:33.117] �[0m[�[0m�[0minfo�[0m] �[0m�[0m�[31m  java.lang.AssertionError: assertion failed: expected Done, found Failure(org.apache.pekko.stream.RemoteStreamRefActorTerminatedException: Remote target receiver of data Some(Actor[pekko://StreamRefsSpec/system/Materializers/StreamSupervisor-2568/$$m-SourceRef-8#-437783934]) terminated. Local stream terminating, message loss (on remote side) may have happened.)�[0m�[0m
2023-11-01T08:51:33.1342827Z [11-01 08:51:33.117] �[0m[�[0m�[0minfo�[0m] �[0m�[0m�[31m  at scala.Predef$.assert(Predef.scala:279)�[0m�[0m
2023-11-01T08:51:33.1344798Z [11-01 08:51:33.117] �[0m[�[0m�[0minfo�[0m] �[0m�[0m�[31m  at org.apache.pekko.testkit.TestKitBase.expectMsg_internal(TestKit.scala:473)�[0m�[0m
2023-11-01T08:51:33.1347487Z [11-01 08:51:33.117] �[0m[�[0m�[0minfo�[0m] �[0m�[0m�[31m  at org.apache.pekko.testkit.TestKitBase.expectMsg(TestKit.scala:449)�[0m�[0m
2023-11-01T08:51:33.1349643Z [11-01 08:51:33.117] �[0m[�[0m�[0minfo�[0m] �[0m�[0m�[31m  at org.apache.pekko.testkit.TestKitBase.expectMsg$(TestKit.scala:449)�[0m�[0m
2023-11-01T08:51:33.1351749Z [11-01 08:51:33.118] �[0m[�[0m�[0minfo�[0m] �[0m�[0m�[31m  at org.apache.pekko.testkit.TestKit.expectMsg(TestKit.scala:982)�[0m�[0m

[mdedetrich]

A test failed

This appears to be unrelated, if you look at the source code i.e.
https://github.com/apache/incubator-pekko/blob/b0fdac259bd57fdd481483f3fe9a7aec6e1ff38a/stream-tests/src/test/scala/org/apache/pekko/stream/scaladsl/StreamRefsSpec.scala#L361-L362

specifically against the linked issue https://github.com/akka/akka/issues/30844 you can see that upstream its also reported as a flaky test

[mdedetrich]

@pjfanning I just rebased on main and force pushed (triggering another run) and there aren't any test failures so I can confirm that its indeed an irrelevant flaky test.

[jxnu-liguobin]

I have written some tests (without using artifacts) that do not show any performance impact.

I have discussed this with Kerr before. Is it necessary to use packaged artifacts for testing?

[mdedetrich]

I have written some tests (without using artifacts) that do not show any performance impact.

This is both good and bad news depending on how it was tested, it does mean that potentially akka has a lot of hotspots already taken care of (not surprising) but I would also be curious as to what JDK was used.

Is it necessary to use packaged artifacts for testing?

Nope, the packages were more designed to test it with projects like pekko-http however I would raise what I said earlier in the OP, specifically

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.

i.e. To enable full inlining in other modules like pekko-http, in addition to enabling the inliner in pekko-http build you would actually need to compile against pekko core artifacts that have been published with the inliner feature.

[He-Pin]

Do we really need to rewrite it in Scala?

[mdedetrich]

Yes, quoting from OP

The latest version of the inliner actually cross checks between both the JVM bytecode and source to make sure code behaves the correctly. This can cause issues when mixing Java/Scala sources of which Akka/Pekko codebase is one. While this can be solved by changing the compiler order to CompileOrder.JavaThenScala unfortunately the problematic sources in question have cyclic dependencies between Scala and Java (I originally tried to move the problematic modules into a non published core sub project using dependsOn but this only worked with the non cyclic dependencies of which org.apache.pekko.util.Unsafe is the only one). Instead to solve this issue I opted to convert the problematic Java sources to Scala, thankfully the Java code that ended up being converted is trivial and hence even on bytecode level its basically equivalent.

[He-Pin]

I don't think we should use something like $.MODULE$

[mdedetrich]

This is the only way to access Scala objects within Java, I have no choice in this?

[He-Pin]

IIRC, Scala 3 will handle the @inline annotation, doesn't it?

[mdedetrich]

No it doesn't, it ignores it (see https://contributors.scala-lang.org/t/make-scala-2-inline-annotation-work-as-inline-keyword-for-scala-3-with-source-3-0-migration/6286/9?u=mdedetrich)

[He-Pin]

My feedback:

1. I don't think we should rewirte those Java code to Scala, we maybe migrate those to VarHandle later. cats-effects doesn't too https://github.com/typelevel/cats-effect/tree/series/3.x/core/jvm/src/main/java/cats/effect/unsafe
2. We need split this PR:

• the @inline annotation modification and inline addition for Scala 3.
• the java to scala rewritten, we may not need it.
• the plugin itself.

[mdedetrich]

My feedback:

1. I don't think we should rewirte those Java code to Scala, we maybe migrate those to VarHandle later. cats-effects doesn't too https://github.com/typelevel/cats-effect/tree/series/3.x/core/jvm/src/main/java/cats/effect/unsafe

We have to because the inliner doesn't work (as in it will fail to compile). This is because the inliner needs to reference bytecode with .scala files and so existing @inline anmotations won't work with rewriting these specific

2. We need split this PR:

• the @inline annotation modification and inline addition for Scala 3.

What's reasoning behind this (not sure what this is meant to achieve)? The splitting out of traits for different Scala versions is intended to be in a single PR?

• the java to scala rewritten, we may not need it.

As mentioned before it was definitely necessary when I was writing the PR. Some of it may no longer be necessary as stuff did move around as PR was being worked on over time.

• the plugin itself.

Which plugin are you talking about?

[jrudolph]

I said it before but I'll repeat here, I think changes like this are work-intensive, high risk, and low reward, and in summary most likely not worth doing.

Maybe the @inline annotation is misleading that there might or should be a performance optimization potential that can somehow be realized. But it's important to understand that most of the code that contained @inline annotations were written more than a decade ago ~ in Scala 2.9.x time where expectations and realities of the backend and of the JDKs in use were vastly different.

In general, the potential of micro-optimizations is already small in mature code, but this kind is especially small because the Scala backend is not the last backend in the chain but there's the JDK behind it that will do any easy and static inlining with a much higher hit-rate than you can expect from the Scala backend.

Basically, the only place where you can expect wins through inlining early in the pipeline is highly abstract, megamorphic code that requires explicit code expansion (explosion) in a way that the JIT will never do (i.e. expand megamorphic call sites leading to potential exponential code blowup). In some case, the fact that a call-site is mostly monomorphic but looks megamorphic can mislead the JIT, or you want a single call-site to be optimized. These are the places where macros like parboiled2 or loop fusion solutions can shine, but these are very specific cases.

In particular, the Actor backend is and should not be such a case.

As a follow up, I think the most worthwhile changes here would be to focus purely on safe refactoring and modernization of the low-level code, keeping a close look on keeping performance. Only when that is done a risky change like enabling the inliner can be evaluated. But even then, it would make much more sense to start such a work on the basis of real world performance profiles which often show when JIT inlining was not successful or might be useful.

[mdedetrich]

Maybe the @inline annotation is misleading that there might or should be a performance optimization potential that can somehow be realized. But it's important to understand that most of the code that contained @inline annotations were written more than a decade ago ~ in Scala 2.9.x time where expectations and realities of the backend and of the JDKs in use were vastly different.

If this is the case then maybe its best to just remove all of the @inline annotations (aside from the obvious cases which are inside of org.apache.pekko.util.* where there is no reason not to inline). Most of the complexity of the PR is arising from making the @inline annotations work on both Scala 2/Scala 3 and apparently according to the author of the Scala 2 inliner, its heuristics are already good enough that it would be quite surprising if it didn't catch code that should be inlined.

Basically, the only place where you can expect wins through inlining early in the pipeline is highly abstract, megamorphic code that requires explicit code expansion (explosion) in a way that the JIT will never do (i.e. expand megamorphic call sites leading to potential exponential code blowup). In some case, the fact that a call-site is mostly monomorphic but looks megamorphic can mislead the JIT, or you want a single call-site to be optimized. These are the places where macros like parboiled2 or loop fusion solutions can shine, but these are very specific cases.

Agreed and this is the main intent of enabling the inliner. I had a presumption that there is a reason why the Akka codebase was littered with these @inline annotations but it appears that at least for modern Scala backends its largely a red herring.

It seems like the best way to go is to actually remove all of the @inline annotations (aside from the exception I listed earlier) while still enabling the Scala 2 inliner to see if we get some easy wins and make sure we diagnose what those wins are. Do note that the Scala 2 inliner is enabled for some of the most critical/performance sensitive Scala OS code, i.e. the Scala 2 compiler itself as well as Zinc so I see this really as a case of having some free lunch with almost no risk.

@jrudolph @He-Pin @jxnu-liguobin Would the buy in be better for this feature if I change the scope of the PR to do what I stated (i.e. remove all @inline annotations aside from org.apache.pekko.util.* while still enabling the Scala 2 inliner)?

[jxnu-liguobin]

Would the buy in be better for this feature if I change the scope of the PR to do what I stated

I think so, or at least it will keep people more focused.
btw, Our team started using pekko this week.

[mdedetrich]

As discussed, PR is closed in favour of #857 (I made a new PR to keep this one as a reference).

[He-Pin]

I think we can split this PR to make the progress more smoth.

[mdedetrich]

I think we can split this PR to make the progress more smoth.

Its already been done, see #857

[mdedetrich]

Closing this PR, as stated in #857 if there are further inline opportunities its best to tackle them individually while also demonstrating the performance benefits.