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

Emit the warning if the -Vstatistics option isn't set #47

Merged
merged 4 commits into from
Nov 23, 2023
Merged

Emit the warning if the -Vstatistics option isn't set #47

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3650743
Emit the warning if the 'Ystatistics' option isn't set
danicheg Nov 2, 2023
299c46d
Use named arguments in config instantiation
danicheg Nov 2, 2023
d8ea328
Let the 'statistics' compile option vary for 2.12 and 2.13
danicheg Nov 21, 2023
5413ff9
Merge branch 'main' into fix-#45
danicheg Nov 23, 2023
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
29 changes: 20 additions & 9 deletions plugin/src/main/scala/ch/epfl/scala/ProfilingPlugin.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,16 @@ import ch.epfl.scala.profiledb.utils.AbsolutePath
import ch.epfl.scala.profilers.ProfilingImpl
import ch.epfl.scala.profilers.tools.{Logger, SettingsOps}

import scala.reflect.internal.util.{SourceFile, Statistics}
import scala.reflect.internal.util.{NoPosition, SourceFile, Statistics}
import scala.reflect.io.Path
import scala.tools.nsc.Reporting.WarningCategory
import scala.tools.nsc.io.AbstractFile
import scala.tools.nsc.{Global, Phase}
import scala.tools.nsc.plugins.{Plugin, PluginComponent}
import scala.util.Try
import scala.util.matching.Regex

class ProfilingPlugin(val global: Global) extends Plugin {
class ProfilingPlugin(val global: Global) extends Plugin { self =>
// Every definition used at init needs to be lazy otherwise it slays the compiler
val name = "scalac-profiling"
val description = "Adds instrumentation to keep an eye on Scalac performance."
Expand Down Expand Up @@ -55,13 +56,13 @@ class ProfilingPlugin(val global: Global) extends Plugin {
}

private final lazy val config = PluginConfig(
super.options.contains(ShowProfiles),
super.options.contains(NoProfileDb),
findOption(SourceRoot, SourceRootRegex).map(AbsolutePath.apply),
findSearchIds(findOption(PrintSearchResult, PrintSearchRegex)),
super.options.contains(GenerateMacroFlamegraph),
super.options.contains(PrintFailedMacroImplicits),
super.options.contains(ShowConcreteImplicitTparams)
showProfiles = super.options.contains(ShowProfiles),
noDb = super.options.contains(NoProfileDb),
sourceRoot = findOption(SourceRoot, SourceRootRegex).map(AbsolutePath.apply),
printSearchIds = findSearchIds(findOption(PrintSearchResult, PrintSearchRegex)),
generateMacroFlamegraph = super.options.contains(GenerateMacroFlamegraph),
printFailedMacroImplicits = super.options.contains(PrintFailedMacroImplicits),
concreteTypeParamsInImplicits = super.options.contains(ShowConcreteImplicitTparams)
)

private lazy val logger = new Logger(global)
Expand Down Expand Up @@ -294,6 +295,16 @@ class ProfilingPlugin(val global: Global) extends Plugin {
override def run(): Unit = {
super.run()

if (!SettingsOps.areStatisticsEnabled(global)) {
val flagName = global.settings.Ystatistics.name.replace("-V", "-Y")
Copy link
Collaborator

@SethTisue SethTisue Nov 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

normally if -V is supported, it is canonical, and normally if a -Y form exists as well, it is only a deprecated alias. So unless you want to argue otherwise, I should we be documenting and recommending the -V form?

cc @som-snytt

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

normally if -V is supported, it is canonical, and normally if a -Y form exists as well, it is only a deprecated alias.

Thanks! Is this Scala-version-specific logic?

Copy link

Choose a reason for hiding this comment

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

scala> settings.Ystatistics.name
val res1: String = -Vstatistics

scala> settings.Ystatistics.abbreviations
val res2: List[String] = List(-Ystatistics)

Copy link
Collaborator

@SethTisue SethTisue Nov 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

But in 2.12,

scala 2.12.18> :power

scala 2.12.18> settings.Ystatistics.name
res0: String = -Ystatistics

scala 2.12.18> settings.Ystatistics.abbreviations
res1: List[String] = List()

Copy link
Contributor

Choose a reason for hiding this comment

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

So just removing .replace("-V", "-Y") should give the right flag on all versions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, perhaps this difference between -Y and -V is too subtle for the end users so we can show -V for 2.13 and -Y for 2.12. Any objections?

Copy link
Collaborator

Choose a reason for hiding this comment

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

SGTM

Copy link
Collaborator

@SethTisue SethTisue Nov 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I've submitted #52 to update the readme accordingly.

It would be good to have a more permanent home for the doc; I opened #53 on that.

global.runReporting.warning(
NoPosition,
s"`${self.name}` compiler plugin requires the option `$flagName` to be enabled",
WarningCategory.OtherDebug,
""
)
}

val graphsRelativePath = ProfileDbPath.GraphsProfileDbRelativePath
val graphsDir = globalOutputDir.resolve(graphsRelativePath)
reportStatistics(graphsDir)
Expand Down
Loading