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

Performance improvements #1564

Merged
merged 3 commits into from
Nov 27, 2019
Merged

Performance improvements #1564

merged 3 commits into from
Nov 27, 2019

Conversation

olafurpg
Copy link
Member

Incorporates changes from #1386 and makes them compile on 2.11/2.12/2.13. The performance improvements are small but noticeable!

[info] Benchmark                          Mode  Cnt     Score     Error  Units
[info] Micro.ExtraLarge.scalafmtAfter     avgt   10   826.260 ± 139.865  ms/op
[info] Micro.ExtraLarge.scalafmtBefore    avgt   10   960.471 ± 157.597  ms/op

jrudolph and others added 3 commits November 27, 2019 11:59
@jrudolph
Remove recursion, foreach is somewhat slow.
146ddc7
@jrudolph
Remove recursion in TreeOps.getOwners and use java.util.HashMap instead
57f7d5c
Make performance optimizations compile with 2.11/2.12
16de240 
Looks like these changes result in a noticeable speedup.
```
[info] Micro.ExtraLarge.scalafmt  avgt   10  995.077 ± 36.546  ms/op # current master
[info] Micro.ExtraLarge.scalafmt  avgt   10  826.260 ± 139.865  ms/op
```

Full results
```
[info] Micro.ExtraLarge.scalafmt          avgt   10   826.260 ± 139.865  ms/op
[info] Micro.ExtraLarge.scalafmt_rewrite  avgt   10  2001.845 ±  92.829  ms/op
[info] Micro.Large.scalafmt               avgt   10   183.766 ±  17.961  ms/op
[info] Micro.Large.scalafmt_rewrite       avgt   10   257.113 ±  29.702  ms/op
[info] Micro.Medium.scalafmt              avgt   10    69.160 ±   8.780  ms/op
[info] Micro.Medium.scalafmt_rewrite      avgt   10    76.610 ±   3.320  ms/op
[info] Micro.Small.scalafmt               avgt   10     4.655 ±   0.470  ms/op
[info] Micro.Small.scalafmt_rewrite       avgt   10     5.731 ±   0.980  ms/op
```
@olafurpg
Copy link
Member Author

The commit changing Input.VirtualFile to Input.File (ddb5bd8) was not included since it should no longer be necessary with the next release of Scalameta (see scalameta/scalameta#1914)

@olafurpg
Copy link
Member Author

Thank you @jrudolph for this contribution!

@olafurpg olafurpg mentioned this pull request Nov 27, 2019
Closed
poslegm
poslegm approved these changes Nov 27, 2019
View reviewed changes
Copy link
Collaborator

@poslegm poslegm left a comment

Choose a reason for hiding this comment

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

Thank you a lot for this really important improvement!

@olafurpg
Copy link
Member Author

Re-ran the benchmark with more iterations and the changes are less significant but still noticeable

# after
[info] Micro.ExtraLarge.scalafmt  avgt   30  831.742 ± 49.979  ms/op
# before
[info] Micro.ExtraLarge.scalafmt  avgt   30  901.238 ± 10.708  ms/op

@olafurpg olafurpg merged commit 717f229 into scalameta:master Nov 27, 2019
@olafurpg olafurpg deleted the performance branch November 27, 2019 14:12
@jrudolph
Copy link
Contributor

Cool, almost forgot about that :) Thanks for bringing it over the finish line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@poslegm poslegm poslegm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@olafurpg @jrudolph @poslegm