You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "mdedetrich (via GitHub)" <gi...@apache.org> on 2023/12/23 01:18:55 UTC
[PR] Remove @inline annotations and enable Scala 2 inliner [incubator-pekko]
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Remove @inline annotations and enable Scala 2 inliner [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #857:
URL: https://github.com/apache/incubator-pekko/pull/857#issuecomment-1868256126
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.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Remove @inline annotations and enable Scala 2 inliner [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #857:
URL: https://github.com/apache/incubator-pekko/pull/857#discussion_r1435522109
##########
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
Review Comment:
Well the thing is, I got feedback from the other PR that we should just remove the `@inline` annotation/`inline` keyword 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 premise 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.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Remove @inline annotations and enable Scala 2 inliner [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #857:
URL: https://github.com/apache/incubator-pekko/pull/857#issuecomment-1868263615
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.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Remove @inline annotations and enable Scala 2 inliner [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #857:
URL: https://github.com/apache/incubator-pekko/pull/857#discussion_r1435537916
##########
actor/src/main/mima-filters/1.1.0.backwards.excludes/option-converters.excludes :
##########
@@ -0,0 +1,30 @@
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJava$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJava")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJavaPrimitive")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJava$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toScala")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toJavaPrimitive")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toScala")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toJavaGeneric")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toScala")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toJavaGeneric")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalLong.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalLong.toScala$extension")
+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")
Review Comment:
This is all for code that is `@InternalApi`/package private so no 3rd party lib should compile against.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Remove @inline annotations and enable Scala 2 inliner [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #857:
URL: https://github.com/apache/incubator-pekko/pull/857#discussion_r1435530262
##########
project/PekkoInlinePlugin.scala:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * license agreements; and to You under the Apache License, version 2.0:
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * This file is part of the Apache Pekko project, derived from Akka.
+ */
+
+import sbt.Keys._
+import sbt._
+import sbt.plugins.JvmPlugin
+
+object PekkoInlinePlugin extends AutoPlugin {
+ override def trigger: PluginTrigger = allRequirements
+
+ override def requires: Plugins = JvmPlugin
+
+ val enabled = !sys.props.contains("pekko.no.inline")
+
+ private val flagsFor212 = Seq(
+ "-opt-inline-from:org.apache.pekko.**",
+ "-opt-inline-from:<sources>",
+ "-opt:l:inline")
+
+ private val flagsFor213 = Seq(
+ "-opt-inline-from:org.apache.pekko.**",
+ "-opt-inline-from:<sources>",
+ "-opt:l:inline")
+
+ // Optimizer not yet available for Scala3, see https://docs.scala-lang.org/overviews/compiler-options/optimizer.html
+ private val flagsFor3 = Seq()
Review Comment:
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.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Remove @inline annotations and enable Scala 2 inliner [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #857:
URL: https://github.com/apache/incubator-pekko/pull/857#discussion_r1435522109
##########
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
Review Comment:
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 premise 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.
So I would recommend doing such a change in a future PR with benchmarks that demonstrate an improvement.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Remove @inline annotations and enable Scala 2 inliner [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #857:
URL: https://github.com/apache/incubator-pekko/pull/857#discussion_r1435541494
##########
actor/src/main/mima-filters/1.1.0.backwards.excludes/option-converters.excludes :
##########
@@ -0,0 +1,30 @@
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJava$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJava")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJavaPrimitive")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJava$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toScala")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toJavaPrimitive")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toScala")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toJavaGeneric")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toScala")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toJavaGeneric")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalLong.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalLong.toScala$extension")
+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")
Review Comment:
It will still have no 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
```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.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Remove @inline annotations and enable Scala 2 inliner [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #857:
URL: https://github.com/apache/incubator-pekko/pull/857#discussion_r1435541494
##########
actor/src/main/mima-filters/1.1.0.backwards.excludes/option-converters.excludes :
##########
@@ -0,0 +1,30 @@
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJava$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJava")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJavaPrimitive")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJava$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toScala")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toJavaPrimitive")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toScala")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toJavaGeneric")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toScala")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toJavaGeneric")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalLong.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalLong.toScala$extension")
+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")
Review Comment:
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
```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.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Remove @inline annotations and enable Scala 2 inliner [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #857:
URL: https://github.com/apache/incubator-pekko/pull/857#discussion_r1435536641
##########
actor/src/main/mima-filters/1.1.0.backwards.excludes/option-converters.excludes :
##########
@@ -0,0 +1,30 @@
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJava$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJava")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJavaPrimitive")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJava$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toScala")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toJavaPrimitive")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toScala")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toJavaGeneric")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toScala")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toJavaGeneric")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalLong.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalLong.toScala$extension")
+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")
Review Comment:
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 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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Remove @inline annotations and enable Scala 2 inliner [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #857:
URL: https://github.com/apache/incubator-pekko/pull/857#discussion_r1435510272
##########
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
Review Comment:
as these code lives in scala-3, should it be `inline def`?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Remove @inline annotations and enable Scala 2 inliner [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich merged PR #857:
URL: https://github.com/apache/incubator-pekko/pull/857
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Remove @inline annotations and enable Scala 2 inliner [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #857:
URL: https://github.com/apache/incubator-pekko/pull/857#issuecomment-1868244635
> lgtm
Thanks, ill wait till the end of the month to get some more reviews.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Remove @inline annotations and enable Scala 2 inliner [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #857:
URL: https://github.com/apache/incubator-pekko/pull/857#issuecomment-1872433050
I will merge this so we can get to testing it out in the wild to see if there are any regressions
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Remove @inline annotations and enable Scala 2 inliner [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #857:
URL: https://github.com/apache/incubator-pekko/pull/857#discussion_r1435541494
##########
actor/src/main/mima-filters/1.1.0.backwards.excludes/option-converters.excludes :
##########
@@ -0,0 +1,30 @@
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJava$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJava")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJavaPrimitive")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJava$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toScala")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toJavaPrimitive")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toScala")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toJavaGeneric")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toScala")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toJavaGeneric")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalLong.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalLong.toScala$extension")
+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")
Review Comment:
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
```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.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Remove @inline annotations and enable Scala 2 inliner [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #857:
URL: https://github.com/apache/incubator-pekko/pull/857#issuecomment-1868257372
> 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 the `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, 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.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Remove @inline annotations and enable Scala 2 inliner [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #857:
URL: https://github.com/apache/incubator-pekko/pull/857#discussion_r1435529769
##########
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
Review Comment:
Thanks for the detail explain.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Remove @inline annotations and enable Scala 2 inliner [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #857:
URL: https://github.com/apache/incubator-pekko/pull/857#discussion_r1435537916
##########
actor/src/main/mima-filters/1.1.0.backwards.excludes/option-converters.excludes :
##########
@@ -0,0 +1,30 @@
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJava$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJava")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJavaPrimitive")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJava$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toScala")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toJavaPrimitive")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toScala")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toJavaGeneric")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toScala")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toJavaGeneric")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalLong.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalLong.toScala$extension")
+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")
Review Comment:
This is all for code that is `@InternalApi`/package private so no 3rd party lib should compile against it.
##########
actor/src/main/mima-filters/1.1.0.backwards.excludes/option-converters.excludes :
##########
@@ -0,0 +1,30 @@
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJava$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJava")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJavaPrimitive")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJava$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toScala")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toJavaPrimitive")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toScala")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toJavaGeneric")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toScala")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toJavaGeneric")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalLong.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalLong.toScala$extension")
+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")
Review Comment:
This is all for code that is `@InternalStableApi`/package private so no 3rd party lib should compile against it.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Remove @inline annotations and enable Scala 2 inliner [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #857:
URL: https://github.com/apache/incubator-pekko/pull/857#discussion_r1435538682
##########
actor/src/main/mima-filters/1.1.0.backwards.excludes/option-converters.excludes :
##########
@@ -0,0 +1,30 @@
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJava$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJava")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJavaPrimitive")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJava$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOption.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toScala")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toJavaPrimitive")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptional.toJavaPrimitive$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toScala")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toJavaGeneric")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalDouble.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toScala")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toJavaGeneric")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toScala$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalInt.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalLong.toJavaGeneric$extension")
+ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.pekko.util.OptionConverters#RichOptionalLong.toScala$extension")
+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")
Review Comment:
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.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Remove @inline annotations and enable Scala 2 inliner [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #857:
URL: https://github.com/apache/incubator-pekko/pull/857#discussion_r1435530047
##########
project/PekkoInlinePlugin.scala:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * license agreements; and to You under the Apache License, version 2.0:
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * This file is part of the Apache Pekko project, derived from Akka.
+ */
+
+import sbt.Keys._
+import sbt._
+import sbt.plugins.JvmPlugin
+
+object PekkoInlinePlugin extends AutoPlugin {
+ override def trigger: PluginTrigger = allRequirements
+
+ override def requires: Plugins = JvmPlugin
+
+ val enabled = !sys.props.contains("pekko.no.inline")
+
+ private val flagsFor212 = Seq(
+ "-opt-inline-from:org.apache.pekko.**",
+ "-opt-inline-from:<sources>",
+ "-opt:l:inline")
+
+ private val flagsFor213 = Seq(
+ "-opt-inline-from:org.apache.pekko.**",
+ "-opt-inline-from:<sources>",
+ "-opt:l:inline")
+
+ // Optimizer not yet available for Scala3, see https://docs.scala-lang.org/overviews/compiler-options/optimizer.html
+ private val flagsFor3 = Seq()
Review Comment:
Add a //TODO here?
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Remove @inline annotations and enable Scala 2 inliner [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #857:
URL: https://github.com/apache/incubator-pekko/pull/857#discussion_r1435522109
##########
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
Review Comment:
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.
--
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: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org