Skip to content

Reuse command info cache during a PS session #695

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
Merged
kapilmb merged 2 commits into from
Jan 24, 2017
Merged

Reuse command info cache during a PS session #695

kapilmb merged 2 commits into from
Jan 24, 2017

Conversation

@kapilmb
Copy link

@kapilmb kapilmb commented Jan 24, 2017 •
edited by raghushantha
Loading

Before

PS> 1..5 | % {measure-command {Invoke-ScriptAnalyzer ~\Source\Repos\PowerShellGet\PowerShellGet\PSModule.psm1}} | select TotalSeconds

TotalSeconds
------------
  25.9780775
   25.881099
  27.8301631
  28.9696967
  28.2952561

After

PS> 1..5 | % {measure-command {Invoke-ScriptAnalyzer ~\Source\PowerShellGet\PowerShellGet\PSModule.psm1}} | select
 TotalSeconds

TotalSeconds
------------
  28.1845615
   2.9607327
   3.1941642
   3.8798161
   3.0282973

This change is Reviewable

Kapil Borle added 2 commits January 23, 2017 15:44
Revert "Allow overwrite Helper instance"
889c49f 
This reverts commit 210d904.

We revert this because, this allows the instance to persist
through a powershell session across multiple pssa invocations.
This is allows us to reuse the command info cache across
multiple pssa invocations.
Reset command info cache only if it is null
94c5860 
Since, helper is a singleton that persists through multiple
pssa invocation, in a given powershell session, this
helps us reuse the commandinfo cache created during
the first pssa invocation. Results in significant perf
improvement for subsequent pssa invocations.
daviwil
daviwil reviewed Jan 24, 2017
View reviewed changes
Engine/Helper.cs
lock (syncRoot)
{
instance = value;
if (instance == null)
Copy link
Contributor

@daviwil daviwil Jan 24, 2017

Choose a reason for hiding this comment

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

Didn't this null check cause some other bug we saw recently?

Copy link
Author

@kapilmb kapilmb Jan 24, 2017

Choose a reason for hiding this comment

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

Yes it did cause some issue earlier - Get-ScriptAnalyzer command would instantiate Helper.Instance to new Helper() and when Invoke-ScriptAnalyzer would try to set the instance again, it would ignore the setter, which cause Invoke-ScriptAnalyzer to crash.

But after adding this, that bug was resolved and the removing this null check was redundant.

Copy link
Contributor

@daviwil daviwil Jan 24, 2017

Choose a reason for hiding this comment

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

Great!

@kapilmb
Copy link
Author

kapilmb commented Jan 24, 2017

Thanks @daviwil

@kapilmb kapilmb merged commit c23d0ae into development Jan 24, 2017
@kapilmb kapilmb deleted the kapilmb/FixPerformance branch January 24, 2017 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@daviwil daviwil daviwil 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

@kapilmb @daviwil @msftclas