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

Lazy initialisation of LinkedList in TokenOperations constructor -> 7% performance gain for formatter #1453

Conversation

bergmeister
Copy link
Collaborator

@bergmeister bergmeister commented Apr 18, 2020

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

Christoph Bergmeister added 2 commits April 18, 2020 23:47
Copy link
Contributor

@rjmholt rjmholt left a comment

Choose a reason for hiding this comment

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

Awesome!

@rjmholt
Copy link
Contributor

rjmholt commented Apr 20, 2020

Also, is there a good reason to use a linked list here? Surely it should just be a List<>?

@bergmeister
Copy link
Collaborator Author

bergmeister commented Apr 21, 2020

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.

@JamesWTruher JamesWTruher merged commit 022009b into PowerShell:master Apr 21, 2020
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.

None yet

3 participants