-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Remove AllScope from most default aliases #5268
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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.
Does it make sense to leave these aliases readonly? I think we talked about this earlier. We could use |
That's 2 questions. I don't feel strongly about using As for using the |
If we remove ReadOnly here we can safely move to 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. |
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. |
// Native commands we keep because the functions act correctly on Linux | ||
new SessionStateAliasEntry("clear", | ||
"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 |
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.
Should we just remove the !CORECLR section?
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 considered that - but maybe a couple still make sense - ise e.g. - and would Out-Printer
come back?
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.
It's possible, so I guess it's best to leave it for now.
new SessionStateAliasEntry("rjb", "Remove-Job"), | ||
new SessionStateAliasEntry("sajb", "Start-Job"), | ||
new SessionStateAliasEntry("spjb", "Stop-Job"), | ||
new SessionStateAliasEntry("wjb", "Wait-Job"), | ||
#if !CORECLR |
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.
Should we remove the !CORECLR section?
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 thought most of these cmdlets might be ported to CORECLR, so left it.
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.
same as above
I considered adding the overload, but if we're fine with removing |
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 }
} } |
To speed up scope creation, I removed AllScope from most default aliases.
This results in a 15-20% speedup for:
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:
This could now fail if the scope number doesn't correspond to global scope.