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

Jump to bottom

Fix remaining missing inline statements/annotations #889

Merged
merged 1 commit into from
Dec 30, 2023
Merged

Fix remaining missing inline statements/annotations #889

merged 1 commit into from
Dec 30, 2023

Conversation

mdedetrich
Copy link
Contributor

The previous PR at #857 managed to miss some inline annotations so this PR fills the rest.

Interestingly I managed to find a Scala 3 bug whereby if you mark a method with inline it cannot be called within Java sources because its missing a symbol, I have reported the issue upstream at scala/scala3#19346

@mdedetrich mdedetrich force-pushed the fill-missing-inline-statements branch from 5df3272 to 7fa861f Compare December 30, 2023 05:12
@He-Pin
Copy link
Member

He-Pin commented Dec 30, 2023

For method which would be called from Java, maybe was expected be marked with noinline?

He-Pin
He-Pin approved these changes Dec 30, 2023
View reviewed changes
Copy link
Member

@He-Pin He-Pin left a comment

Choose a reason for hiding this comment

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

Mostly lgtm

actor/src/main/scala-2.13/org/apache/pekko/dispatch/internal/SameThreadExecutionContext.scala
*/
@InternalApi
private[dispatch] object SameThreadExecutionContext {
@inline def apply(): ExecutionContext = ExecutionContext.parasitic
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need this SameThreadExecutionContext？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, see https://github.com/apache/incubator-pekko/blob/b0fdac259bd57fdd481483f3fe9a7aec6e1ff38a/actor/src/main/scala-2.12/org/apache/pekko/dispatch/internal/SameThreadExecutionContext.scala for why

@mdedetrich
Copy link
Contributor Author

mdedetrich commented Dec 30, 2023 •
edited
Loading

For method which would be called from Java, maybe was expected be marked with noinline?

Maybe but its also not intuitive behaviour. For starters even though its there for Java, nothing is preventing you from calling the method in Scala (in which case we want it to be inlined) and its also different from Scala 2's @inline annotation which can still be called as a standard method in Java.

@noinline is different, its when you want to forcefully prevent any inlining at all and at least in Pekko its used in cases where the stack trace is being inspected (which inlining can mess with).

@mdedetrich mdedetrich force-pushed the fill-missing-inline-statements branch from 7fa861f to 53f58d9 Compare December 30, 2023 05:26
@mdedetrich
Copy link
Contributor Author

Im going to go ahead and merge this now so I can fix any potential future issues in the nightly runs on the other pekko modules.

@mdedetrich mdedetrich merged commit 0f1db53 into apache:main Dec 30, 2023
18 checks passed
@mdedetrich mdedetrich deleted the fill-missing-inline-statements branch December 30, 2023 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@He-Pin He-Pin He-Pin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mdedetrich @He-Pin