Add PekkoInlinePlugin

[mdedetrich]

This PR automatically enables the Scala 2 inliner for Pekkos modules. Unlike pekko core's PekkoInlinePlugin, this version ONLY does inlining that is considered safe, i.e. "-opt-inline-from:<sources>" which only inlines sources internal within the sbt module itself (i.e. never inlines code from outside the sbt module) and the various org.apache.pekko.util/org.apache.pekko.dispatch functions which are guaranteed not to change (hence safe to inline)

Note that even though this plugin is automatically enabled, in exceptional cases you can disable this plugin using DisablePlugin(PekkoInlinePlugin) on the sbt module.

Lastly, it is intended that this inliner should only be used for the 1.1.x branches of the various Pekko modules. I can't recall on the top of my head if we started using the sbt-pekko-build for Pekko projects that are still sitting on 1.0.x. If this is the case then another check needs to be added to make sure that the inliner is only enabled if the version of the module is 1.1.0 or higher, @pjfanning can you confirm/deny this?

[pjfanning]

One of the main reasons for this SBT plugin is to allow 1.0 builds to test with 1.1 builds from other Pekko modules. As a result, I would prefer if the inline support was opt-in - i.e. not enabled by default.

[mdedetrich]

One of the main reasons for this SBT plugin is to allow 1.0 builds to test with 1.1 builds from other Pekko modules. As a result, I would prefer if the inline support was opt-in - i.e. not enabled by default.

Making it opt-in by default is not solving any real problem since by design you would just enable PekkoInlinePlugin on every single sbt module in every single pekko module. If there is an issue with an inliner for a specific sbt module then its actually better to use DisablePlugin(PekkoInlinePlugin) on that specific sbt module because its very clear to external readers that this is something out of the ordinary and the best way to figure out if there is an inlining issue with a sbt module is to enable everywhere and to test.

The better way to solve this issue is to put in a check to make sure that the inliner is only enabled if the version of the sbt module is 1.1.0 or higher, its also codifying what has been agreed upon i.e. that only pekko module's which are version 1.1.0 or higher have the inliner enabled. Is this acceptable to you?

[mdedetrich]

@pjfanning Or are you arguing that by making it optional, it would only be enabled in the main branche's and not in the 1.0.x branches?

[pjfanning]

Yes - making it something that needs to be enabled explicitly means that we can enable it where we need to and that we don't need to go and disable explicitly in places where we don't want it.

[mdedetrich]

@pjfanning I have just updated the PR, basically adding an sbt setting called pekkoInlineEnabled which can be set to false for the various 1.0.x branches for our pekko module (it defaults to !sys.props.contains("pekko.no.inline") which is true unless you have something set for pekko.no.inline). The reason I did it this why is that after having a strong think about it, I came to various conclusions detailed below.

One thing that needs to made is that the inlining should only be set globally, i.e. either the entire sbt build should have every project with inline enabled or every project should have inlined disabled. Toggling inlining on a project by project basis is going to cause problems especially when coupled with incremental compilation (this is why in every other Scala project that uses the inliner its just enabled globally). For this reason, toggling the inliner using EnablePlugin/DisabledPlugin on various projects shouldn't be encouraged/used.

Now fixing this is quite simple, we can just add a pekkoInlineEnabled sbt setting which will either globally toggle the inliner (or not) which the updated PR already does.

The second question is whether this pekkoInlineEnabled should be defaulting to true or false and this is personally the point I deliberated most on. I still think having it set to true is the overall better option and the reason why is a point I made in the original inline PR from apache/pekko#857, i.e.

There is an argument that the default of inlining (i.e. pekko.no.inline) should be off rather than on because it can mess with local incremental compilation (although in practice this rarely happens). The reason why I opted for it to be on by default (aside from other Scala projects having inlining on by default) is that since we are doing manual releasing, it would be very easy to forget the inliner flag when making a build. When we get to the point of being able to build release artifacts in CI then I think is a good time to revisit this (i.e. the github action to make a release build would have the inliner flag enabled and locally it will be disabled by default).

That is, if the inliner is off by default it makes things much more difficult when validating releases. What the current implementation in this PR means is that for given Pekko module

• For the 1.0.x branch only, there will be a ThisBuild / pekkoInlineEnabled := false set which will globally disable any form of inlining whatsoever
• For every other branch (currently just main if 1.0.x exists but in the future will include other branches like 1.1.x) nothing needs to be done. Since pekkoInlineEnabled is defined as !sys.props.contains("pekko.no.inline"), people can just locally set pekko.no.inline if they want to disable the inliner locally for testing/validation

The issue is if we reverse this, i.e. make pekkoInlineEnabled default to false while it means that the 1.0.x branches are clean, it makes changes for the other branches i.e. main/1.1.x etc etc more complicated since we have to override pekkoInlineEnabled. We can just set it to true but then we lose the useful "pekko.no.inline" tuneable knob and if we want this knob then we have to redefine in every pekko module something like ThisBuild / pekkoInlineEnabled := "pekko.inline" which partially defeats the purpose of creating PekkoInlinePlugin inside of sbt-pekko-build in the first place (since the general underlying point of sbt-pekko-build is to remove common sbt boilerplate as well as making the settings consistent in our pekko modules).

In addition I have tested the inliner on pekko-http by publishing this project locally, setting the version in pekko-http to the locally published one and using the appropriate MiMa command)(i.e. sbt -Dpekko.http.parallelExecution=false -Dpekko.test.timefactor=2 -Dpekko.build.pekko.version=main "++ 2.12" mimaReportBinaryIssues) and I can confirm that it works as expected (i.e. the methods are getting inlined). Furthermore it doesn't break any binary compatibility whatsoever which means everything is working as intended.

