Skip to content

Conversation

@SteveL-MSFT
Copy link
Member

@SteveL-MSFT SteveL-MSFT commented Nov 2, 2018

PR Summary

It will be common for users to have at least pwsh and pwsh-preview on the system. Having them share the same ModuleAnalysisCache may result in unexpected behavior as the wrong module may be loaded. Fix is to add a 8 character hash derived from $PSHOME path.

Fix #8172

PR Checklist

@anmenaga
Copy link

anmenaga commented Nov 2, 2018

This is a breaking change, isn't it?

@SteveL-MSFT
Copy link
Member Author

@anmenaga the analysis cache file is an internal implementation detail and shouldn't be used directly by users

@daxian-dbw daxian-dbw merged commit 8eebc26 into PowerShell:master Nov 13, 2018
@SteveL-MSFT SteveL-MSFT deleted the analysis-cache-name branch November 13, 2018 19:34
{
featureNames[index++] = featureName.ToLowerInvariant();
}
hashBytes = sha1.ComputeHash(Encoding.UTF8.GetBytes(Utils.DefaultPowerShellAppBase));
Copy link
Member

Choose a reason for hiding this comment

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

Why use a cryptographic hash at all? Why not use .GetHashCode()?

Copy link
Collaborator

@iSazonov iSazonov Nov 15, 2018

Choose a reason for hiding this comment

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

We need a stable result but the GetHashCode could be randomized.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe GetHashCode() in .NET Core is now randomized as @iSazonov stated and we need something stable

Copy link
Member

Choose a reason for hiding this comment

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

CRC and MD5 are stable and both take less computation than SHA1. I don't believe this is a security issue, if it is, we shouldn't use SHA1. CRC is relatively simple to calculate we can implement it ourselves as I don't see anything to calculate it in DotNet Core.

Copy link
Collaborator

Choose a reason for hiding this comment

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

https://github.com/dotnet/corefx/search?utf8=%E2%9C%93&q=crc32&type=

but I don't understand how much we will win.

Copy link
Member

Choose a reason for hiding this comment

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

I presume that this has to be calculated at most every startup. Switching to a less intensive hash algorithm will reduce our startup time which is a big factor in how people perceive performance.

cc @daxian-dbw

Copy link
Member

Choose a reason for hiding this comment

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

@TravisEz13 and I discussed this offline yesterday and I agree that we should go with CRC. The main gain is to potentially avoid loading System.Security.Cryptography.Algorithms.dll at powershell startup time, plus there is also gain in CPU time when running the hash algorithm. I will do this change as part of my startup improvement work.

Copy link
Member

Choose a reason for hiding this comment

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

Got this chart from StackOverflow about the collision rate of CRC32:
image

In our scenario, hashing would be done on each powershell core installation on a machine, and in case of any experimental features is enabled, the combination of feature names. I think 100 different hash inputs should be a fair large number for a local machine environment in our scenario, and thus the collision rate would be less than 1 in a million, which should be good.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could use SSE from System.Runtime.Intrinsics.Experimental. We get this in runtime after .Net Core 3.0 release.

@iSazonov iSazonov added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Jan 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-Engine Indicates that a PR should be marked as an engine change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Each install of pwsh should have it's own ModuleAnalysisCache

6 participants