Make Select-String faster by not doing extra work #7673
Conversation
powercode
requested review from
JamesWTruher,
PaulHigin and
dantraMSFT
as code owners
PaulHigin
left a comment
PaulHigin
left a comment
There was a problem hiding this comment.
LGTM
But shouldn't this also use the NoContextTracker?
|
@PaulHigin You are right! |
|
@powercode Please address the CodeFactor issues, specifically in new code. |
|
@adityapatwardhan Do you want me to fix all it the file or just the new ones? Happy to do both, but more to review if I fix all of them. |
|
@powercode Fixing the CodeFactor issues for the code you added / changed should be sufficient. Thanks! |
Don't check if fileinfos are directories (they never are). Don't track context when context size is 0.
powercode
force-pushed
the
SelectString
branch
from
79d5af8 to
439728d
Compare
| return s_inputStream; | ||
| return _path; | ||
| } | ||
| get => !_pathSet ? s_inputStream : _path; |
There was a problem hiding this comment.
More readable:
get => _pathSet ? _path : s_inputStream; There was a problem hiding this comment.
For sure - don't know what I was thinking :)
| { | ||
| throw PSTraceSource.NewArgumentNullException("value"); | ||
| } | ||
| _includeStrings = value ?? throw PSTraceSource.NewArgumentNullException("value"); |
There was a problem hiding this comment.
We could use nameof(value).
src/Microsoft.PowerShell.Commands.Utility/commands/utility/MatchString.cs
Show resolved
Hide resolved
src/Microsoft.PowerShell.Commands.Utility/commands/utility/MatchString.cs
Show resolved
Hide resolved
|
@powercode Please address comments and we'll merge. |
|
@iSazonov I'll have a look this evening. |
There was a problem hiding this comment.
Please revert the remove - we can not change a public contract.
There was a problem hiding this comment.
Should we change to comment to indicated that this isn't used anywhere?
There was a problem hiding this comment.
I think we shouldn't block the PR and please open new tracking Issue to get PowerShell Committee conclusion.
There was a problem hiding this comment.
I force-pushed the revert of the Filename property.
Also removing the non-referenced property FileName.
powercode
force-pushed
the
SelectString
branch
from
10d512e to
fe15a92
Compare
|
@powercode Thanks for great work! |
4 participants
PR Summary
By not tracking context in the common case where Context size is 0, and by not checking if the full name of a FileInfo is a Directory, about 30% of the execution time is cut when piping files into Select-String.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests