apache
consider marking some functions as `@noinline` to fix issues with using Kamon
Pekko 1.1 enables Scala 2 inlining.
https://www.baeldung.com/scala/inline-noinline-annotations
This has caused issues with using Kamon for monitoring Pekko 1.1. https://github.com/kamon-io/Kamon/issues/1352
One option might be to mark some of the functions that Kamon relies on instrumenting as @noinline.
To me this is acceptable as a temporary solution as long as the number of methods we have to mark as @noinline is reasonable (as a rough ballpark I would say < 10) and that ontop of this we also have a path forward to provide official instrumentation/hooks.
The thing I want to avoid is that we make a release with a some methods marked @noinline and then at some point in time this isn't enough and then we end up adding even more methods with @noinline creating a roundabout back and fourth circus.
It seems like the Scala 2 compiler can inline any functions that are marked as final. Does anyone know what happens if a class is marked as final, does that mean all of its function are regarded by the inliner as being final and therefore inline friendly?
It seems like the Scala 2 compiler can inline any functions that are marked as
final. Does anyone know what happens if a class is marked asfinal, does that mean all of its function are regarded by the inliner as being final and therefore inline friendly?
@lrytz Can you quickly answer this one?
I agree with "One option might be to mark some of the functions that Kamon relies on instrumenting as @noinline."
Creating an un-instrumentation library is not what we want, but we need to consider what kind of method can not be inline, we should inline those method irrelevant, but shouldn't inline for the lifecycle method, and execution method.
I've opened https://github.com/apache/pekko/discussions/1487 to discuss a long term solution. I still think this @noinline solution is something that we should do for Pekko 1.1.2. We have broken kamon-pekko by enabling Scala 2 compiler inlining in Pekko 1.1. Maintaining these @noinline annotations indefinitely will be an issue but I think we can maintain them on the 1.1 branch and find a better solution for future releases.
@lrytz Thanks for your assistance. If you have time, would you be able my observation below?
One of the Kamon instrumentation issues with the Pekko code compiled with Scala 2 inlining enabled appears to be with final case class ThreadPoolConfig and Kamon instrumentation looking to instrument the derived copy function that gets added to all case classes. I tried adding @noinline as an annotation at the case class level but that did not appear to help. I might try declaring an @noinline override def copy on this class but I'm wondering if there is a better way to do this.
I see a great solution.
In the -opt:inline setting (see -opt:help) you can declare where to inline from, but only to the class level, not to individual methods. That could probably be changed so that !**.copy would disallow inlining any copy methods.
Creating an un-instrumentation library is not what we want, but we need to consider what kind of method can not be inline, we should inline those method irrelevant, but shouldn't inline for the lifecycle method, and execution method.
If the Scala compiler determines a method should be inlined, there isn't any reason to prevent this inlining aside from the very rare chance that it creates a performance regression (which can theoritically happen but haven't seen any evidence of it so far). The Scala compiler will only inline methods that is safe to do so, even if its for lifecycle/execution methods.
The case we have with instrumentation right now is not a typical one, historically Akka hasn't had official hooks/api for instrumentation and because of that people that wanted to add instrumentation had to resort to methods such as AOP (aspect orientated programming) and/or JVM bytecode/stack inspection at which point all bets are off (inlining or not), i.e. someone could have just refactored the methods in question (which they are free to do so since they are internal/private) and it would have also broken kamon.
tl;dr We should make an official API for this at some point.
I was in the middle of typing:
Copy methods in general will always break mixin context patterns in instrumentation. I don't think they're ever going to be hot paths. Maybe we can find a way to disable inline for copy? That hugely limits the scope of downstream breakage opportunities..
And then i thought 'but wait, if we just have an empty Map[String, Any] baggage type field on objects that could be sensibly instrumented... Well wouldn't that be interesting? No need to block inline on copy, maybe a route to bindings....
I was in the middle of typing:
Copy methods in general will always break mixin context patterns in instrumentation. I don't think they're ever going to be hot paths. Maybe we can find a way to disable inline for copy? That hugely limits the scope of downstream breakage opportunities..
And then i thought 'but wait, if we just have an empty Map[String, Any] baggage type field on objects that could be sensibly instrumented... Well wouldn't that be interesting? No need to block inline on copy, maybe a route to bindings....
@hughsimpson I think it best to use https://github.com/apache/pekko/discussions/1487 to discuss further changes. My preference would be to use use the Pekko Event Stream.
I've opened #1499 to discuss options to support Context Propagation.