Use fast set location at startup #10401

iSazonov · 2019-08-21T12:32:51Z

PR Summary

At startup we create initial runspace and set default location to current file system location (path).
For that we use generic SetLocation() which designed to work with all providers and do many extra checks/operations.
In the startup scenario we know that the path is valid file system path so we can use more simple and more fast set location.

On my system I get win ~10ms or ~1.62% and ~8Mb or ~3%.
The win comes from removing some extra file operations:

PR Context

Related #6443

PR Checklist

bergmeister · 2019-08-21T12:55:42Z

In the startup scenario we know that the path is valid file system path so we can use more simple and more fast set location.

What about the Azure Cloud Shell, which starts by default in the Azure drive, which is a SHiPS provider for a PSDrive?

Requesting a Cloud Shell.Succeeded.
Connecting terminal...
Welcome to Azure Cloud Shell

Type "az" to use Azure CLI
Type "help" to learn about Cloud Shell

get-locationMOTD: Discover installed Azure modules: Get-Module Az* -ListAvailableVERBOSE: Authenticating to Azure ...VERBOSE: Building your Azure drive ...
Azure:/
PS Azure:\> Get-Location

Path
----
Azure:/

PS Azure:\> Get-PSDrive

Name           Used (GB)     Free (GB) Provider      Root                                                                                                                                                                    CurrentLocation
----           ---------     --------- --------      ----                                                                                                                                                                    ---------------
/                  19.71         28.70 FileSystem    /                                                                                                                                                                        home/christoph
Alias                                  Alias
Azure                                  SHiPS         AzurePSDrive#Azure
Env                                    Environment
Function                               Function
Variable                               Variable

iSazonov · 2019-08-21T13:00:42Z

What about the Azure Cloud Shell, which starts by default in the Azure drive?

I guess it is loaded after initial startup and also it is remoting scenario.

SteveL-MSFT · 2019-08-21T15:41:11Z

@bergmeister I'm pretty sure CloudShell uses the PowerShell profile to create the Azure drive, logon, and Set-Location so all that is after pwsh startup anyways.

iSazonov · 2019-08-22T03:05:55Z

Perhaps this code can still be improved, but it also works for Linux where I can’t debug :-(

daxian-dbw · 2019-08-29T21:30:57Z

@TravisEz13 and @adityapatwardhan Given you two have experiences with FileSystemProvider related code, could you please review this PR when you have time? Thanks!

iSazonov · 2019-08-30T03:29:35Z

src/System.Management.Automation/engine/SessionStateLocationAPIs.cs

+            {
+                // See if the path is a relative or absolute
+                // path.
+                if (Globber.IsAbsolutePath(path, out string driveName))


I feel this could be optimized too but I don't know how this works on Unix and I afraid to make breaking change.

Additional thought is that the branch works only on Unix and at startup we possibly already initialized current drive and no need to repeat the step.

TravisEz13

@daxian-dbw I only have experience with the location globber.

TravisEz13 · 2019-09-03T21:19:33Z

src/System.Management.Automation/engine/SessionStateLocationAPIs.cs

+            PathInfo current = null;
+            if (PublicSessionState.InvokeCommand.LocationChangedAction != null)
+            {
+                // It is under a condition to avoid the unneeded allocation - CurrentLocation always allocates.


I don't understand your comment. The two parts of the sentence seem to conflict. How about this:

Suggested change

// It is under a condition to avoid the unneeded allocation - CurrentLocation always allocates.

// Allocate a new location using the CurrentLocation property

I want protect the condition from removing because CurrentLocation always allocates and we should avoid the allocation if we don't use CurrentLocation in code below. Your proposal doesn't reflect this.
What about "CurrentLocation always allocates and we should avoid the allocation if we don't use CurrentLocation later."?

I can understand that much better. Please use that.

src/System.Management.Automation/engine/SessionStateLocationAPIs.cs

…Is.cs Co-Authored-By: Travis Plunk <[email protected]>

src/System.Management.Automation/engine/InitialSessionState.cs

This reverts commit c264379.

src/System.Management.Automation/engine/InitialSessionState.cs

daxian-dbw

I'm generally concerned about the risk of changing the provider code for perf purpose, especially when the gain is insignificant, as the code can be exercised in so many different ways, it's hard for us to enumerate all the scenarios that might be affected.
I think it's worth for the committee to take this PR as case for discussion, to think about what the guidelines/balance should be for performance changes and sensitive code.

daxian-dbw · 2019-09-04T19:55:12Z

src/System.Management.Automation/engine/SessionStateLocationAPIs.cs

+                }
+            }
+
+            string currentPath = path.Substring(CurrentDrive.Root.Length);


I can see there would be problem to this line. Say CurrentDrive is the Temp drive, and the path passed is Temp:\blah, then this Substring call will throw exception.

PS:121> $temp.Root C:\Users\AUser\AppData\Local\Temp\

If current drive is Temp:/ we always call IsAbsolutePath() and always set current drive based on the path. I checked this for shorter code path that is ResetRunspaceState(). Other code paths call previously Bind(), there is a call SetSessionStateDrive(), then again SetSessionStateDrive() and we fall in IsAbsolutePath() too.

See the following repro:

cd temp:\ mkdir test $t =$ExecutionContext.SessionState.GetType() $p = $t.GetProperties("nonpublic, instance") $p = $p[0] $v = $p.GetValue($ExecutionContext.SessionState) $t = $v.GetType() ## Get the existing 'SetLocation(string)' method $m1 = $t.GetMethod("SetLocation", [System.Reflection.BindingFlags]"nonpublic, instance", $null, [type[]]@([string]), $null) ## Success. Location is set to 'Temp:\test' $m1.Invoke($v, @("Temp:\test")) ## Get the new 'SetLocationFileSystemFast' method and invoke it. $m2 = $t.GetMethod("SetLocationFileSystemFast", [System.Reflection.BindingFlags]"nonpublic, instance") ## Fail with 'ArgumentOutOfRangeException' $m2.Invoke($v, @("Temp:\test")) > Exception calling "Invoke" with "2" argument(s): "startIndex cannot be larger than length of string. (Parameter 'startIndex')"

I know you have examined the current existing call sites of this method and know they are safe, but the fast-code-path method is internal, and it could be used by someone in other places in future, without knowing the prerequisites or assumptions for this method to work. This is not the right pattern to follow, it will results code hard to maintain.

joeyaiello · 2019-09-04T22:49:05Z

@PowerShell/powershell-committee reviewed this one, and unfortunately we think we're going to have to reject it.

We're concerned about the risk of unknown behavioral changes. We just don't know enough about what this does to make a statement that this is an okay change to make. For example, as an internal API, other parts of the code could be depending on the non-fast path.
The performance impact is really too negligible for it to be worth the risk: 1.6% startup time is statistically insignificant, and any RAM savings is going to be gone as soon as PowerShell does anything with the filesystem.

iSazonov · 2019-09-05T05:56:11Z

For example, as an internal API, other parts of the code could be depending on the non-fast path.

It is not true. We always call the code with Directory.GetCurrentDirectory() or System.IO.Path.GetDirectoryName(PsUtils.GetMainModule(currentProcess).FileName) that is always a file system file in predictable form. No other entry points exist. So there is no unpredictable risk - we can review all code paths.

The performance impact is really too negligible for it to be worth the risk

My intention was and remains to go through all startup code and remove delays where possible. I would not be surprised if in the end we would get a 20-30 percent increase in performance. (Is startup scenario important?)
After the PR we could optimize the code even more.
We call SetSessionStateDrive() 4 times at startup. We could call only 2 times.
In SetSessionStateDrive() we set current drive and then set location that do again set current drive. We could look if we can avoid the duplication.

Is this enough to continue the work?

daxian-dbw · 2019-09-05T19:01:32Z

No other entry points exist. So there is no unpredictable risk - we can review all code paths.

Again, no other entry points exists today doesn't mean it won't in future, because the new API is internal. This is not a good pattern to follow.

We call SetSessionStateDrive() 4 times at startup. We could call only 2 times.

This was already done by #10416, wasn't it? And that PR has been accepted.

iSazonov · 2019-09-06T12:57:43Z

I rebased to get #10416 in the branch only now. :-) I see we set a location only once at startup.
I believe that we will not be able to speed up the startup scenario by doing something in one place. I think we will have to review all startup code and make small improvements to many subsystems. The set location code is a place where we can still get perf win in some ms. This will seed up creation of runspaces in all scenarios too.
I added new commit where made the method private and move to SessionStateInternal class with SetLocationStateDrive() method. I hope this addresses your concern about internal API.

There is still a comment about AppContainer and I don't still understand how address this after removing globbing code. It seems we need to check that Directory.GetCurrentDirectory() exists?

Thank you for great comments and help!

daxian-dbw · 2019-09-06T17:04:26Z

SessionStateInternal class with SetLocationStateDrive() method. I hope this addresses your concern about internal API.

My concerns are not just it being internal, but in general that it's risky to change the provider code. We cannot enumerate all possible scenarios that exercise the code path. If I can quickly find one potential problem just by looking at the code (which I'm not familiar with), then that's a signal that there may be more issues with the changes that are unknown yet.

My personal principles on performance work are:

the work should be well-guided by profiling;
it's either low hanging fruits that can be easily reviewed with low risks, or more fundamental algorithm level changes that provide significant perf gain;
the most important one -- no behavioral changes should be introduced for a perf changes, unless it's an approved one.

iSazonov · 2019-09-09T08:51:07Z

@daxian-dbw Thanks! Sorry if I noisy.

In the PR we do not do behavioral changes in the PR.
SetSessionStateDrive() method code and comments say that we use always and exclusively FileSystem provider to set init drive and always set location to Directory.GetCurrentDirectory() if requested.
It seems the method name is misleading. Perhaps InitSessionStateDriveToFileSystemDrive() would be better.
Moreover, this code is enclosed in try-catch and all exceptions is ignored. Even if something happens in the original code, we probably don’t even see it too. We see only the consequences that is another current location than Directory.GetCurrentDirectory() but we have also another path for AppContainer which implies that no script should depend on this.

There are only two scenarios where we set a folder location (that only the PR care).
First, runspace creation scenario, we will see wrong prompt in console. Currently it works well.
Then, we will have init file system location not in Directory.GetCurrentDirectory(). I set current drive to Temp:/ and create new runspace - this works well too. We could add the test.
Second, there is a runspace reset scenario. I can not found how could we test this but at first look it is the same as runspace creation scenario - in the sense that if creation works, then the reset works too.

Last notice is that single scenario where set location is important is startup one because we want get right prompt. Remaining is not important because we use OS current path for initialization and it is not predictable and reliable source in multi runspace environment which means that nobody should lean on the value and set needed location explicitly in current runspace.

Now мy only concern is with AppContainer scenario, which is mentioned in the comments. I do not understand how this can happen and how debug it. It looks like Directory.GetCurrentDirectory() returns nonexistent value in AppContainer.
Update: I run the PR build in Windows container and pwsh is started with right prompt. Not tested for Linux container. Perhaps there was an issue with .Net Core 1.0?

iSazonov · 2019-09-17T11:04:11Z

@joeyaiello @daxian-dbw
I added some commits to make the code more clean and understandable and measure a runspace creation scenario with follow script:

measure-Command {
for( $i=0; $i -lt 100; $i++) {
    [runspace] $rs1 = [runspacefactory]::CreateRunspace()
    [runspace] $rs2 = [runspacefactory]::CreateRunspace()
    [runspace] $rs3 = [runspacefactory]::CreateRunspace()
    [runspace] $rs4 = [runspacefactory]::CreateRunspace()
    [runspace] $rs5 = [runspacefactory]::CreateRunspace()
    [runspace] $rs6 = [runspacefactory]::CreateRunspace()
    [runspace] $rs7 = [runspacefactory]::CreateRunspace()
    [runspace] $rs8 = [runspacefactory]::CreateRunspace()
    [runspace] $rs9 = [runspacefactory]::CreateRunspace()
    [runspace] $rs10 = [runspacefactory]::CreateRunspace()

    $rs1.Open()
    $rs2.Open()
    $rs3.Open()
    $rs4.Open()
    $rs5.Open()
    $rs6.Open()
    $rs7.Open()
    $rs8.Open()
    $rs9.Open()
    $rs10.Open()

    $rs1.Dispose()
    $rs2.Dispose()
    $rs3.Dispose()
    $rs4.Dispose()
    $rs5.Dispose()
    $rs6.Dispose()
    $rs7.Dispose()
    $rs8.Dispose()
    $rs9.Dispose()
    $rs10.Dispose()
}

}

Before the change:

Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 18
Milliseconds      : 510
Ticks             : 185101449
TotalDays         : 0.000214237788194444
TotalHours        : 0.00514170691666667
TotalMinutes      : 0.308502415
TotalSeconds      : 18.5101449
TotalMilliseconds : 18510.1449

After the change:

Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 10
Milliseconds      : 654
Ticks             : 106545000
TotalDays         : 0.000123315972222222
TotalHours        : 0.00295958333333333
TotalMinutes      : 0.177575
TotalSeconds      : 10.6545
TotalMilliseconds : 10654.5

That is 41.5% for the samples (~8 ms per 1 runspace).
Perhaps this is not very much for startup scenario (~2%) but clearly visible for scenarios that actively use runspaces.

daxian-dbw · 2019-09-17T16:55:00Z

Interesting. I will take a close look this week. Currently working on driving the preview.4 release.

iSazonov · 2019-09-23T03:21:01Z

PerfView screenshot

Bind takes 26.8% (that is nearly 100% in AutomationEngine constructor) including UpdatesType() - 13.4% and SetSessionDrive() - 12.0. The both is good candidates for perf investigations.

daxian-dbw · 2019-09-26T00:12:44Z

I added some commits to make the code more clean and understandable ...

@iSazonov I don't see new commits here. Are there new commits in your branch that are not shown here?

iSazonov · 2019-09-26T06:42:07Z

