Skip to content

WIP: Improve the performance of the formatter #1280

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

Closed
wants to merge 3 commits into from

Conversation

rjmholt
Copy link
Contributor

@rjmholt rjmholt commented Jul 2, 2019

PR Summary

@mattpwhite noted in a comment in the PS extension repo that the formatter is one of the big causes for performance problems in the PowerShell extension.

With that in mind, I want to see what I can do to drive down some of those performance issues.

In this particular PR, I've rewritten the way formatting occurs, so that:

  • Rather than running each formatting rule in its own invocation, they are all run together
  • Rather than using a lot of LINQ and an array of strings to represent a text file, we perform edits in a cleverer way and amortise allocations to some extent

This doesn't directly address @mattpwhite's issue where the formatter likes to import modules, but it goes to some extent.

I'll say that when I first saw the formatting code I was a bit horrified, and I really don't like the idea of the module import stuff either, so I'm going to do my best on that.

Here's an indicative performance result on a pathological case:

$s = 'gci;'*1000
Measure-Command { Invoke-Formatter $s }

Output in PSSA 1.18.1

Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 15
Milliseconds      : 339
Ticks             : 153390477
TotalDays         : 0.00017753527430555555
TotalHours        : 0.0042608465833333335
TotalMinutes      : 0.255650795
TotalSeconds      : 15.3390477
TotalMilliseconds : 15339.047700000001

Output with the changes in this branch:

Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 0
Milliseconds      : 61
Ticks             : 619337
TotalDays         : 7.168252314814815E-07
TotalHours        : 1.7203805555555554E-05
TotalMinutes      : 0.0010322283333333334
TotalSeconds      : 0.061933699999999994
TotalMilliseconds : 61.9337

(A ~250x speedup)

PR Checklist

@bergmeister
Copy link
Collaborator

I vaguely remember reading a comment from the original maintainer of PSSA that wrote the code that the formatter has the technical debt of the rules to be executed in a certain sequence therefore I think refactoring to execute all rules in parallel like Invoke-ScriptAnalyzer will give you a hard time. Currently there are even cases where one has to format twice to correct all the violations as some suggestions can eleminate each other.
I'm definitely up for improving what can be improved but just with the caveat that you'll be fighting a beast...

@rjmholt
Copy link
Contributor Author

rjmholt commented Jul 3, 2019

Currently there are even cases where one has to format twice to correct all the violations as some suggestions can eleminate each other

Yes I noticed that. I still have to do that to some extent, but I've tried to minimise that; the formatter currently only allows one correction per line, but I've changed it to apply all non-overlapping corrections in a single pass.

@mattpwhite
Copy link

Just wanted to chime in and say thanks. Seems like a really impressive improvement.

@rjmholt
Copy link
Contributor Author

rjmholt commented Jul 3, 2019

Just wanted to chime in and say thanks. Seems like a really impressive improvement.

Well that's for a pathological scenario. On a normal file it's only about a 2x speedup at the moment. I think some of that can be addressed in the formatting logic, but there's plenty more work to do in the rules. Given your trace it's going to require more work to shave out some of the file path stuff and module loading.

@JamesWTruher
Copy link
Contributor

rob asked me to close this - something else may come later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants