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

Enrich the debug info with source files for implicit searches by type #49

Merged
merged 5 commits into from
Feb 11, 2024

Conversation

danicheg
Copy link
Collaborator

@danicheg danicheg commented Nov 9, 2023

Resolves #44

In #44, @lolgab suggested adding info about source files to the flamegraph. In fact, this could make the flamegraph unreadable (imagine putting the dozens of strings like /Users/foo/projects/bar/src/main/scala/baz/qux/quux.scala to the single cell). Here, I'm suggesting another approach to tackling the root issue — assisting in determining the source files for the particular types in implicit searches.

Current state

We already have the compiler plugin option -P:scalac-profiling:show-profiles that prints the debug info to the output:

[info] Implicit searches by type:
[info] LinkedHashMap(
[info]   "key.type => ?{def key: ?}" -> 1,
[info]   "com.softwaremill.quicklens.QuicklensMapAtFunctor[M,K,T]" -> 1,
[info]   "app.type => ?{def === : ?}" -> 1,
[info]   "io.circe.Encoder[java.time.LocalDate]" -> 1,
[info]   "io.circe.Encoder[Boolean]" -> 1,
...

But it's lacking the info about the source files to peek at.

Proposed changes

This PR modifies the output by bringing the source files alongside the number of firings for the particular types in implicit searches.

[info] Implicit searches by type:
[info] LinkedHashMap(
[info]   "Array[String] => ?{def mkString: ?}" -> """
[info] {
[info]   "firings": 1,
[info]   "sourceFiles": [
[info]     /Users/foo/Documents/bar/src/main/scala/baz/package.scala
[info]   ]
[info] }""",
[info]   "x$4.type => ?{def id: ?}" -> """
[info] {
[info]   "firings": 1,
[info]   "sourceFiles": [
[info]     /Users/foo/Documents/bar/src/main/scala/baz/Baz.scala
[info]   ]
[info] }""",
...

Copy link
Collaborator

@SethTisue SethTisue left a comment

Choose a reason for hiding this comment

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

LGTM (within the limits of my limited knowledge of this repo, anyway)

@lolgab
Copy link
Contributor

lolgab commented Nov 20, 2023

I tried this and I don't understand what -> "" means.
Also this is not a valid JSON key anymore since you would need to escape the " with \".
Probably you are missing a ._1 somewhere

@lolgab
Copy link
Contributor

lolgab commented Nov 20, 2023

Is it possible to add the line number alongside the file name? Like package.scala:33

@danicheg
Copy link
Collaborator Author

I tried this and I don't understand what -> "" means.

How do you work with that debug output? For my usage, it's no more than just text and not valid code at all 🤔

Is it possible to add the line number alongside the file name? Like package.scala:33

The issue is that a single file could contain multiple implicit searches for the same type, so we must account for that information. Also, according to the reflect API, we could have not only the single-point position but ranges too. It seems that the output can become verbose and difficult to read.

@lolgab
Copy link
Contributor

lolgab commented Nov 20, 2023

How do you work with that debug output? For my usage, it's no more than just text and not valid code at all 🤔

Sure, but what is the meaning of the second string? I tried it on a 200KLOC codebase and it was always an empty string.

@lolgab
Copy link
Contributor

lolgab commented Nov 20, 2023

The issue is that a single file could contain multiple implicit searches for the same type, so we must account for that information. Also, according to the reflect API, we could have not only the single-point position but ranges too. It seems that the output can become verbose and difficult to read.

I understand. But with files with thousands of LOC this is not very useful since the definition can be anywhere.
You could return the start line (and column if you have) of the range and return a file multiple times if the search touches different places of the same file.
The output is already super verbose, this will make it more precise and useful.

One note for the future. It would be nice if this information was written to a file instead of printed to the stdout. I need to do sbtn compile > somefile to be able to read it and I have all the annoying [info] output from Sbt.

@danicheg
Copy link
Collaborator Author

It would be nice if this information was written to a file instead of printed to the stdout.

I also thought about this one 😄 I wasn't just sure about the approach to managing this. Like, we can ask for the location for that debug file. But it seems we're going to ask so many locations from users: source root, target, debug file. But I don't mind iterating over this since I've tried to enhance the current output data at this run.

@lolgab from your perspective, what output format does fit the debug purposes best (considering adding the positions info)?

@lolgab
Copy link
Contributor

lolgab commented Nov 20, 2023

It would be nice if this information was written to a file instead of printed to the stdout.

I also thought about this one 😄 I wasn't just sure about the approach to managing this. Like, we can ask for the location for that debug file. But it seems we're going to ask so many locations from users: source root, target, debug file. But I don't mind iterating over this since I've tried to enhance the current output data at this run.

Yes, it can be iterated over later.

@lolgab from your perspective, what output format does fit the debug purposes best (considering adding the positions info)?

I would use the same format printed by the compiler:
SourceFile.scala:22

It's nice because if you command+click from an IDE it will move you to the exact line.

@danicheg
Copy link
Collaborator Author

I would use the same format printed by the compiler:
SourceFile.scala:22

So, instead of having a set of file names, you're suggesting adding a list of paths containing positions, right? But I was about the whole output, which is currently a LinkedHashMap stringified types to the info of firings + source files.

@SethTisue
Copy link
Collaborator

@lolgab any further thoughts on this?

@danicheg danicheg added the scalac-profiling Relates to the compiler plugin label Dec 10, 2023
@SethTisue
Copy link
Collaborator

ping @lolgab

@danicheg danicheg marked this pull request as draft January 8, 2024 08:31
@danicheg
Copy link
Collaborator Author

danicheg commented Jan 8, 2024

I plan to revise what I did here a few months ago in light of @lolgab's comments and some changes in my mindset.

@lolgab
Copy link
Contributor

lolgab commented Jan 10, 2024

@SethTisue Sorry for my late response.
My general thought is that the output of the plugin should be as useful and as more actionable as possible.
Currently, it's a wall of text printed with pprint, which is useful for printing data structures as "compilable" Scala code.
But here we generally don't want to copy-paste the output and write it into a Scala file. Also, it's almost impossible since the output is huge (it exceeded my terminal max line scroll history).
I would prefer a machine-readable format like JSON with information useful to understand where the span is coming from (so I would print compiler-like paths) in a json object.
Also, I would prefer to see the output printed on a file instead of on the console, since it's too much to be read while compiling.

@danicheg danicheg added this to the v1.1.0-RC3 milestone Jan 19, 2024
@danicheg
Copy link
Collaborator Author

danicheg commented Feb 4, 2024

@lolgab I've thought about this PR and your suggestions. I agree that debugging the output built by the show-profiles option as the SBT's output isn't convenient. But let me outline what this option produces at the moment:

  • Macro data per call-site
  • Macro data per file
  • Macro data in total
  • Macro repeated expansions
  • Macro expansions by type
  • Implicit searches by position

And with this PR, we are going to add:

  • Implicit searches by type

Given all of this, it'd break the coherency of that option if we make the output just of the 'Implicit searches by type' to be logged into the external file. Instead, let's bring another option, say 'export-profiles', that will prepare the output in a human-readable format. I opened #105, and it'd be exciting if you could write your thoughts there.

For this PR, I'm willing to keep the show-profilies coherent and print the introduced new debug info to the output as well.

@danicheg danicheg marked this pull request as ready for review February 11, 2024 09:49
@danicheg
Copy link
Collaborator Author

So, given my previous comment, the ultimate output will have the following format:

[info] Implicit searches by type:
[info] LinkedHashMap(
[info]   "dependency.type => ?{def as: ?}" -> ImplicitSearchDebugInfo(
[info]     firings = 1,
[info]     sourceFiles = List(
[info]       "/Users/foo/scala-steward/modules/core/src/main/scala/org/scalasteward/core/update/UpdateAlg.scala"
[info]     )
[info]   ),
[info]   "maybeRepoConfig.type => ?{def |+| : ?}" -> ImplicitSearchDebugInfo(
[info]     firings = 1,
[info]     sourceFiles = List(
[info]       "/Users/foo/scala-steward/modules/core/src/main/scala/org/scalasteward/core/repoconfig/RepoConfigAlg.scala"
[info]     )
[info]   )

Yeah, it's still not super convenient to explore SBT's output, but at the very least, it will be aligned with the entire output produced by the existing compiler plugin option. All other work to make the output more accessible will be done within #105.
Given the approval by @SethTisue (the code hasn't almost changed since then), I'm merging this. In any case, please feel free to reach out to me. The next RC will be published in a month or so.

@danicheg danicheg merged commit 2ebc390 into scalacenter:main Feb 11, 2024
5 checks passed
@danicheg danicheg deleted the fix-#44 branch February 11, 2024 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scalac-profiling Relates to the compiler plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display file name of offending implicits in flamegraph
3 participants