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

Use subcolon args to simplify optimizer options #9810

Merged
merged 3 commits into from Feb 20, 2022

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Nov 16, 2021

Optimizer is enabled by -opt:inline:**, as described in the new overview. Warnings are enabled by -Wopt and verbose output by -Vinline.

Fixes scala/bug#11873

@scala-jenkins scala-jenkins added this to the 2.13.8 milestone Nov 16, 2021
@lrytz lrytz modified the milestones: 2.13.8, 2.13.9 Nov 18, 2021
@som-snytt som-snytt force-pushed the issue/11873 branch 2 times, most recently from 5ca6c01 to dc9b785 Compare November 22, 2021 06:09
@som-snytt
Copy link
Contributor Author

obviously, allowing -opt:inline:** breaks -Wconf:cat=foo:s

@som-snytt som-snytt marked this pull request as ready for review December 12, 2021 06:54
@som-snytt som-snytt requested a review from lrytz December 12, 2021 06:55
@som-snytt
Copy link
Contributor Author

som-snytt commented Dec 13, 2021

I thought I wasn't supposed to update the build, but I must have tossed a bad coin.

[error] -opt-inline-from is deprecated: use -opt:inline:**
[error] one error found

Edit: setupPublishCore needs old options

++ sbt -sbt-version 1.5.6 -Dsbt.override.build.repos=true -Dsbt.repository.config=/home/jenkins/workspace/scala-2.13.x-validate-main/scripts/sbt-repositories-config -warn 'setupPublishCore https://scala-ci.typesafe.com/artifactory/scala-pr-validation-snapshots/' publish
[error] 'all' is not a valid choice for '-opt'
[error] 'inline:scala/**' is not a valid choice for '-opt'
[error] 'all' is not a valid choice for '-opt'
[error] 'inline:scala/**' is not a valid choice for '-opt'
[error] 'all' is not a valid choice for '-opt'
[error] 'inline:scala/**' is not a valid choice for '-opt'

Edit: undeprecate the old options because no point in 2.13.

Also, some usages of -opt:l:inline in tests require only -opt:inline, others really do need -opt:all or a subset.

@som-snytt som-snytt force-pushed the issue/11873 branch 5 times, most recently from 6f8fe94 to ab29500 Compare December 14, 2021 00:52
@lrytz
Copy link
Member

lrytz commented Dec 15, 2021

Thank you for taking this on!

I see two issues with the current state:

  • -opt:inline:pattern only enables the inliner, no local optimizations. There is no situtaion I can think of for anyone to ever want this, local optimizations should always be on (except maybe us for testing the inliner). It's too easy to make that mistake - even this PR changes many -opt:l:inline -opt-inline-from:** to -opt:inline:**.
  • -opt:inline and -opt:all default to -opt:inline:**, which is unsafe. Since we're changing the flags, it's a chance to issue an error when specifying plain -opt:inline without a pattern.

Could we make opt:inline:pattern enalbe all optimizations, remove the all option, and require a pattern?

@som-snytt
Copy link
Contributor Author

I will defer to your expert opinion on those two gotchas.

The only use case for fine-grained options is, "The optimizer broke my code, so I have to turn off a feature for this file."

I agree, it was a nuisance to fiddle with -opt:all in the tests. (I intended to enable only minimal options for testing, but didn't want to comprehend every test.)

On not defaulting to double star, it would be nice if there were a convenient default, such as, inline from classes with sources in this build. Then an option to inline from dependencies; from std lib; from platform lib. But maybe real programmers don't mind saying com.example.**. Oh wait, there is <sources>, why isn't that the default? Probably in sbt that only works for a clean build? and not for subprojects.

I will follow up with the tighter screws before I forget how it works.

@lrytz
Copy link
Member

lrytz commented Dec 15, 2021

About defaulting to <sources>: it's already not a good idea today to enable the optimizer during development, but I wouldn't be surprised if it still happens. Defaulting to <sources> makes would make things even more random.

I think requiring people to think about inlining when enabling it is a good thing :)

@som-snytt
Copy link
Contributor Author

Thanks lrytz, you have me singing the classic tune, "Puttin' on the Rytz". Probably I've made that pun before.

The new behavior is much better. One difference is that before, you needed -opt:inline or probably -opt:l:inline and -opt-inline-from:** to enable inlining, the way you need both keys turned simultaneously to launch the missiles.

Specifying only one option would silently not inline anything.

Now, -opt:inline requires the pattern, and -opt-inline-from:** is just an alias that works like -opt:inline:** so that by itself it enables.

@som-snytt
Copy link
Contributor Author

I needed this the other day when I couldn't remember how to use -opt, so I used -optimise, with an ess in case it didn't accept zed. It was to check out -Wperformance.

Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

LGTM, thank you very much! A few minor suggestions, if you agree I'm happy to push them, what ever works best.

build.sbt Outdated Show resolved Hide resolved
project/ScriptCommands.scala Outdated Show resolved Hide resolved
src/compiler/scala/tools/nsc/settings/ScalaSettings.scala Outdated Show resolved Hide resolved

val inlineFrom = Choice(
"inline",
s"Inline method invocations from specified sites; enables all optimizations; see -Yopt-inline-heuristics.\n$inlineHelp",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
s"Inline method invocations from specified sites; enables all optimizations; see -Yopt-inline-heuristics.\n$inlineHelp",
s"Inline method invocations, enables all optimizations. Requires specifying sites from where to inline, see below. See also -Yopt-inline-heuristics.\n\n$inlineHelp",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason this is unresolved is that I missed it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I pushed approximately that. I had already added a newline to the inlineHelp. It also follows -opt-inline-from:help.

src/compiler/scala/tools/nsc/settings/ScalaSettings.scala Outdated Show resolved Hide resolved
src/compiler/scala/tools/nsc/settings/ScalaSettings.scala Outdated Show resolved Hide resolved
Grouping options `l:none`, `l:default`, `l:method`, `l:inline`,
are renamed `none`, `default`, `local`, `all`.

`-opt-inline-from:**` is dropped in favor of `-opt:inline:**`.

`-opt-warnings:_` is renamed `-Wopt:_`.

The ambiguity in parsing `-option:command:a,b,c`, where `-Wconf`
sees values `command:a`, `b`, `c`, is resolving by special-
casing `MultiChoiceSetting` only.
@SethTisue
Copy link
Member

@som-snytt do you want a second round of review from Lukas, or are you reasonably confident you've addressed his concerns and we should go ahead and merge?

@som-snytt
Copy link
Contributor Author

yes, now it is quite gorgeous after following up his tweaks. It is tweakèd beauty.

@SethTisue SethTisue merged commit c917d72 into scala:2.13.x Feb 20, 2022
@som-snytt som-snytt deleted the issue/11873 branch February 20, 2022 06:14
@lrytz lrytz added the release-notes worth highlighting in next release notes label Mar 4, 2022
@lrytz
Copy link
Member

lrytz commented Mar 4, 2022

The old settings (-opt:l:XX and -opt-inline-from:XX) continue to work and without deprecation warning, but we should still update the PR description to explain what's changed and include it the release notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes worth highlighting in next release notes
Projects
None yet
4 participants