@daxian-dbw Closed PR doesn't show new commits. :-)
master...iSazonov:perf-startup-setlocationfast
I can pull new PR if you want.

iSazonov · 2019-10-17T03:33:28Z

@daxian-dbw @SteveL-MSFT I'd want to bring back the change. I still believe it is useful for startup and related scenarios.

daxian-dbw · 2020-02-11T17:32:24Z

@PoshChan Please remind me in 24 hours

PoshChan · 2020-02-12T17:32:49Z

@daxian-dbw, this is the reminder you requested 24 hours ago

Use fast set location at startup

045447f

iSazonov added the CL-Performance Indicates that a PR should be marked as a performance improvement in the Change Log label Aug 21, 2019

iSazonov added this to the 7.0.0-preview.4 milestone Aug 21, 2019

iSazonov requested review from SteveL-MSFT and daxian-dbw August 21, 2019 12:32

Fix typo

afe4d0f

SteveL-MSFT assigned daxian-dbw Aug 21, 2019

daxian-dbw requested review from TravisEz13 and adityapatwardhan August 21, 2019 19:53

iSazonov mentioned this pull request Aug 22, 2019

Cleanup AutomationEngine and remove extra SetSessionStateDrive method call #10416

Merged

14 tasks

daxian-dbw removed their request for review August 29, 2019 21:29

iSazonov commented Aug 30, 2019

View reviewed changes

TravisEz13 requested changes Sep 3, 2019

View reviewed changes

ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Sep 3, 2019

iSazonov and others added 2 commits September 4, 2019 09:39

Update src/System.Management.Automation/engine/SessionStateLocationAP…

701ba94

…Is.cs Co-Authored-By: Travis Plunk <[email protected]>

Remove Globber.IsAbsolutePath() check

c264379

PoshChan reviewed Sep 4, 2019

View reviewed changes

src/System.Management.Automation/engine/InitialSessionState.cs Show resolved Hide resolved

Revert "Remove Globber.IsAbsolutePath() check"

830d553

This reverts commit c264379.

PoshChan reviewed Sep 4, 2019

View reviewed changes

src/System.Management.Automation/engine/InitialSessionState.cs Show resolved Hide resolved

daxian-dbw added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Sep 4, 2019

daxian-dbw reviewed Sep 4, 2019

View reviewed changes

joeyaiello closed this Sep 4, 2019

joeyaiello removed the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Sep 4, 2019

joeyaiello added the Committee-Reviewed PS-Committee has reviewed this and made a decision label Sep 4, 2019

iSazonov mentioned this pull request Oct 25, 2019

MSIX pkg starts in system32 instead of user's home directory #10884

Closed

	// It is under a condition to avoid the unneeded allocation - CurrentLocation always allocates.
	// Allocate a new location using the CurrentLocation property

Use fast set location at startup #10401

Use fast set location at startup #10401

Uh oh!

Conversation

iSazonov commented Aug 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

bergmeister commented Aug 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Aug 21, 2019

Uh oh!

SteveL-MSFT commented Aug 21, 2019

Uh oh!

iSazonov commented Aug 22, 2019

Uh oh!

daxian-dbw commented Aug 29, 2019

Uh oh!

iSazonov Aug 30, 2019

Choose a reason for hiding this comment

Uh oh!

TravisEz13 left a comment

Choose a reason for hiding this comment

Uh oh!

TravisEz13 Sep 3, 2019

Choose a reason for hiding this comment

Uh oh!

iSazonov Sep 4, 2019

Choose a reason for hiding this comment

Uh oh!

TravisEz13 Sep 4, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daxian-dbw left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Sep 4, 2019

Choose a reason for hiding this comment

Uh oh!

iSazonov Sep 5, 2019

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Sep 5, 2019

Choose a reason for hiding this comment

Uh oh!

joeyaiello commented Sep 4, 2019

Uh oh!

iSazonov commented Sep 5, 2019

Uh oh!

daxian-dbw commented Sep 5, 2019

Uh oh!

iSazonov commented Sep 6, 2019

Uh oh!

daxian-dbw commented Sep 6, 2019

Uh oh!

iSazonov commented Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Sep 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daxian-dbw commented Sep 17, 2019

Uh oh!

iSazonov commented Sep 23, 2019

Uh oh!

daxian-dbw commented Sep 26, 2019

Uh oh!

iSazonov commented Sep 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Oct 17, 2019

Uh oh!

iSazonov commented Aug 21, 2019 •

edited

Loading

bergmeister commented Aug 21, 2019 •

edited

Loading

daxian-dbw left a comment •

edited

Loading

iSazonov commented Sep 9, 2019 •

edited

Loading

iSazonov commented Sep 17, 2019 •

edited

Loading

iSazonov commented Sep 26, 2019 •

edited

Loading