Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mark stop() method as noinline #1485

Merged
merged 2 commits into from
Sep 25, 2024
Merged

mark stop() method as noinline #1485

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
82991a5
mark stop() method as noinline
pjfanning Sep 17, 2024
674740a
Update Dispatch.scala
pjfanning Sep 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion actor/src/main/scala/org/apache/pekko/actor/dungeon/Dispatch.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,9 @@ private[pekko] trait Dispatch { this: ActorCell =>
catch handleException

// ➡➡➡ NEVER SEND THE SAME SYSTEM MESSAGE OBJECT TO TWO ACTORS ⬅⬅⬅
final def stop(): Unit =
// this function is marked `noinline` because Kamon and other metrics libraries
// instrument this function
@noinline final def stop(): Unit =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why this method has been inline. Does us toggle something on? If does, for those method like start, resume, suspend shouldn't be inline too.

Copy link
Contributor

@jrudolph jrudolph Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Scala compiler is free to inline methods when it is sound. This is the case, when the final receiver of a call can be determined. Whether or not that has any performance impact is always hard to say. In general, the JVM JIT compilation will also do heavy inlining and may in many cases do the same kind of optimizations at runtime. In other cases, like e.g. for higher-order methods, the Scala compiler might be able to generate significantly less code by earlier inlining, which also helps JIT compilations.

That said, the point of adding @noinline here is that some telemetry tools like Kamon instrument internal APIs to do their work. This has mainly historic reasons. In the future, it could be better to introduce official hooks for these kinds of use cases, so that it is easier to track what the public telemetry API is instead of relying on keeping some internal APIs stable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification. @jrudolph

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure this PR can be resolve the kamon issue, because Kamon Instruementation seem like didn't advise/intercept this method.

the stack trace of the original issue post shows that the DispatcherPrerequisites weren't assigned correctly.

Is it probably caused by this inline?

pekko/actor/src/main/scala-2.13/org/apache/pekko/compat/PartialFunction.scala

Lines 28 to 29 in 0f1db53

@inline def fromFunction[A, B](f: A => B): scala.PartialFunction[A, B] =
scala.PartialFunction.fromFunction(f)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Roiocam - there will need to be more changes. This PR is just to show one change and to highlight that the change doesn't break anything (tests or binary compatibility checks).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1489 adds more annotations

try dispatcher.systemDispatch(this, Terminate())
catch handleException

Expand Down