wdyt?

[pjfanning]

I would still much prefer if this was not enabled by default - that you need to explicitly enable this behaviour.

We have 2 places where we want to enable it (pekko-core main and pekko-http main) and lots of modules and the 1.0.x branches on pekko-core and pekko-http where we don't want to enable it yet.

[mdedetrich]

I would still much prefer if this was not enabled by default - that you need to explicitly enable this behaviour.

I understand but this causes actual technical issues as I have outlined before, otherwise I wouldn't be arguing for it.

We have 2 places where we want to enable it (pekko-core main and pekko-http main)

I wasn't under the impression that this plugin is meant to be used for pekko main, if thats the case then the argument for having it enabled by default is even stronger. Also in pekko main the plugin is currently automatically enabled and it hasn't caused any issues.

and lots of modules and the 1.0.x branches on pekko-core and pekko-http where we don't want to enable it yet.

This is easily solvable with one line of code, when you update to the latest version of sbt-pekko-build you just add

ThisBuild / pekkoInlineEnabled := false

and thats it. I fail to understand why this is such a big deal especially since in the future the common case for the inliner is going to be enabled, not disabled. Once every Pekko module has its 1.1.x release, there will not be a single place where the inliner will be disabled aside from the 1.0.x branches.

Its actually best that we deal with the uncommon case (which is inliner being disabled) now as sbt-pekko-build is slowly being integrated rather than the other way around.

[mdedetrich]

Also now that I think of it, this sbt plugin really shouldn't be used in pekko main because its causing a circular dependency, we have code here referencing pekko http (i.e.

sbt-pekko-build/src/main/scala/com/github/pjfanning/pekkobuild/PekkoHttpDependency.scala

Line 16 in 0d42f2b

object PekkoHttpDependency extends VersionRegex {

) which means if it used in pekko-main then you have pekko-main relying on pekko-http behaviour which can rely on pekko-main behaviour.

We should either remove PekkoHttpDependency or rethink the problem we are solving.

[pjfanning]

Also now that I think of it, this sbt plugin really shouldn't be used in pekko main because its causing a circular dependency, we have code here referencing pekko http (i.e.

sbt-pekko-build/src/main/scala/com/github/pjfanning/pekkobuild/PekkoHttpDependency.scala

Line 16 in 0d42f2b

object PekkoHttpDependency extends VersionRegex {

) which means if it used in pekko-main then you have pekko-main relying on pekko-http behaviour which can rely on pekko-main behaviour.
We should either remove PekkoHttpDependency or rethink the problem we are solving.

this code does not use PekkoHttp code - it just looks at Maven Central repo to find version numbers using raw Scala code

[mdedetrich]

this code does not use PekkoHttp code - it just looks at Maven Central repo to find version numbers using raw Scala code

I know, but we have code referencing Pekko http sitting inside of Pekko which goes against basic design, i.e. no code inside of pekko should be aware of pekko-http existence and it just creates confusion.

I think this problem can easily be solved by just generalizing all of this maven central lookup logic into a trait (lets call it PekkoDependency) and then inside the various pekko modules we can extend that trait if they need this functionality, i.e.

PekkoHttpDependency extends PekkoDependency // if you want this functionality for pekko-http
PekkoConnectorsDependency extends PekkoDependency // if you want this functionality for pekko-connectors

Ill make a PR for this tomorrow

[pjfanning]

I'm still against making the inline behaviour the default. The PekkoDependency support is useful for us to put on 1.0.x branches (or main when 1.0.x is not yet forked). This will become dangerous with this PR change. Developers could easily enable inlining when they don't mean to.

[mdedetrich]

Inlining is meant to be a feature that is globally toggled, turned on and left that way, there is never a case for disabling it other than as a knob so you can see the effects which is why pekko.no.inline exists.

When I am saying I am doing so because this is how its used in the entirety of the Scala ecosystem, including massive critical projects such as the scala2 compiler and zinc inremental compiler

Developers could easily enable inlining when they don't mean to.

How, it would require a PR that has to be reviewed???? If we are talking about Pekko specifically, there is a setting to disable it for the 1.0.x branches (pekkoInlineEnabled) and I don't see the argument that "developers could accidentally turn it on" (its obviously going to contain a code comment saying the inliner is disabled due to 1.0.x) when

• You can argue the reverse (developer can accidentally turn it on in 1.0.x?)
• We are talking about the 1.0.x branches, which practically speaking no one is going to touch aside from the occasional security updates and/or cherry picks. There is a transition period regarding the rest of the modules moving to 1.1.x, but that is exactly that, transitional and hence temporary.

With this kind of defensive reasoning you can argue anything, developers can accidentally do a lot of things that can break a project. Having it disabled in 1.0.x is in of itself an already extremely defensive/excessive/reactionary stance since tbh personally the reason I am doing such is not because of any technical justification but just because of largely subjective buy in.

[mdedetrich]

I have updated the PR to handle the case when sbt-pekko-build is used for pekko-core (there is a pekkoInlineCoreProject setting which needs to be set to true only for https://github.com/apache/incubator-pekko)

[mdedetrich]

I have just added another commit which turns pekkoInlineCoreProject into a general setting (i.e. pekkoCoreProject) since it allows other plugins to use this setting to determine if we are dealing with pekko core or a pekko module.

The ability to determine if the project is pekko core or not is going to be a somewhat common occurance as pekko core does have a fair amount of build functionality which is bespoke to it.

[pjfanning]

lgtm