Merge TypesV3_Ps1Xml.cs with Types_Ps1Xml.cs by iSazonov · Pull Request #10575 · PowerShell/PowerShell

iSazonov · 2019-09-19T10:09:19Z

PR Summary

Move all type definitions from TypesV3_Ps1Xml.cs to Types_Ps1Xml.cs
Simplify the code that processed them.

PR Context

I remember Jason's comment that make no sense to have separate TypesV3_Ps1Xml.cs file.
I discovered this while did a performance measure runspace creation. There is biggest delay on type importing (up to 30%!). I don't expect that the change visibly increase performance but it open a way for further investigations.

PR Checklist

This reverts commit 1042dfb.

daxian-dbw · 2019-09-20T17:51:50Z

I think merging typev3.ps1xml content with types.ps1xml is not a breaking change, but @JamesWTruher can you please take a quick look and see if you have any concerns?

JamesWTruher

from a code perspective, I don't think there's anything wrong with this, other than my suggestions below. However, I'm wondering why we need this code at all any longer. We don't need back-compat with V3 and we've eliminated the need to have references to the files at all. Could we remove references to *types.ps1xml entirely and just rely on the .cs files?

JamesWTruher · 2019-09-20T18:01:59Z

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

@@ -1417,7 +1417,6 @@ private static InitialSessionState CreateRestrictedForRemoteServer()

            List<string> allowedTypes = new List<string>();


Suggested change

List<string> allowedTypes = new List<string>();

List<string> allowedTypes = new List<string>() { "types.ps1xml" };

JamesWTruher · 2019-09-20T18:02:13Z

src/System.Management.Automation/singleshell/config/MshSnapinInfo.cs


            // System.Management.Automation formats & types files
-            Collection<string> types = new Collection<string>(new string[] { "types.ps1xml", "typesv3.ps1xml" });
+            Collection<string> types = new Collection<string>();


Suggested change

Collection<string> types = new Collection<string>();

Collection<string> types = new Collection<string>() { "types.ps1xml" };

JamesWTruher · 2019-09-20T18:02:20Z

src/System.Management.Automation/singleshell/config/MshSnapinInfo.cs

                         "Help.format.ps1xml","HelpV3.format.ps1xml","PowerShellCore.format.ps1xml","PowerShellTrace.format.ps1xml",
                         "Registry.format.ps1xml"});
-            Collection<string> smaTypes = new Collection<string>(new string[] { "types.ps1xml", "typesv3.ps1xml" });
+            Collection<string> smaTypes = new Collection<string>();


Suggested change

Collection<string> smaTypes = new Collection<string>();

Collection<string> smaTypes = new Collection<string>() { "types.ps1xml" };

iSazonov · 2019-09-20T18:59:42Z

Could we remove references to *types.ps1xml entirely and just rely on the .cs files?

Now types.ps1xml is only a key to reference predefined types (also there is still GetEvent.types.ps1xml) which we load in each runspace at first step. It is not perf critical and I believe we could refactor the code. At least the code could become more readable.
Main problem is that we clone TypeTable per runspace creation and do this complex and relatively slow. I have still no ideas how improve this. Obvious idea is exclude cloning type table with predefined types and reuse it. I am not sure that it is simple to implement.

iSazonov · 2019-10-08T06:06:45Z

@daxian-dbw Could you please review and merge?

daxian-dbw

This change turns out to be potentially breaking. Not sure if we should proceed now.
Given the necessity of this change is low, I tend to play conservatively and avoid the breaking change.

daxian-dbw · 2019-10-08T22:45:46Z

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

                ProcessTypeData(filePath, errors, Types_Ps1Xml.Get());
                result = true;
            }
-            else if (string.Equals(Path.Combine(psHome, "typesv3.ps1xml"), filePath, StringComparison.OrdinalIgnoreCase))


This method is used by public TypeTable(IEnumerable<string> typeFiles). This is a public API and this change is potentially a breaking change if the typeFiles passed to that public constructor contains types3.ps1xml.

I added new commit to partially address the breaking.

After that I hope the breaking change will be in gray area.
The only scenario that we cannot fix is to load 'typesv3.ps1xml' file without 'types.ps1xml'. This scenario is incredible.

daxian-dbw · 2019-10-09T18:23:31Z

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

+            {
+                // Workaround for a scenario where we get the 'typesv3.ps1xml' file name from public API.
+                // Now all types from 'typesv3.ps1xml' is in 'types.ps1xml' so we load 'types.ps1xml'.
+                ProcessTypeData("types.ps1xml", errors, Types_Ps1Xml.Get());


This means types.ps1xml will be processed twice, which could potentially cause issue that I don't know of (conflicts?).

There GetOrAdd() is used. (But it will slow down)

I'd prefer a breaking change (gray area) and get rid typesv3.ps1xml (and then types.ps1xml as James asked). It looks like very rarely edge case.
GitHub search https://github.com/search?q=TypeTable+typesv3.ps1xml&type=Code
This is certainly not an exhaustive test, but it returned nothing excluding PowerShell code.
This open way to make the code faster (currently ~30% in runspace creation).

This open way to make the code faster (currently ~30% in runspace creation).

I don't think merging those two source files and eliminating types3.ps1xml would help much on the perf here.
I will let the committee review the proposed change.

I don't think merging those two source files and eliminating types3.ps1xml would help much on the perf here.

My thought is that it opens a way for future optimizations.

Just took a look at the code in TypeTable.cs, and duplicate TypeData will result in error during processing:

PowerShell/src/System.Management.Automation/engine/TypeTable.cs

Lines 2698 to 2702 in 36bc894

if (membersCollection[member.name] != null && !isOverride)

{

AddError(errors, typeName, TypesXmlStrings.DuplicateMember, member.name);

return;

}

The GetOrAdd() is only to get the member collection of the type. Error will happen when processing members of the type.

I tried the compiled artifact and get the error. If PowerShell Committee conclusion will be to have a workaround I will discover another workaround but I hope we will agree to remove.

daxian-dbw · 2019-10-31T19:29:13Z

Given that Types_Ps1Xml.cs, TypesV3_Ps1Xml.cs and GetEvent_Types_Ps1Xml.cs are removed in #10898, I think this PR can be closed now.

iSazonov added 2 commits September 19, 2019 14:54

Move types from TypesV3_Ps1Xml.cs to Types_Ps1Xml.cs

ddc1849

Move types from GetEvent_Types_Ps1Xml.cs to Types_Ps1Xml.cs

1042dfb

iSazonov added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label Sep 19, 2019

iSazonov added this to the 7.0.0-preview.5 milestone Sep 19, 2019

iSazonov requested review from SteveL-MSFT and daxian-dbw September 19, 2019 10:09

Revert "Move types from GetEvent_Types_Ps1Xml.cs to Types_Ps1Xml.cs"

53b465a

This reverts commit 1042dfb.

iSazonov changed the title ~~Merge TypesV3_Ps1Xml.cs and GetEvent_Types_Ps1Xml.cs to Types_Ps1Xml.cs~~ Merge TypesV3_Ps1Xml.cs to Types_Ps1Xml.cs Sep 19, 2019

iSazonov changed the title ~~Merge TypesV3_Ps1Xml.cs to Types_Ps1Xml.cs~~ Merge TypesV3_Ps1Xml.cs with Types_Ps1Xml.cs Sep 19, 2019

Cleanup code

114dc29

iSazonov force-pushed the merge-typesv3 branch from c18343b to 114dc29 Compare September 19, 2019 12:05

iSazonov marked this pull request as ready for review September 20, 2019 04:27

daxian-dbw self-assigned this Sep 20, 2019

JamesWTruher reviewed Sep 20, 2019

View reviewed changes

Address feedback

d429a35

daxian-dbw reviewed Oct 8, 2019

View reviewed changes

Address breaking change

5618f73

daxian-dbw reviewed Oct 9, 2019

View reviewed changes

daxian-dbw added Breaking-Change breaking change that may affect users Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Oct 9, 2019

iSazonov modified the milestones: 7.0.0-preview.5, 7.0.0-preview.6 Oct 24, 2019

daxian-dbw removed the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Oct 31, 2019

daxian-dbw closed this Oct 31, 2019

iSazonov deleted the merge-typesv3 branch November 1, 2019 05:46

daxian-dbw removed this from the 7.0.0-preview.6 milestone Jan 9, 2020

		@@ -1417,7 +1417,6 @@ private static InitialSessionState CreateRestrictedForRemoteServer()

		List<string> allowedTypes = new List<string>();

	Collection<string> types = new Collection<string>();
	Collection<string> types = new Collection<string>() { "types.ps1xml" };

	Collection<string> smaTypes = new Collection<string>();
	Collection<string> smaTypes = new Collection<string>() { "types.ps1xml" };

	if (membersCollection[member.name] != null && !isOverride)
	{
	AddError(errors, typeName, TypesXmlStrings.DuplicateMember, member.name);
	return;
	}

Conversation

iSazonov commented Sep 19, 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!

daxian-dbw commented Sep 20, 2019

Uh oh!

JamesWTruher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Sep 20, 2019

Uh oh!

iSazonov commented Oct 8, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Oct 10, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Oct 10, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Oct 31, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

iSazonov commented Sep 19, 2019 •

edited

Loading

daxian-dbw left a comment •

edited

Loading

iSazonov Oct 10, 2019 •

edited

Loading

daxian-dbw Oct 10, 2019 •

edited

Loading