Update `PSConfiguration.ReadValueFromFile` to make it faster and more memory efficient by daxian-dbw · Pull Request #10839 · PowerShell/PowerShell

daxian-dbw · 2019-10-18T23:36:21Z

PR Summary

There are 2 changes included here:

Change how configuration file is accessed to make it faster and memory efficient.
Change TypeCatalogGen to generate slightly faster dictionary initialization code.

The TypeCatalogGen is more of a cleanup change, to use the collection initializer, which is slightly faster than the dict[xxx] = "..." operation.

Benchmark results for the `PSConfiguration` change

For the PSConfiguration change, I ran a benchmark. The benchmark code and results can be found below.
The new implementation is a lot faster than the current, but overall it won't affect much the startup of pwsh, because we spent very little time in PSConfiguraiton.
The new implementation has a lot less allocation (88.63KB vs. 1.69KB). This is the more interesting part.

    class Program
    {
        static void Main(string[] args)
        {
            var summary = BenchmarkRunner.Run<Benchmark_ReadConfigData>();
        }
    }

    [DisassemblyDiagnoser(printAsm: true, printSource: true, recursiveDepth: 2)]
    [MemoryDiagnoser]
    public class Benchmark_ReadConfigData
    {
        [Benchmark(Baseline = true)]
        public void ReadConfigUsingCurrentImpl()
        {
            // Those methods are called during the startup of 'pwsh', so using them as the operations for the benchmark.
            PowerShellConfig.Instance.GetExperimentalFeatures();
            PowerShellConfig.Instance.GetModulePath(ConfigScope.AllUsers);
            PowerShellConfig.Instance.GetModulePath(ConfigScope.CurrentUser);
            PowerShellConfig.Instance.GetPowerShellPolicies(ConfigScope.CurrentUser);
            PowerShellConfig.Instance.GetPowerShellPolicies(ConfigScope.AllUsers);
            PowerShellConfig.Instance.GetExecutionPolicy(ConfigScope.CurrentUser, "Microsoft.PowerShell");
            PowerShellConfig.Instance.GetExecutionPolicy(ConfigScope.AllUsers, "Microsoft.PowerShell");
        }

        [Benchmark]
        public void ReadConfigUsingNewImpl()
        {
            // Those methods are called during the startup of 'pwsh', so using them as the operations for the benchmark.
            NewPowerShellConfig.Instance.GetExperimentalFeatures();
            NewPowerShellConfig.Instance.GetModulePath(ConfigScope.AllUsers);
            NewPowerShellConfig.Instance.GetModulePath(ConfigScope.CurrentUser);
            NewPowerShellConfig.Instance.GetPowerShellPolicies(ConfigScope.CurrentUser);
            NewPowerShellConfig.Instance.GetPowerShellPolicies(ConfigScope.AllUsers);
            NewPowerShellConfig.Instance.GetExecutionPolicy(ConfigScope.CurrentUser, "Microsoft.PowerShell");
            NewPowerShellConfig.Instance.GetExecutionPolicy(ConfigScope.AllUsers, "Microsoft.PowerShell");
        }
    }

PR Checklist

…zation code

…y efficient

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

iSazonov · 2019-10-19T15:58:57Z

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

+        private readonly JObject[] configRoots = new JObject[2];
+        private readonly JObject emptyConfig;
+        private readonly JsonSerializer serializer;


Should we use _ prefix for private field names?

We initialize configRoots here and in constructor too.

We could initialize configRoots with " empty config" that could simplify to introduce "DefaultConfig".

For (1) I don't plan to make that change in this PR given it's a style change that will make the PR harder to review.
For (2) good catch. Fixed.
For (3) emtpyConfig means we have attempted reading the file, and there is not configuration defined in the corresponding file (either an empty file, or file doesn't exist). So configRoots cannot be initialized to emptyConfig. They mean different things.

For (3) In the case emptyConfig is not reflect the purpose.

Can you elaborate why you think it doesn't reflect the purpose?

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

src/TypeCatalogGen/TypeCatalogGen.cs

test/xUnit/csharp/test_PSConfiguration.cs

iSazonov · 2019-10-22T04:19:54Z

src/TypeCatalogGen/TypeCatalogGen.cs

            foreach (KeyValuePair<string, TypeMetadata> pair in typeNameToAssemblyMap)
            {
-                sourceCode.AppendLine(string.Format(CultureInfo.InvariantCulture, SourceFormat, pair.Key, pair.Value.AssemblyName));
+                sourceCode.Append(string.Format(CultureInfo.InvariantCulture, SourceFormat, pair.Key, pair.Value.AssemblyName, Environment.NewLine));


Environment.NewLine on first position it would be more clear.

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

ghost · 2019-11-21T22:35:49Z

🎉v7.0.0-preview.6 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

daxian-dbw added 2 commits October 18, 2019 11:04

Update TypeCatalogGen to generate slightly faster dictionary initiali…

9d5233c

…zation code

Change how configuraiton file is accessed to make it faster and memor…

8f01199

…y efficient

PoshChan reviewed Oct 18, 2019

View reviewed changes

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

daxian-dbw added 2 commits October 18, 2019 22:57

Fix a bug

29a5247

Make the singleton field readonly

4218bd5

PoshChan reviewed Oct 19, 2019

View reviewed changes

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

Fix tests

d3de435

PoshChan reviewed Oct 19, 2019

View reviewed changes

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

Add back the missing using directive

018fa21

iSazonov reviewed Oct 19, 2019

View reviewed changes

daxian-dbw added 2 commits October 20, 2019 22:43

Fix CodeFactor/Codacy issues

e649eae

Address Ilya's comments

875a42a

daxian-dbw changed the title ~~WIP: Update PSConfiguration.ReadValueFromFile to make it faster and more memory efficient~~ Update PSConfiguration.ReadValueFromFile to make it faster and more memory efficient Oct 21, 2019

daxian-dbw assigned adityapatwardhan Oct 21, 2019

daxian-dbw requested review from adityapatwardhan and rjmholt October 21, 2019 19:44

iSazonov reviewed Oct 22, 2019

View reviewed changes

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

adityapatwardhan approved these changes Oct 30, 2019

View reviewed changes

adityapatwardhan merged commit 7772418 into PowerShell:master Oct 30, 2019

adityapatwardhan added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Oct 30, 2019

adityapatwardhan added this to the 7.0.0-preview.6 milestone Oct 30, 2019

daxian-dbw deleted the perf branch November 1, 2019 06:25

daxian-dbw added CL-Performance Indicates that a PR should be marked as a performance improvement in the Change Log and removed CL-Engine Indicates that a PR should be marked as an engine change in the Change Log labels Nov 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update `PSConfiguration.ReadValueFromFile` to make it faster and more memory efficient#10839

Update `PSConfiguration.ReadValueFromFile` to make it faster and more memory efficient#10839
adityapatwardhan merged 8 commits intoPowerShell:masterfrom
daxian-dbw:perf

daxian-dbw commented Oct 18, 2019 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iSazonov Oct 19, 2019

Uh oh!

daxian-dbw Oct 21, 2019

Uh oh!

iSazonov Oct 22, 2019

Uh oh!

daxian-dbw Oct 22, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iSazonov Oct 22, 2019

Uh oh!

Uh oh!

ghost commented Nov 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

daxian-dbw commented Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Benchmark results for the PSConfiguration change

PR Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iSazonov Oct 19, 2019

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

iSazonov Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iSazonov Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghost commented Nov 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

daxian-dbw commented Oct 18, 2019 •

edited

Loading

Benchmark results for the `PSConfiguration` change