FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Remove AllScope from most default aliases by lzybkr · Pull Request #5268 · PowerShell/PowerShell · GitHub

/ PowerShell Public

Remove AllScope from most default aliases - #5268

Merged
Dongbo Wang (daxian-dbw) merged 2 commits into
PowerShell:masterfrom
lzybkr:less_allscope
Oct 31, 2017
ConversationCommits (2)ChecksFiles changed
Merged

Remove AllScope from most default aliases#5268
Dongbo Wang (daxian-dbw) merged 2 commits into
PowerShell:masterfrom
lzybkr:less_allscope

Conversation

Jason Shirk (lzybkr) commented Oct 29, 2017

Copy link
Copy Markdown
Contributor

To speed up scope creation, I removed AllScope from most default aliases.

This results in a 15-20% speedup for:

function foo {}
for ($i = 0; $i -lt 100kb; $i++) { & { foo } }

I left AllScope of a few frequently used aliases because it does make command lookup faster. If we introduce something like dynamic sites for command lookup, then we could probably remove the rest
of the AllScope aliases.

This is a low risk breaking change. One can ask for aliases at a particular scope:

Get-Alias -Scope 1 nsn

This could now fail if the scope number doesn't correspond to global scope.

Remove AllScope from most default aliases
3195a43 
To speed up scope creation, I removed AllScope from most default
aliases.

This results in a 15-20% speedup for:

    function foo {}
    for ($i = 0; $i -lt 100kb; $i++) { & { foo } }

I left AllScope of a few frequently used aliases because it does
make command lookup faster. If we introduce something like dynamic
sites for command lookup, then we could probably remove the rest
of the AllScope aliases.

This is a low risk breaking change. One can ask for aliases at
a particular scope:

    Get-Alias -Scope 1 nsn

This could now fail if the scope number doesn't correspond to global
scope.

Ilya (iSazonov) commented Oct 30, 2017 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Does it make sense to leave these aliases readonly? I think we talked about this earlier. We could use [Alias()] in cmdlet definitions.

Jason Shirk (lzybkr) commented Oct 30, 2017

Copy link
Copy Markdown
Contributor Author

That's 2 questions.

I don't feel strongly about using ReadOnly. I think it's unnecessary, but also don't see the harm.

As for using the Alias attribute - I thought there was an unrelated reason to not use the attribute - but I don't recall. If it was simply the lack of ability to specify scope options, the attribute could be enhanced to support that.

Ilya (iSazonov) commented Oct 30, 2017

Copy link
Copy Markdown
Collaborator

If we remove ReadOnly here we can safely move to Alias() attribute.
I found the Dongbo Wang (@daxian-dbw) related comment(s) and comment.

After PR 3595 I tried to enhance Alias attribute in the my branch. I couldn't implement AllScope and froze the work. But ReadOnly works. If we don't remove ReadOnly here after deletion of so much AllScope, maybe it makes sense to finish the work for ReadOnly here or I can push new PR later if you approve.

Steve Lee (SteveL-MSFT) commented Oct 30, 2017

Copy link
Copy Markdown
Member

If we remove AllScope from all the aliases, what perf impact would it be to the commonly used ones? Also, it seems like we should have an internal overload of SessionStateAliasEntry constructor rather than passing empty string everytime for the description.

Steve Lee (SteveL-MSFT) requested changes Oct 30, 2017
View reviewed changes
Comment thread src/System.Management.Automation/engine/InitialSessionState.cs
"Clear-Host", "", ScopedItemOptions.AllScope),
new SessionStateAliasEntry("clear", "Clear-Host"),
//#if !CORECLR is used to disable aliases for cmdlets which are not available on OneCore
#if !CORECLR

Steve Lee (SteveL-MSFT) Oct 30, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should we just remove the !CORECLR section?

Jason Shirk (lzybkr) Oct 30, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I considered that - but maybe a couple still make sense - ise e.g. - and would Out-Printer come back?

Steve Lee (SteveL-MSFT) Oct 31, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It's possible, so I guess it's best to leave it for now.

Comment thread src/System.Management.Automation/engine/InitialSessionState.cs
new SessionStateAliasEntry("sajb", "Start-Job"),
new SessionStateAliasEntry("spjb", "Stop-Job"),
new SessionStateAliasEntry("wjb", "Wait-Job"),
#if !CORECLR

Steve Lee (SteveL-MSFT) Oct 30, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should we remove the !CORECLR section?

Jason Shirk (lzybkr) Oct 30, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I thought most of these cmdlets might be ported to CORECLR, so left it.

Steve Lee (SteveL-MSFT) Oct 31, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

same as above

Jason Shirk (lzybkr) commented Oct 30, 2017

Copy link
Copy Markdown
Contributor Author

I considered adding the overload, but if we're fine with removing ReadOnly, we won't even need that.

Jason Shirk (lzybkr) commented Oct 30, 2017

Copy link
Copy Markdown
Contributor Author

As for perf impact on the common commands - it depends a lot, but command lookup isn't exactly cheap - there is a 25% difference in perf between in the two examples below - in the first, command lookup happens just once, in the second, it happens on every invocation.

function foo {}
measure-command { & {
    $c = Get-Command foo
    for ($i = 0; $i -lt 100kb; $i++) { & $c }
} }
measure-command { & {
    for ($i = 0; $i -lt 100kb; $i++) {foo }
} }

Dongbo Wang (daxian-dbw) merged commit b108086 into PowerShell:master Oct 31, 2017
James Truher (JamesWTruher) added the Breaking-Change breaking change that may affect users label Oct 31, 2017
Jason Shirk (lzybkr) deleted the less_allscope branch October 31, 2017 21:09
Jason Shirk (lzybkr) mentioned this pull request Nov 3, 2017
Closed
ExE Boss (ExE-Boss) mentioned this pull request Aug 24, 2018
Closed
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026
Remove AllScope from most default aliases (PowerShell#5268)
dffadc0 
* Remove AllScope from most default aliases

To speed up scope creation, I removed AllScope from most default
aliases.

This results in a 15-20% speedup for:

    function foo {}
    for ($i = 0; $i -lt 100kb; $i++) { & { foo } }

I left AllScope of a few frequently used aliases because it does
make command lookup faster. If we introduce something like dynamic
sites for command lookup, then we could probably remove the rest
of the AllScope aliases.

This is a low-risk breaking change. One can ask for aliases at
a particular scope:

    Get-Alias -Scope 1 nsn

This could now fail if the scope number doesn't correspond to global
scope.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Steve Lee (SteveL-MSFT) SteveL-MSFT approved these changes
Dongbo Wang (daxian-dbw) Awaiting requested review from daxian-dbw
sergei (vors) Awaiting requested review from vors
Bruce Payette (BrucePay) Awaiting requested review from BrucePay

Assignees

Dongbo Wang (daxian-dbw)

Labels

Breaking-Change breaking change that may affect users

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL