Set local pipeline thread stack size #5927

iSazonov · 2018-01-17T10:52:43Z

PR Summary

Related #4268.

Set local pipeline thread stack size to 10MB by default.
Read the PipelineMaxStackSizeMB configuration value from config.

Thread stack OS system default size is:

on Windows - 1MB
on MacOs - 0.5MB
on Linux - 8MB

We should use the same stack size for all platforms to get predictable behavior.
This value should match the Windows PowerShell value for backward compatibility (I expect that .Net Core and Windows PowerShell consume the stack as well as .Net Framework and Windows PowerShell).
Perhaps default 10MB may be too large if we use many local pipelines for paralleling. We can reconsider this later.

PR Checklist

Note: Please mark anything not applicable to this PR NA.

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
[NA] User facing Documentation needed
- [NA] Issue filed - Issue link:
Change is not breaking
[NA] Make sure you've added a new test if existing tests do not effectively test the code changed
- [NA] Add [feature] if the change is significant or affects feature tests
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

daxian-dbw · 2018-01-17T19:46:33Z

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

        public ProtectedEventLogging ProtectedEventLogging { get; set; }
        public Transcription Transcription { get; set; }
        public UpdatableHelp UpdatableHelp { get; set; }
+        public UpdatableHelp PipelineMaxStackSizeMB { get; set; }


The PipelineMaxStackSizeMB setting is not a policy in windows powershell, and thus it shouldn't be added to PowerShellPolicies. It should be a setting like psmodulepath, or executionpolicy or ConsolePrompting.

THe PipelineMaxStackSizeMB was stored at SOFTWARE\Microsoft\PowerShell\1\ShellIds in windows powershell, not at SOFTWARE\Policies\...

I believe previously we agreed that PowerShell Core is a separate product and it should have its policies and not use policies and configs of Windows PowerShell.
Why don't we have consistency between the settings in the file and the registry if we the consistency by default on Unix? I wouldn't be surprised if MSFT decides to port Registry API to make it easier to port applications. (I would take this into account).
Also if the settings are in different places, it is difficult to find it and makes it very difficult to configure.
Another thought is that we must put (later?) config options and GPO policies in different registry keys.

I think we should develop this my comment about priority - maybe open new tracking Issue?
Also we could open new tracking Issue (?) to enumerate config options and to solve where put different options - in policy key or/and in config key.

Today I suggest:

Have PipelineMaxStackSizeMB (and most other options) in registry and file.

Put PipelineMaxStackSizeMB in "Software\Policies\Microsoft\PowerShellCore". Later we can move/duplicate it in "Software\Microsoft\PowerShellCore" - today we haven't code to process the registry branches in priority.

I didn't mean PipelineMaxStackSizeMB should be put in Registry on Windows. I think this setting should be in the config file only on both Windows and Unix.

As far as I understand, only the GPO related settings will live in Registry and powershell.config.json on Windows for PowerShell Core (registry setting takes precedence). Non-GPO settings that used to be stored in Registry in Windows PowerShell now should live in the config file only in PowerShell Core, such as executionpolicy, ConsolePrompting and many others. You can take a look at PSConfiguration.cs to find more of them.

The type PowerShellPolicies only represents the GPO settings. PipelineMaxStackSizeMB is not a policy setting but a normal powershell setting, so it shouldn't be put in PowerShellPolicies.

@daxian-dbw I wanted to create an Issue to discuss but the text was too big and I made a document #5947

daxian-dbw · 2018-01-17T19:52:58Z

src/System.Management.Automation/engine/hostifaces/LocalPipeline.cs

-                return i * 1000000;
-            }
-        }
+        internal static int StackSize => GetPipelineMaxStackSizeOption(10);


If we don't cache the stack size, then this static property seems not necessary as it only adds one more redirection to call GetPipelineMaxStackSizeOption.

daxian-dbw · 2018-01-22T18:17:38Z

@iSazonov PipelineMaxStackSizeMB is a regular powershell setting in Windows PowerShell. You are treating it as a policy setting instead of a regular setting in powershell core. I disagree with that. I think it should be a regular setting just like executionpolicy or ConsolePrompting.

iSazonov · 2018-01-23T15:16:41Z

@daxian-dbw What is "policy" in the context?

Software\Policies\Microsoft\PowerShellCore - I believe it is "policy"?

Software\Microsoft\PowerShell\1\ShellIds or
Software\PowerShellCore - I believe it is "regular settings"? Do you say that we shouldn't use Registry for this settings and keep its only in a config file?

daxian-dbw · 2018-01-23T18:05:19Z

What is "policy" in the context?

It's short for "Group Policy Settings". Those are the settings that are originally under "Software\Policies". Each of the settings has a corresponding GPO in "Local Group Policy Editor" in Control Panel. The new PowerShell Core ones will also be available in the editor. We will publish a .admx file for them.

The settings that are not originally under "Software\Policies" are regular powershell settings. As far as I understand, we don't use Registry for those settings but only refer to the config file.

iSazonov · 2018-01-24T03:43:53Z

Thanks for clarify!

As far as I understand, we don't use Registry for those settings but only refer to the config file.

Is this PowerShell MSFT team solution? And why?

Registry is common place for Windows applications to store settings:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms724871(v=vs.85).aspx
https://msdn.microsoft.com/en-us/library/ms973902.aspx

This is critical for enterprises. By using GPO/GPP, they can centrally manage (distribute polices and settings too) applications on workstations and server farms.
For example, if they receive feedback from employees that their scripts fail because of a stack overflow, the enterprise administrator can easily propagate the parameter value as a default that does not overlap local settings at the application or user level.

SteveL-MSFT · 2018-01-24T04:06:35Z

For PSCore6, we want to have configuration not in the registry. This makes it easier to deploy via zip, for example, as well as support side-by-side more simply. I do agree that for an enterprise, we should support GPO. In which case, there is no issue with having both configuration and policy.

iSazonov · 2018-01-24T04:46:06Z

This makes it easier to deploy via zip, for example, as well as support side-by-side more simply.

The ability to have settings in the registry does not conflict with zip and side-by-side. (We could even enhance our API to support applications by type and name)

@SteveL-MSFT What your conclusion about #5947? - I'd move it in RFC repo but it's frozen. I would have moved RFC repo here with label like RFC-* - so we get more feedback.

iSazonov · 2018-01-25T11:46:08Z

I created PowerShell/PowerShell-RFC#111.

Should we wait the RFC?

SteveL-MSFT · 2018-02-21T23:23:42Z

@PowerShell/powershell-committee reviewed this and don't believe that the stack size should even be a configurable setting until there is a clear need to make it settable. Recommendation is to not even have it settable despite what Windows PowerShell did.

iSazonov · 2018-02-22T05:14:14Z

@SteveL-MSFT Stack size default is different on different platforms - should we set the same default internally to get predictable behavior on all platforms? I think it make sense - see the original Issue.

SteveL-MSFT · 2018-02-22T23:14:44Z

@iSazonov yes, @PowerShell/powershell-committee agrees to changing the default on all systems. We thought the only point that needed discussion from the committee was configuration.

daxian-dbw · 2018-02-23T18:15:35Z

A new PR was submitted based on the committee's review: #6224
I will close this PR.

iSazonov assigned daxian-dbw Jan 17, 2018

iSazonov requested a review from daxian-dbw January 17, 2018 10:52

iSazonov requested review from BrucePay and lzybkr as code owners January 17, 2018 10:52

iSazonov force-pushed the set-localpipeline-stack-size branch from 2404c5d to 5096276 Compare January 17, 2018 13:10

Set local pipeline thread stack size

35a3378

iSazonov force-pushed the set-localpipeline-stack-size branch from 5096276 to 35a3378 Compare January 17, 2018 14:27

daxian-dbw requested changes Jan 17, 2018

View reviewed changes

Remove StackSize property, change Registry key, fix typo

d229ae8

SteveL-MSFT requested a review from TravisEz13 January 24, 2018 04:03

Merge branch 'master' into set-localpipeline-stack-size

c69c7ca

Fix property assign

79b91ae

SteveL-MSFT added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Feb 21, 2018

SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Feb 21, 2018

iSazonov mentioned this pull request Feb 23, 2018

Set pipeline thread stack size #6224

Merged

9 tasks

daxian-dbw closed this Feb 23, 2018

Set local pipeline thread stack size #5927

Set local pipeline thread stack size #5927

Uh oh!

Conversation

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

Uh oh!

PR Summary

PR Checklist

Uh oh!

daxian-dbw Jan 17, 2018

Choose a reason for hiding this comment

Uh oh!

iSazonov Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

iSazonov Jan 19, 2018 • 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 Jan 17, 2018

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Jan 23, 2018

Uh oh!

daxian-dbw commented Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Jan 24, 2018

Uh oh!

SteveL-MSFT commented Jan 24, 2018

Uh oh!

iSazonov commented Jan 24, 2018

Uh oh!

iSazonov commented Jan 25, 2018

Uh oh!

SteveL-MSFT commented Feb 21, 2018

Uh oh!

iSazonov commented Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveL-MSFT commented Feb 22, 2018

Uh oh!

daxian-dbw commented Feb 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

iSazonov commented Jan 17, 2018 •

edited

Loading

iSazonov Jan 19, 2018 •

edited

Loading

daxian-dbw commented Jan 22, 2018 •

edited

Loading

daxian-dbw commented Jan 23, 2018 •

edited

Loading

iSazonov commented Feb 22, 2018 •

edited

Loading