Skip to content

Use fast set location at startup #10401

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

Closed
iSazonov wants to merge 5 commits into from
Closed

Use fast set location at startup #10401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
045447f
Use fast set location at startup
iSazonov Aug 21, 2019
afe4d0f
Fix typo
iSazonov Aug 21, 2019
701ba94
Update src/System.Management.Automation/engine/SessionStateLocationAP…
iSazonov Sep 4, 2019
c264379
Remove Globber.IsAbsolutePath() check
iSazonov Sep 4, 2019
830d553
Revert "Remove Globber.IsAbsolutePath() check"
iSazonov Sep 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions src/System.Management.Automation/engine/InitialSessionState.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3259,20 +3259,17 @@ internal static void SetSessionStateDrive(ExecutionContext context, bool setLoca

if (proceedWithSetLocation && setLocation)
{
CmdletProviderContext providerContext = new CmdletProviderContext(context);

try
{
providerContext.SuppressWildcardExpansion = true;
context.EngineSessionState.SetLocation(Directory.GetCurrentDirectory(), providerContext);
context.EngineSessionState.SetLocationFileSystemFast(Directory.GetCurrentDirectory());
}
catch (ItemNotFoundException)
{
// If we can't access the Environment.CurrentDirectory, we may be in an AppContainer. Set the
// default drive to $pshome
System.Diagnostics.Process currentProcess = System.Diagnostics.Process.GetCurrentProcess();
string defaultPath = System.IO.Path.GetDirectoryName(PsUtils.GetMainModule(currentProcess).FileName);
context.EngineSessionState.SetLocation(defaultPath, providerContext);
context.EngineSessionState.SetLocationFileSystemFast(defaultPath);
}
}
}
Expand Down
73 changes: 73 additions & 0 deletions src/System.Management.Automation/engine/SessionStateLocationAPIs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,79 @@ internal PathInfo SetLocation(string path, CmdletProviderContext context, bool l
return this.CurrentLocation;
}

/// <summary>
/// Changes the current working directory to the path specified.
/// It is only for FileSystem provider paths and optimized to be fast.
/// </summary>
/// <param name="path">
/// The file system path of the new current working directory.
/// </param>
/// <exception cref="ArgumentNullException">
/// If <paramref name="path"/> is null.
/// </exception>
/// <exception cref="DriveNotFoundException">
/// If <paramref name="path"/> refers to a drive that does not exist.
/// </exception>
internal void SetLocationFileSystemFast(string path)
{
if (path == null)
{
throw PSTraceSource.NewArgumentNullException(nameof(path));
}

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

@TravisEz13 TravisEz13 Sep 3, 2019

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@iSazonov iSazonov Sep 4, 2019

Choose a reason for hiding this comment

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

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."?

Copy link
Member

@TravisEz13 TravisEz13 Sep 4, 2019

Choose a reason for hiding this comment

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

I can understand that much better. Please use that.

current = CurrentLocation;
}

PSDriveInfo previousWorkingDrive = CurrentDrive;

if (LocationGlobber.IsProviderDirectPath(path))
{
// The path is a provider-direct (drive is implicitly in the provider path) path so use the current
// provider and its hidden drive but don't modify the path
// at all.
ProviderInfo provider = CurrentLocation.Provider;
CurrentDrive = provider.HiddenDrive;
}
else
{
// See if the path is a relative or absolute
// path.
if (Globber.IsAbsolutePath(path, out string driveName))
Copy link
Collaborator Author

@iSazonov iSazonov Aug 30, 2019

Choose a reason for hiding this comment

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

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.

{
// Since the path is an absolute path
// we need to change the current working
// drive
CurrentDrive = GetDrive(driveName);

// Now that the current working drive is set,
// process the rest of the path as a relative path.
}
}

string currentPath = path.Substring(CurrentDrive.Root.Length);
Copy link
Member

@daxian-dbw daxian-dbw Sep 4, 2019

Choose a reason for hiding this comment

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

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\

Copy link
Collaborator Author

@iSazonov iSazonov Sep 5, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member

@daxian-dbw daxian-dbw Sep 5, 2019

Choose a reason for hiding this comment

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

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.


CurrentDrive.CurrentLocation = currentPath;

// Now make sure the current drive is set in the provider's
// current working drive hashtable
ProvidersCurrentWorkingDrive[CurrentDrive.Provider] = CurrentDrive;

// Set the $PWD variable to the new location
this.SetVariable(SpecialVariables.PWDVarPath, this.CurrentLocation, false, true, CommandOrigin.Internal);

// If an action has been defined for location changes, invoke it now.
if (PublicSessionState.InvokeCommand.LocationChangedAction != null)
{
var eventArgs = new LocationChangedEventArgs(PublicSessionState, current, CurrentLocation);
PublicSessionState.InvokeCommand.LocationChangedAction.Invoke(ExecutionContext.CurrentRunspace, eventArgs);
s_tracer.WriteLine("Invoked LocationChangedAction");
}
}

/// <summary>
/// Determines if the specified path is the current working directory
/// or a parent of the current working directory.
Expand Down