Remove @inline annotations and enable Scala 2 inliner #857

mdedetrich · 2023-12-23T01:18:53Z

This PR is a conservative continuation of #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/compiler (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 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
```
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 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 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).

He-Pin · 2023-12-23T07:06:59Z

actor/src/main/scala-3/org/apache/pekko/util/ByteIterator.scala

@@ -39,11 +39,11 @@ object ByteIterator {
      extends ByteIterator {
    iterator =>

-    @inline final def len: Int = until - from
+    final def len: Int = until - from


as these code lives in scala-3, should it be inline def?

Well the thing is, I got feedback from the other PR that we should just remove the @inline annotation as its misleading.

I mean I can add the inline keyword there, but then I would also retain the @inline annotation which goes against the point of this PR. The whole premise of this PR is to remove all of the @inline/inline (aside from the util wrappers) and if we can demonstrate performance improvements with inline/@inline then this can be done in future PR's (and this includes @inline annotations removed in this PR).

So I would recommend doing such a change in a future PR with benchmarks that demonstrate an improvement.

Thanks for the detail explain.

He-Pin · 2023-12-23T08:25:04Z

project/PekkoInlinePlugin.scala

+    "-opt:l:inline")
+
+  // Optimizer not yet available for Scala3, see https://docs.scala-lang.org/overviews/compiler-options/optimizer.html
+  private val flagsFor3 = Seq()


Add a //TODO here?

I could, but its outside of our control. I don't know if Scala 3 ever plans to add an inliner like the one that exists in Scala 2.

He-Pin

lgtm

mdedetrich · 2023-12-23T08:49:30Z

lgtm

Thanks, ill wait till the end of the month to get some more reviews.

pjfanning · 2023-12-23T08:53:17Z

actor/src/main/mima-filters/1.1.0.backwards.excludes/option-converters.excludes

+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalLong.toScala")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalLong.toJavaGeneric")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalLong.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalLong.toJavaGeneric$extension")


I am worried about this level of bin incompatibility. Imagine a 3rd party lib that uses Pekko OptionConverters and that is compiled against Pekko 1.0.x. Is this going to mean that they will need to create a new release that compiles against Pekko 1.1.x?

This is all for code that is @InternalStableApi/package private so no 3rd party lib should compile against it.

How about our own modules? I don't want to have to re-release many Pekko modules after a Pekko-Core 1.1 release. Ideally, a module like Pekko HTTP 1.0 will work Pekko 1.0 and Pekko 1.1.

It will still have no adverse effect, quoting from original post and bolding the relevant part

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

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 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.

pjfanning · 2023-12-23T09:50:51Z

I remain skeptical about this. The perf gains are unproven and mainly seem to target the Java user conversions (when most users use Scala).

So my vote is -0. If we end up getting build failures in other Pakko modules, I will be arguing for this to be reverted or partially reverted as needed.

mdedetrich · 2023-12-23T09:57:39Z

I remain skeptical about this. The perf gains are unproven and mainly seem to target the Java user conversions (when most users use Scala).

There definitely are performance improvements as it has inlined code that wasn't inlined before, its just a question of whether the code is in a hotspot or not (I am talking about the inliner in general here, not just org.apache.pekko.util.* specifically, the scala 2 inliner will automatically inline any code that it sees as slow as long as its considered safe)

Also afaik and relevant to org.apache.pekko.util.*, historically most Akka/Pekko users have been Java and not Scala

So my vote is -0. If we end up getting build failures in other Pakko modules, I will be arguing for this to be reverted or partially reverted as needed.

Sure, Im happy with that. Ill take responsibility if there are any issues, but I already tested this locally with pekko-http (including linking against M0-snapshot and 1.0.x) and there aren't any issues.

pjfanning · 2023-12-23T10:27:54Z

Ok. I'm ok with this being merged as long as we revert anything that causes build issues - if those happen - in other Pekko repos.

mdedetrich · 2023-12-30T02:41:09Z

I will merge this so we can get to testing it out in the wild to see if there are any regressions

Remove @inline annotations and enable Scala 2 inliner

55cb35b

mdedetrich force-pushed the enable-inliner-remove-inline-annotation branch from 3fa7dfa to 55cb35b Compare December 23, 2023 02:47

mdedetrich added this to the 1.1.0 milestone Dec 23, 2023

mdedetrich requested review from jrudolph, raboof, gmethvin, He-Pin, seglo, pjfanning and jxnu-liguobin December 23, 2023 05:04

mdedetrich mentioned this pull request Dec 23, 2023

Enable inliner for Scala 2 and add Scala 3 inlining #305

Closed

He-Pin reviewed Dec 23, 2023

View reviewed changes

He-Pin approved these changes Dec 23, 2023

View reviewed changes

pjfanning reviewed Dec 23, 2023

View reviewed changes

mdedetrich mentioned this pull request Dec 28, 2023

perf: Drop the usages of SupervisedGraphStageLogic to reduce allocation. #864

Merged

mdedetrich merged commit 8cb7d25 into apache:main Dec 30, 2023
18 checks passed

mdedetrich deleted the enable-inliner-remove-inline-annotation branch December 30, 2023 02:41

This was referenced Dec 30, 2023

Fix remaining missing inline statements/annotations #889

Merged

Add PekkoInlinePlugin pjfanning/sbt-pekko-build#2

Merged

mdedetrich mentioned this pull request Jan 14, 2024

Set currentVersion in PekkoCoreDependency to 1.1.0-M1 once M1 is released apache/pekko-http#419

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove @inline annotations and enable Scala 2 inliner #857

Remove @inline annotations and enable Scala 2 inliner #857

mdedetrich commented Dec 23, 2023 •

edited

Loading

He-Pin Dec 23, 2023

mdedetrich Dec 23, 2023 •

edited

Loading

He-Pin Dec 23, 2023

He-Pin Dec 23, 2023

mdedetrich Dec 23, 2023

He-Pin left a comment

mdedetrich commented Dec 23, 2023

pjfanning Dec 23, 2023

mdedetrich Dec 23, 2023 •

edited

Loading

pjfanning Dec 23, 2023

mdedetrich Dec 23, 2023 •

edited

Loading

pjfanning commented Dec 23, 2023

mdedetrich commented Dec 23, 2023 •

edited

Loading

pjfanning commented Dec 23, 2023

mdedetrich commented Dec 30, 2023

Remove @inline annotations and enable Scala 2 inliner #857

Remove @inline annotations and enable Scala 2 inliner #857

Conversation

mdedetrich commented Dec 23, 2023 • edited Loading

He-Pin Dec 23, 2023

Choose a reason for hiding this comment

mdedetrich Dec 23, 2023 • edited Loading

Choose a reason for hiding this comment

He-Pin Dec 23, 2023

Choose a reason for hiding this comment

He-Pin Dec 23, 2023

Choose a reason for hiding this comment

mdedetrich Dec 23, 2023

Choose a reason for hiding this comment

He-Pin left a comment

Choose a reason for hiding this comment

mdedetrich commented Dec 23, 2023

pjfanning Dec 23, 2023

Choose a reason for hiding this comment

mdedetrich Dec 23, 2023 • edited Loading

Choose a reason for hiding this comment

pjfanning Dec 23, 2023

Choose a reason for hiding this comment

mdedetrich Dec 23, 2023 • edited Loading

Choose a reason for hiding this comment

pjfanning commented Dec 23, 2023

mdedetrich commented Dec 23, 2023 • edited Loading

pjfanning commented Dec 23, 2023

mdedetrich commented Dec 30, 2023

mdedetrich commented Dec 23, 2023 •

edited

Loading

mdedetrich Dec 23, 2023 •

edited

Loading

mdedetrich Dec 23, 2023 •

edited

Loading

mdedetrich Dec 23, 2023 •

edited

Loading

mdedetrich commented Dec 23, 2023 •

edited

Loading