GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Set local pipeline thread stack size by iSazonov · Pull Request #5927 · PowerShell/PowerShell

Conversation

@iSazonov

@iSazonov Ilya (iSazonov) commented Jan 17, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

PR Summary

Related #4268.

  1. Set local pipeline thread stack size to 10MB by default.
  2. 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@iSazonov Ilya (iSazonov) assigned Dongbo Wang (daxian-dbw) Jan 17, 2018
@iSazonov
Ilya (iSazonov) requested a review from Dongbo Wang (daxian-dbw) January 17, 2018 10:52
@iSazonov
Ilya (iSazonov) requested review from Bruce Payette (BrucePay) and Jason Shirk (lzybkr) as code owners January 17, 2018 10:52
@iSazonov
Ilya (iSazonov) force-pushed the set-localpipeline-stack-size branch from 2404c5d to 5096276 Compare January 17, 2018 13:10
@iSazonov
Set local pipeline thread stack size
35a3378
@iSazonov
Ilya (iSazonov) force-pushed the set-localpipeline-stack-size branch from 5096276 to 35a3378 Compare January 17, 2018 14:27
daxian-dbw
Dongbo Wang (daxian-dbw) requested changes Jan 17, 2018
View reviewed changes
Comment thread src/System.Management.Automation/engine/PSConfiguration.cs Outdated
public ProtectedEventLogging ProtectedEventLogging { get; set; }
public Transcription Transcription { get; set; }
public UpdatableHelp UpdatableHelp { get; set; }
public UpdatableHelp PipelineMaxStackSizeMB { get; set; }

@daxian-dbw Dongbo Wang (daxian-dbw) Jan 17, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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\...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@iSazonov Ilya (iSazonov) Jan 18, 2018

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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:

  1. Have PipelineMaxStackSizeMB (and most other options) in registry and file.
  2. 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Jan 18, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@iSazonov Ilya (iSazonov) Jan 19, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

daxian-dbw reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread src/System.Management.Automation/engine/hostifaces/LocalPipeline.cs Outdated
return i * 1000000;
}
}
internal static int StackSize => GetPipelineMaxStackSizeOption(10);

@daxian-dbw Dongbo Wang (daxian-dbw) Jan 17, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@iSazonov
Remove StackSize property, change Registry key, fix typo
d229ae8
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Jan 22, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Ilya (@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iSazonov

Ilya (iSazonov) commented Jan 23, 2018

Copy link
Copy Markdown
Collaborator Author

Dongbo Wang (@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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@daxian-dbw

Dongbo Wang (daxian-dbw) commented Jan 23, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iSazonov

Ilya (iSazonov) commented Jan 24, 2018

Copy link
Copy Markdown
Collaborator Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@SteveL-MSFT
Steve Lee (SteveL-MSFT) requested a review from Travis Plunk (TravisEz13) January 24, 2018 04:03
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Jan 24, 2018

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iSazonov

Ilya (iSazonov) commented Jan 24, 2018

Copy link
Copy Markdown
Collaborator Author

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)

Steve Lee (@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iSazonov
Merge branch 'master' into set-localpipeline-stack-size
c69c7ca
@iSazonov

Ilya (iSazonov) commented Jan 25, 2018

Copy link
Copy Markdown
Collaborator Author

I created PowerShell/PowerShell-RFC#111.

Should we wait the RFC?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iSazonov
Fix property assign
79b91ae
@SteveL-MSFT Steve Lee (SteveL-MSFT) added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Feb 21, 2018
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Feb 21, 2018

Copy link
Copy Markdown
Member

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@SteveL-MSFT Steve Lee (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

Ilya (iSazonov) commented Feb 22, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

Steve Lee (@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Feb 22, 2018

Copy link
Copy Markdown
Member

Ilya (@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iSazonov Ilya (iSazonov) mentioned this pull request Feb 23, 2018
Set pipeline thread stack size #6224
Merged
5 of 9 tasks
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Feb 23, 2018

Copy link
Copy Markdown
Member

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

iSazonov reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@daxian-dbw Dongbo Wang (daxian-dbw) closed this Feb 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw Dongbo Wang (daxian-dbw) daxian-dbw requested changes
@BrucePay Bruce Payette (BrucePay) Awaiting requested review from BrucePay
@lzybkr Jason Shirk (lzybkr) Awaiting requested review from lzybkr
@TravisEz13 Travis Plunk (TravisEz13) Awaiting requested review from TravisEz13

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@daxian-dbw Dongbo Wang (daxian-dbw)

Labels

Committee-Reviewed PS-Committee has reviewed this and made a decision

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@iSazonov @daxian-dbw @SteveL-MSFT
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.