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/05/01 08:32:35 UTC
[GitHub] [incubator-pekko] mdedetrich opened a new pull request, #305: Add project level inline settings
mdedetrich opened a new pull request, #305:
URL: https://github.com/apache/incubator-pekko/pull/305
This PR is currently prototyping whether inlining is safe within Pekko. Although historically there were issues with scalac inline settings especially in older versions of Scala, afaik they are considered safe for latest versions of Scala 2.12/2.13
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#issuecomment-1786763123
So there may be a bit more work (and more info needs to be added to PR) but its at a stage where people can review it. I have created 2 builds of pekko, one with inline enabled and one without inline enabled
* inlined: https://drive.google.com/file/d/1jfBdLgA_2g-TmFGCxnyvgUQePG6040Rz/view?usp=sharing
* no-inlined: https://drive.google.com/file/d/11ulnOvuNSlwKu9NhurHDSZgwfN-73jI3/view?usp=sharing
You can place either in `~/.ivy2/local/org.apache.pekko` to test them out and of course you can use tools like [jardiff](https://github.com/lightbend-labs/jardiff) to do a diff between the jars to see the bytecode differences.
@He-Pin If you have time doing some initial performance testing would be fantastic. The most critical thing would be if there is a regression (I don't expect this at all) but of course the main point of this PR is to see performance gains, especially for JDK 8/JDK 11 (I know that very recent versions of JDK do a better job of inlining but most people are still using 8/11)
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#issuecomment-1837287373
> 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
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#issuecomment-1837207235
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
```
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#issuecomment-1868212805
I think we can split this PR to make the progress more smoth.
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#discussion_r1377168944
##########
actor/src/main/scala/org/apache/pekko/dispatch/AbstractDispatcher.scala:
##########
@@ -329,7 +329,7 @@ abstract class MessageDispatcher(val configurator: MessageDispatcherConfigurator
/**
* INTERNAL API
*/
- @inline protected[pekko] final val isThroughputDeadlineTimeDefined = throughputDeadlineTime.toMillis > 0
+ protected[pekko] final val isThroughputDeadlineTimeDefined = throughputDeadlineTime.toMillis > 0
Review Comment:
This inline is removed because its pointless, since `isThroughputDeadlineTimeDefined` is a `val` it will immediately get evaluated when class is initialized to a constant.
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#issuecomment-1837396970
> 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 ate you talking about?
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#discussion_r1413011919
##########
actor/src/main/scala/org/apache/pekko/actor/AbstractActorRef.scala:
##########
@@ -0,0 +1,32 @@
+/*
+ * 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.
+ */
+
+/*
+ * Copyright (C) 2009-2022 Lightbend Inc. <https://www.lightbend.com>
+ */
+
+package org.apache.pekko.actor
+
+import org.apache.pekko.util.Unsafe
+
+object AbstractActorRef {
+ private[actor] var cellOffset = 0L
+ private[actor] var lookupOffset = 0L
Review Comment:
Do we really need to rewrite it in Scala?
--
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] Enable inliner for Scala 2 [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#issuecomment-1746485459
Currently blocked by https://github.com/lampepfl/dotty/issues/18646 (although may not be entirely critical).
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#discussion_r1378535643
##########
project/PekkoInlinePlugin.scala:
##########
@@ -0,0 +1,48 @@
+/*
+ * 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:l:inline")
+
+ private val flagsFor213 = Seq(
+ "-opt-inline-from:org.apache.pekko.**",
Review Comment:
> 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?
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "jxnu-liguobin (via GitHub)" <gi...@apache.org>.
jxnu-liguobin commented on PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#issuecomment-1788649990
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?
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#discussion_r1413415709
##########
actor/src/main/scala/org/apache/pekko/actor/AbstractActorRef.scala:
##########
@@ -0,0 +1,32 @@
+/*
+ * 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.
+ */
+
+/*
+ * Copyright (C) 2009-2022 Lightbend Inc. <https://www.lightbend.com>
+ */
+
+package org.apache.pekko.actor
+
+import org.apache.pekko.util.Unsafe
+
+object AbstractActorRef {
+ private[actor] var cellOffset = 0L
+ private[actor] var lookupOffset = 0L
Review Comment:
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](https://www.scala-sbt.org/1.x/docs/Java-Sources.html) 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.
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#issuecomment-1837393953
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.
--
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
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #305: Enable inliner for Scala 2
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#discussion_r1185305997
##########
project/PekkoInlinePlugin.scala:
##########
@@ -0,0 +1,48 @@
+/*
+ * 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:l:inline")
+
+ private val flagsFor213 = Seq(
+ "-opt-inline-from:org.apache.pekko.**",
Review Comment:
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).
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#issuecomment-1863658904
> 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.
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#discussion_r1413015536
##########
actor/src/main/java/org/apache/pekko/dispatch/AbstractBoundedNodeQueue.java:
##########
@@ -45,29 +45,29 @@ protected AbstractBoundedNodeQueue(final int capacity) {
}
private void setEnq(Node<T> n) {
- Unsafe.instance.putObjectVolatile(this, enqOffset, n);
+ Unsafe$.MODULE$.instance().putObjectVolatile(this, enqOffset, n);
Review Comment:
I don't think we should use something like `$.MODULE$`
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "jxnu-liguobin (via GitHub)" <gi...@apache.org>.
jxnu-liguobin commented on PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#issuecomment-1837321458
I have written some artifacts (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?
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#issuecomment-1872454817
Closing this PR, as stated in https://github.com/apache/incubator-pekko/pull/857 if there are further inline opportunities its best to tackle them individually while also demonstrating the performance benefits.
--
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
[GitHub] [incubator-pekko] mdedetrich commented on pull request #305: Enable inliner for Scala 2
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#issuecomment-1545740871
Found a bug with the inliner in Scala 2.13, see https://github.com/scala/bug/issues/12787
--
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
[GitHub] [incubator-pekko] mdedetrich commented on pull request #305: Enable inliner for Scala 2
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#issuecomment-1559239270
A fix for the Scala 2.13 inline bug that I found has already been supplied (see https://github.com/scala/scala/pull/10404). Hopefully it will get merged and then released for Scala 2.13.11
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#issuecomment-1837200968
> @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.
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#issuecomment-1788551880
I have added more info in the original post about the inliner.
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#discussion_r1377169219
##########
actor/src/main/scala/org/apache/pekko/actor/package.scala:
##########
Review Comment:
These methods are deprecated and inlining them causes issues 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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#issuecomment-1789748106
> 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.
--
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
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #305: Enable inliner for Scala 2
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#discussion_r1185305997
##########
project/PekkoInlinePlugin.scala:
##########
@@ -0,0 +1,48 @@
+/*
+ * 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:l:inline")
+
+ private val flagsFor213 = Seq(
+ "-opt-inline-from:org.apache.pekko.**",
Review Comment:
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 etc etc).
--
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
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #305: Enable inliner for Scala 2
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#discussion_r1185305997
##########
project/PekkoInlinePlugin.scala:
##########
@@ -0,0 +1,48 @@
+/*
+ * 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:l:inline")
+
+ private val flagsFor213 = Seq(
+ "-opt-inline-from:org.apache.pekko.**",
Review Comment:
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.
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 etc etc).
--
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
[GitHub] [incubator-pekko] mdedetrich commented on pull request #305: Enable inliner for Scala 2
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#issuecomment-1545617588
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
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#discussion_r1413015882
##########
actor/src/main/scala-3/org/apache/pekko/pattern/CircuitBreakerInline.scala:
##########
@@ -0,0 +1,59 @@
+/*
+ * 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, which was derived from Akka.
+ */
+
+/*
+ * Copyright (C) 2009-2022 Lightbend Inc. <https://www.lightbend.com>
+ */
+
+package org.apache.pekko.pattern
+
+import org.apache.pekko
+import pekko.annotation.InternalApi
+import pekko.util.Unsafe
+
+import scala.concurrent.duration.FiniteDuration
+
+@InternalApi
+private[pekko] trait CircuitBreakerInline { this: CircuitBreaker =>
+
+ /**
+ * Helper method for access to underlying state via Unsafe
+ *
+ * @param oldState Previous state on transition
+ * @param newState Next state on transition
+ * @return Whether the previous state matched correctly
+ */
+ inline final def swapState(oldState: State, newState: State): Boolean =
+ Unsafe.instance.compareAndSwapObject(this, stateOffset, oldState, newState)
+
+ /**
+ * Helper method for accessing underlying state via Unsafe
+ *
+ * @return Reference to current state
+ */
+ inline final def currentState: State =
+ Unsafe.instance.getObjectVolatile(this, stateOffset).asInstanceOf[State]
+
+ /**
+ * Helper method for updating the underlying resetTimeout via Unsafe
+ */
+ inline final def swapResetTimeout(oldResetTimeout: FiniteDuration, newResetTimeout: FiniteDuration): Boolean =
+ Unsafe.instance.compareAndSwapObject(
+ this,
+ resetTimeoutOffset,
+ oldResetTimeout,
+ newResetTimeout)
+
+ /**
+ * Helper method for accessing to the underlying resetTimeout via Unsafe
+ */
+ inline final def currentResetTimeout: FiniteDuration =
Review Comment:
IIRC, Scala 3 will handle the `@inline` annotation, doesn't 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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#discussion_r1413019610
##########
project/PekkoInlinePlugin.scala:
##########
@@ -0,0 +1,48 @@
+/*
+ * 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:l:inline")
+
+ private val flagsFor213 = Seq(
+ "-opt-inline-from:org.apache.pekko.**",
Review Comment:
I mean this one. @mdedetrich
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#issuecomment-1837377249
> 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 that in addition to enabling the inliner in pekko-http build you would actually need to compile against pekko core artifacts that have the inliner enabled.
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich closed pull request #305: Enable inliner for Scala 2 and add Scala 3 inlining
URL: https://github.com/apache/incubator-pekko/pull/305
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#issuecomment-1788338539
Excellent! A little busy at the shopping festival, will take a closer look soon~
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#discussion_r1413415709
##########
actor/src/main/scala/org/apache/pekko/actor/AbstractActorRef.scala:
##########
@@ -0,0 +1,32 @@
+/*
+ * 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.
+ */
+
+/*
+ * Copyright (C) 2009-2022 Lightbend Inc. <https://www.lightbend.com>
+ */
+
+package org.apache.pekko.actor
+
+import org.apache.pekko.util.Unsafe
+
+object AbstractActorRef {
+ private[actor] var cellOffset = 0L
+ private[actor] var lookupOffset = 0L
Review Comment:
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](https://www.scala-sbt.org/1.x/docs/Java-Sources.html) 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.
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "jrudolph (via GitHub)" <gi...@apache.org>.
jrudolph commented on PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#issuecomment-1838068511
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.
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#issuecomment-1837165755
@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)?
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#issuecomment-1868212939
> I think we can split this PR to make the progress more smoth.
Its already been done, see 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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#issuecomment-1868208989
As discussed, PR is closed in favour of https://github.com/apache/incubator-pekko/pull/857 (I made a new PR to keep this one as a reference).
--
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
[GitHub] [incubator-pekko] spangaer commented on a diff in pull request #305: Enable inliner for Scala 2
Posted by "spangaer (via GitHub)" <gi...@apache.org>.
spangaer commented on code in PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#discussion_r1184962375
##########
project/PekkoInlinePlugin.scala:
##########
@@ -0,0 +1,48 @@
+/*
+ * 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:l:inline")
+
+ private val flagsFor213 = Seq(
+ "-opt-inline-from:org.apache.pekko.**",
Review Comment:
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?
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "jxnu-liguobin (via GitHub)" <gi...@apache.org>.
jxnu-liguobin commented on PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#issuecomment-1867863378
> 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.
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#issuecomment-1837315423
@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.
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#discussion_r1413411217
##########
actor/src/main/scala-3/org/apache/pekko/pattern/CircuitBreakerInline.scala:
##########
@@ -0,0 +1,59 @@
+/*
+ * 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, which was derived from Akka.
+ */
+
+/*
+ * Copyright (C) 2009-2022 Lightbend Inc. <https://www.lightbend.com>
+ */
+
+package org.apache.pekko.pattern
+
+import org.apache.pekko
+import pekko.annotation.InternalApi
+import pekko.util.Unsafe
+
+import scala.concurrent.duration.FiniteDuration
+
+@InternalApi
+private[pekko] trait CircuitBreakerInline { this: CircuitBreaker =>
+
+ /**
+ * Helper method for access to underlying state via Unsafe
+ *
+ * @param oldState Previous state on transition
+ * @param newState Next state on transition
+ * @return Whether the previous state matched correctly
+ */
+ inline final def swapState(oldState: State, newState: State): Boolean =
+ Unsafe.instance.compareAndSwapObject(this, stateOffset, oldState, newState)
+
+ /**
+ * Helper method for accessing underlying state via Unsafe
+ *
+ * @return Reference to current state
+ */
+ inline final def currentState: State =
+ Unsafe.instance.getObjectVolatile(this, stateOffset).asInstanceOf[State]
+
+ /**
+ * Helper method for updating the underlying resetTimeout via Unsafe
+ */
+ inline final def swapResetTimeout(oldResetTimeout: FiniteDuration, newResetTimeout: FiniteDuration): Boolean =
+ Unsafe.instance.compareAndSwapObject(
+ this,
+ resetTimeoutOffset,
+ oldResetTimeout,
+ newResetTimeout)
+
+ /**
+ * Helper method for accessing to the underlying resetTimeout via Unsafe
+ */
+ inline final def currentResetTimeout: FiniteDuration =
Review Comment:
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)
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#discussion_r1413411349
##########
actor/src/main/java/org/apache/pekko/dispatch/AbstractBoundedNodeQueue.java:
##########
@@ -45,29 +45,29 @@ protected AbstractBoundedNodeQueue(final int capacity) {
}
private void setEnq(Node<T> n) {
- Unsafe.instance.putObjectVolatile(this, enqOffset, n);
+ Unsafe$.MODULE$.instance().putObjectVolatile(this, enqOffset, n);
Review Comment:
This is the only way to access Scala objects within Java, I have no choice in this?
--
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] Enable inliner for Scala 2 and add Scala 3 inlining [incubator-pekko]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #305:
URL: https://github.com/apache/incubator-pekko/pull/305#discussion_r1377168944
##########
actor/src/main/scala/org/apache/pekko/dispatch/AbstractDispatcher.scala:
##########
@@ -329,7 +329,7 @@ abstract class MessageDispatcher(val configurator: MessageDispatcherConfigurator
/**
* INTERNAL API
*/
- @inline protected[pekko] final val isThroughputDeadlineTimeDefined = throughputDeadlineTime.toMillis > 0
+ protected[pekko] final val isThroughputDeadlineTimeDefined = throughputDeadlineTime.toMillis > 0
Review Comment:
This inline is removed because its pointless, since `isThroughputDeadlineTimeDefined` is a val it will immediately get evaluated when class is initialized to a constant.
--
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