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

Scala 2 forwardport: -Yprofile-trace #19897

Merged
merged 12 commits into from
Oct 22, 2024
Merged

Conversation

WojciechMazur
Copy link
Contributor

@WojciechMazur WojciechMazur commented Mar 7, 2024

  • Scala 2 tracing profiler backport from Emit detailed compiler trace under -Yprofile-trace scala#7364 extended with more Scala 3 idiomatic syntax based on inlined methods
  • Fixes the context.profiler which could have been null, now it's initially a NoOp Profiler
  • Check dependencies of -Yprofile-enabled dependent tasks, now we get an error if -Yprofile-trace is set without -Yprofile-enabled

@bishabosha
Copy link
Member

@keynmol petition here

@keynmol
Copy link
Contributor

keynmol commented Apr 5, 2024

YES YES YES PLEASE
This would be so much better than the several formats currently enabled by 3 different flags

@WojciechMazur WojciechMazur added this to the 3.5.0 milestone Apr 5, 2024
@WojciechMazur WojciechMazur force-pushed the backport/Yprofiler-trace branch 2 times, most recently from 880e2a7 to 70523b7 Compare April 10, 2024 12:12
@WojciechMazur WojciechMazur marked this pull request as ready for review April 11, 2024 09:47
@WojciechMazur
Copy link
Contributor Author

@nicolasstucki I think you we're doing the original backport of Profiler from Scala 2. Can you take a look at this and make a review?

@nicolasstucki nicolasstucki self-requested a review April 12, 2024 10:03
@nicolasstucki
Copy link
Contributor

I generated the profile for

enum Foo:
  case A
  case B
  case C

and got the following result

Screenshot 2024-04-18 at 10 52 36

All metrics (except for GC) are missing in the type phase.

@dwijnand dwijnand changed the title Scala 2 backport: -Yprofile-trace Scala 2 forwardport: -Yprofile-trace Apr 18, 2024
@WojciechMazur
Copy link
Contributor Author

The metrics are collected after the CompilationUnit with at least 10ms intervals. Parser used overriden runOn method that didn't run onUnit callback. Becouse there was only 1 compilation unit it seemed like collection of metrics started after the typer. In fact it works the same in Scala 2, but I've fixed to collect initial results in the constructor of the profiler - now we got statistics from the very beginning of the compilation run
image

@WojciechMazur WojciechMazur force-pushed the backport/Yprofiler-trace branch from 16795d6 to 6eb6a23 Compare May 3, 2024 12:29
@WojciechMazur WojciechMazur force-pushed the backport/Yprofiler-trace branch 3 times, most recently from cc05d6a to 72d5e0e Compare May 8, 2024 11:04
@Gedochao Gedochao requested review from sjrd and rochala May 9, 2024 08:32
@Kordyjan Kordyjan removed this from the 3.5.0 milestone May 10, 2024
@WojciechMazur
Copy link
Contributor Author

WojciechMazur commented Sep 13, 2024

I needed to rebase the PR which is not good news for the reviewing. The improvements after the first review round start with 592a892

@WojciechMazur WojciechMazur force-pushed the backport/Yprofiler-trace branch from dc9766c to 718af3a Compare September 14, 2024 13:57
@WojciechMazur
Copy link
Contributor Author

test performance please

@dottybot
Copy link
Member

performance test scheduled: 1 job(s) in queue, 1 running.

@dottybot
Copy link
Member

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/19897/ to see the changes.

Benchmarks is based on merging with main (3097a84)

@WojciechMazur WojciechMazur merged commit ecc332f into main Oct 22, 2024
50 checks passed
@WojciechMazur WojciechMazur deleted the backport/Yprofiler-trace branch October 22, 2024 16:56
@tgodzik tgodzik added the release-notes Should be mentioned in the release notes label Oct 28, 2024
@WojciechMazur WojciechMazur added this to the 3.6.3 milestone Dec 9, 2024
tgodzik pushed a commit to scala/scala3-lts that referenced this pull request Feb 7, 2025
* Scala 2 tracing profiler backport from
scala/scala#7364 extended with more Scala 3
idiomatic syntax based on inlined methods
* Fixes the `context.profiler` which could have been `null`, now it's
initially a NoOp Profiler
* Check dependencies of `-Yprofile-enabled` dependent tasks, now we get
an error if `-Yprofile-trace` is set without `-Yprofile-enabled`
tgodzik added a commit to scala/scala3-lts that referenced this pull request Feb 7, 2025
* Scala 2 tracing profiler backport from
scala/scala#7364 extended with more Scala 3
idiomatic syntax based on inlined methods
* Fixes the `context.profiler` which could have been `null`, now it's
initially a NoOp Profiler
* Check dependencies of `-Yprofile-enabled` dependent tasks, now we get
an error if `-Yprofile-trace` is set without `-Yprofile-enabled`
[Cherry-picked ecc332f][modified]
tgodzik pushed a commit to scala/scala3-lts that referenced this pull request Feb 13, 2025
* Scala 2 tracing profiler backport from
scala/scala#7364 extended with more Scala 3
idiomatic syntax based on inlined methods
* Fixes the `context.profiler` which could have been `null`, now it's
initially a NoOp Profiler
* Check dependencies of `-Yprofile-enabled` dependent tasks, now we get
an error if `-Yprofile-trace` is set without `-Yprofile-enabled`
tgodzik added a commit to scala/scala3-lts that referenced this pull request Feb 13, 2025
* Scala 2 tracing profiler backport from
scala/scala#7364 extended with more Scala 3
idiomatic syntax based on inlined methods
* Fixes the `context.profiler` which could have been `null`, now it's
initially a NoOp Profiler
* Check dependencies of `-Yprofile-enabled` dependent tasks, now we get
an error if `-Yprofile-trace` is set without `-Yprofile-enabled`
[Cherry-picked ecc332f][modified]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes Should be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants