Skip to content

Scala 2 forwardport: -Yprofile-trace #19897

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
Merged
merged 12 commits into from
Oct 22, 2024
Merged

Scala 2 forwardport: -Yprofile-trace #19897

merged 12 commits into from
Oct 22, 2024

Conversation

@WojciechMazur
Copy link
Contributor

@WojciechMazur WojciechMazur commented Mar 7, 2024 •
edited
Loading

  • 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

@WojciechMazur WojciechMazur force-pushed the backport/Yprofiler-trace branch from 3eeaa3d to 8c55116 Compare March 7, 2024 19:54
@WojciechMazur WojciechMazur mentioned this pull request Mar 11, 2024
Closed
@bishabosha
Copy link
Member

bishabosha commented Apr 5, 2024

@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 mentioned this pull request Apr 8, 2024
Closed
@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

WojciechMazur commented Apr 11, 2024

@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 reviewed Apr 12, 2024
View reviewed changes
@nicolasstucki nicolasstucki self-requested a review April 12, 2024 10:03
@nicolasstucki
Copy link
Contributor

nicolasstucki commented Apr 18, 2024

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.

nicolasstucki
nicolasstucki suggested changes Apr 18, 2024
View reviewed changes
@lrytz lrytz mentioned this pull request Apr 18, 2024
Closed
@dwijnand dwijnand changed the title Scala 2 backport: -Yprofile-trace Scala 2 forwardport: -Yprofile-trace Apr 18, 2024
@WojciechMazur WojciechMazur mentioned this pull request May 2, 2024
Closed
@WojciechMazur
Copy link
Contributor Author

WojciechMazur commented May 3, 2024

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 rochala and sjrd 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 •
edited
Loading

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
Backport -Yprofile-trace from Scala 2
b9af5a2 
Adapt PresentationCompiler to always set (by default noop) profiler
@WojciechMazur
Profile more events, make the profiler more Scala3 idiomatic
3450dfb
@WojciechMazur
Fix assignment of RunIds - make sure they'll start with 1 == InitialR…
016dfdd 
…unId instead of 2
@WojciechMazur
Initialize context with NoOpProfiler to prevent null pointer exceptio…
4077575 
…ns and to match the store non-nullable signature
@WojciechMazur
Give compilation error when -Yprofile-trace is used without -Yprofile…
94f7613 
…-enabled. Allow to define dependencies in String/Phase settings (previously unused)
@WojciechMazur
More idiomatic trace-profiler integraiton. Prevent malformed outputs …
c0ecfe0 
…by appending GC events after profiling is done.
@WojciechMazur
Workaround changes to runId starting with 1 instead of 2 due to probl…
485c044 
…ems with scala2-library-bootstrapped compilation
@WojciechMazur
Remove commented out withPostSetHook actions
592a892
@WojciechMazur
Document SettingDependencies type
dc10c53
@WojciechMazur
Add unit tests for ChromeTrace and port FileUtilsTest from Scala2
e76fce4
@WojciechMazur
Reduce ammount of allocations, increase ammontof counters tracing and…
718af3a 
… other cleanups
@WojciechMazur WojciechMazur force-pushed the backport/Yprofiler-trace branch from dc9766c to 718af3a Compare September 14, 2024 13:57
@WojciechMazur
Copy link
Contributor Author

WojciechMazur commented Sep 23, 2024

test performance please

@dottybot
Copy link
Member

dottybot commented Sep 23, 2024

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

@dottybot
Copy link
Member

dottybot commented Sep 23, 2024

Performance test finished successfully:

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

Benchmarks is based on merging with main (3097a84)

@filipwiech filipwiech mentioned this pull request Sep 23, 2024
Open
@WojciechMazur WojciechMazur requested a review from mbovel October 3, 2024 18:21
@WojciechMazur WojciechMazur merged commit ecc332f into main Oct 22, 2024
@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
@WojciechMazur @tgodzik
Scala 2 forwardport: -Yprofile-trace (scala#19897)
2fb4db6 
* 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
@tgodzik
Scala 2 forwardport: -Yprofile-trace (scala#19897)
c39d519 
* 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 tgodzik mentioned this pull request Feb 7, 2025
Closed
tgodzik pushed a commit to scala/scala3-lts that referenced this pull request Feb 13, 2025
@WojciechMazur @tgodzik
Scala 2 forwardport: -Yprofile-trace (scala#19897)
fb8b3dd 
* 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
@tgodzik
Scala 2 forwardport: -Yprofile-trace (scala#19897)
eb66111 
* 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 tgodzik mentioned this pull request Feb 13, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mbovel mbovel mbovel approved these changes

@nicolasstucki nicolasstucki Awaiting requested review from nicolasstucki

@sjrd sjrd Awaiting requested review from sjrd

@rochala rochala Awaiting requested review from rochala

Assignees

@sjrd sjrd

@mbovel mbovel

@WojciechMazur WojciechMazur

Labels

release-notes Should be mentioned in the release notes

Projects

None yet

Milestone

3.6.3

Development

Successfully merging this pull request may close these issues.

10 participants

@WojciechMazur @bishabosha @keynmol @nicolasstucki @dottybot @mbovel @sjrd @tgodzik @Kordyjan