Remove AllScope from most default aliases #5268

Merged
merged 2 commits into from Oct 31, 2017

Conversation

Projects
None yet
5 participants
Member

lzybkr commented Oct 29, 2017

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

@lzybkr lzybkr requested review from daxian-dbw, vors, BrucePay and SteveL-MSFT Oct 29, 2017

Collaborator

iSazonov commented Oct 30, 2017

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

Member

lzybkr commented Oct 30, 2017

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.

Collaborator

iSazonov commented Oct 30, 2017

If we remove ReadOnly here we can safely move to Alias() attribute.
I found the @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.

Owner

SteveL-MSFT commented Oct 30, 2017

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
@SteveL-MSFT

SteveL-MSFT Oct 30, 2017

Owner

Should we just remove the !CORECLR section?

@lzybkr

lzybkr Oct 30, 2017

Member

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

@SteveL-MSFT

SteveL-MSFT Oct 31, 2017

Owner

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
@SteveL-MSFT

SteveL-MSFT Oct 30, 2017

Owner

Should we remove the !CORECLR section?

@lzybkr

lzybkr Oct 30, 2017

Member

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

@SteveL-MSFT

SteveL-MSFT Oct 31, 2017

Owner

same as above

Member

lzybkr commented Oct 30, 2017

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

Member

lzybkr commented Oct 30, 2017

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 }
} }

@daxian-dbw daxian-dbw merged commit b108086 into PowerShell:master Oct 31, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lzybkr lzybkr deleted the lzybkr:less_allscope branch Oct 31, 2017

@lzybkr lzybkr referenced this pull request in lzybkr/PSReadLine Nov 3, 2017

Closed

PSReadline does not handle alias removal #453

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment