GeezerVison is showing github.com — Lazy initialisation of LinkedList in TokenOperations constructor -> 7% performance gain for formatter by bergmeister · Pull Request #1453 · PowerShell/PSScriptAnalyzer
Conversation
PR Summary
This is a simple optimizaton as many consumers that construct the TokenOperations class do not call APIs that use the LinkedList. This changes the CPU consumption of this class constructor during a format run from 8% to 1%. Of course one could optimize further but I suggest to do the quick wins first with little risk of code regression.
Performance gains are mainly in formatter rules but also in relatively expensive UseSupportsShouldProcess rule.
Also remove unused method. Technically this is a breaking change due to them being publicly exposed but I don't think people use them, the only usage that I can imagine is in custom rules. I did a global GitHub search and it confirmed that no one is probably using those APIs.
This class is quite disgusting overall, littered with TODOs and private/public methods spread across the file. Idea is to keep changes minimal and safe.
PR Checklist
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
…% performance gain
Also remove unused method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Also, is there a good reason to use a linked list here? Surely it should just be a List<>?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Also, is there a good reason to use a linked list here? Surely it should just be a List<>?
When you look at the callers, they use the .Previous and .Next properties of a LL, which makes the code easier to read I have to say. With this we reduced the overhead from 8% to 1% so although one could optimize more, this was an easy way of achieving much without having to change to much in the spirit of the 80-20 rule. Only once other, bigger perf impacts have been optimized, I'd come back to optimize the LinkedList away completely as well or at least create it only once and then share it.
|
rjmholt reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
1 more reviewer
Reviewers whose approvals may not affect merge requirements
Uh oh!
There was an error while loading. Please reload this page.
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.