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.

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

Enable scala2 inliner #605

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mdedetrich
Copy link
Contributor

@mdedetrich mdedetrich commented Jan 17, 2024

I noticed that this project uses the @inline annotation which actually doesn't do anything unless you have the scala-2 inliner enable, quoting from https://docs.scala-lang.org/overviews/compiler-options/optimizer.html

The @inline annotation only has an effect if the inliner is enabled. It tells the inliner to always try to inline the annotated method or callsite.

Ontop of this the PR splits out the implementation between Scala 2 and Scala 3. This is because Scala 3 hasn't ported the Scala 2 optimizer yet. Instead we use the inline keyword which for our intents and purposes achieves the same effect.

MiMa filters were added but this is for code that was marked private/internal use so it shouldn't be an issue.

@mdedetrich mdedetrich marked this pull request as draft January 17, 2024 13:30
@mdedetrich mdedetrich force-pushed the enable-scala2-inliner branch 5 times, most recently from 7eb7019 to e1f3684 Compare January 17, 2024 13:56
@mdedetrich mdedetrich force-pushed the enable-scala2-inliner branch from e1f3684 to bbea2b4 Compare January 21, 2024 23:07
@mdedetrich mdedetrich marked this pull request as ready for review January 21, 2024 23:13
@@ -9,3 +9,4 @@ danglingParentheses.preset = true
rewrite.rules = [AvoidInfix, SortImports, RedundantBraces, RedundantParens, SortModifiers]
docstrings.style = Asterisk
align.preset = none
project.layout = StandardConvention
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This setting instructs scalafmt to apply the Scala 3 dialect to Scala 3 sources since this is the first time that this project has explicit Scala 3 sources.

@rossabaker
Copy link
Member

Is there any empirical evidence that the inliner helps here? Sometimes it even makes things worse.

@mdedetrich
Copy link
Contributor Author

I did some testing but because I am overseas I only had my M1 pro which is notoriously difficult to benchmark especially when detecting minor performance improvements.

Ill be back at home in a few weeks and can re-run benchmarks on an actual desktop with locked clocks, will ping you then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants