FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Restore caching in Helper.GetCommandInfo by SeeminglyScience · Pull Request #1074 · PowerShell/PSScriptAnalyzer · GitHub

/ PSScriptAnalyzer Public

Restore caching in Helper.GetCommandInfo - #1074

Merged
James Truher (JamesWTruher) merged 2 commits into
PowerShell:developmentfrom
SeeminglyScience:reenable-command-caching
Oct 3, 2018
ConversationCommits (2)ChecksFiles changed
Merged

Restore caching in Helper.GetCommandInfo#1074
James Truher (JamesWTruher) merged 2 commits into
PowerShell:developmentfrom
SeeminglyScience:reenable-command-caching

Conversation

Patrick Meinecke (SeeminglyScience) commented Oct 1, 2018

Copy link
Copy Markdown
Collaborator

PR Summary

This change restores caching of CommandInfo objects in rules were it was previously disabled. My understanding is that it was disabled due to the fact that retrieving from the cache ignored the specified CommandTypes parameter, so the key for the cache is now a struct that takes both the command name, and the specified command types into account.

Performance is significantly improved in situations where either the script being analyzed is quite large, or has many unresolvable commands. For example, this build script previously took about 10 seconds to complete with just PSAvoidUsingCmdletAliases enabled. Now it is ~2 seconds for the first invocation and ~30-60ms for every subsequent invocation.

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets. Please mark anything not applicable to this PR NA.

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • NA - User facing documentation needed
  • Change is not breaking
  • NA - Make sure you've added a new test if existing tests do not effectively test the code changed
  • This PR is ready to merge and is not work in progress
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

Enable command lookup caching by name and filter
ae3c26c 
Add a struct to use as a private struct to use as a key for the command
lookup cache.  Also re-enable caching in Helper.GetCommandInfo.
Rob Holt (rjmholt) approved these changes Oct 1, 2018
View reviewed changes

Rob Holt (rjmholt) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM!

Comment thread Engine/Helper.cs Outdated
public override int GetHashCode()
{
// Algorithm from https://stackoverflow.com/questions/1646807/quick-and-simple-hash-code-combinations
unchecked

Rob Holt (rjmholt) Oct 1, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

On this part, worth reading through PowerShell/PowerShell#7134 and the comment it starts with. We discussed the hashcode stuff a bit, but not sure (1) how far we want to take the bikeshedding on hashcodes or (2) if there's a convenient function to do this in both .NET Core and .NET Framework

Christoph Bergmeister (bergmeister) Oct 1, 2018

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

PSSA already has many conditional compilation options for different version of powershell, therefore using a special, optimised version for .net core would be acceptable (the only restriction is that it should be part of .net standard 2.0 at the moment)

Rob Holt (rjmholt) Oct 1, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Another option might be new Tuple(Name.ToUpperInvariant(), CommandTypes).GetHashCode(). I don't think older framework versions have System.ValueTuple, so this would be forced to allocate :(

Christoph Bergmeister (bergmeister) Oct 1, 2018 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

There is a NuGet package for System.ValueTuple that supports full .net and .net core

Christoph Bergmeister (bergmeister) approved these changes Oct 1, 2018
View reviewed changes

Christoph Bergmeister (bergmeister) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for the performance improvement. After this PR has been merged, we could consider applying your new struct also to GetCommandInfoLegacy. One nit before I'd be happy merging the PR is to put the code of CommandLookupKey into its own file as the Helper class is already way too big.

Patrick Meinecke (SeeminglyScience) commented Oct 1, 2018

Copy link
Copy Markdown
Collaborator Author

Christoph Bergmeister (@bergmeister)

After this PR has been merged, we could consider applying your new struct also to GetCommandInfoLegacy.

The diff view on this one makes this really hard to see, but it already is applied to legacy. They both already pulled from the same cache, the new one just didn't add to it. Looks like the tests ran clean, but if there's any additional scenarios you'd like me to test to ensure the same issue didn't creep up let me know.

put the code of CommandLookupKey into its own file as the Helper class is already way too big.

Yeah I had the same thought. I kept it in the same file because it's only used by Helper so I was able to make it private nested. If I move it out I have to make it internal. Not a big deal obviously and I definitely don't mind doing that, just want to confirm you're aware and still want it moved.

Christoph Bergmeister (bergmeister) commented Oct 1, 2018

Copy link
Copy Markdown
Collaborator

Patrick Meinecke (@SeeminglyScience) Ok, thanks, it did not see before it was also in GetCommandInfoLegacy until I expanded the diff view. Yes, please put the struct into a separate file please, making it internal is fine.

Christoph Bergmeister (bergmeister) commented Oct 1, 2018 •
edited
Loading

Copy link
Copy Markdown
Collaborator

LGTM, I will leave this PR open for 1-2 days before I press the merge button in case James Truher (@JamesWTruher) or others wants to have a quick look at it as well or at least see/know about it.

James Truher (JamesWTruher) approved these changes Oct 3, 2018
View reviewed changes

James Truher (JamesWTruher) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

looks great - I'll merge it

James Truher (JamesWTruher) merged commit b5a19fa into PowerShell:development Oct 3, 2018
Closed
Closed
Jordan Borean (jborean93) mentioned this pull request Apr 14, 2019
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Christoph Bergmeister (bergmeister) bergmeister approved these changes
+2 more reviewers
Rob Holt (rjmholt) rjmholt approved these changes
James Truher (JamesWTruher) JamesWTruher approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL