-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
Reduce string allocations when formatting file system objects. #8831
Reduce string allocations when formatting file system objects. #8831
Conversation
@powercode Please have a look at the test failure. |
@@ -136,6 +143,7 @@ public string ModuleName | |||
internal void SetModule(PSModuleInfo module) | |||
{ | |||
Module = module; | |||
_fullName = null; // clear the cached FullName |
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.
Please remove the comment. Better use _cacheFullModuleName
name.
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.
But it isn't a full module name. That name would be misleading. But sure, I can remove the comment.
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 it is not a full name what about _cachedModuleName?
I see there comment "Gets the name of the provider." So the name could be appropriate - _cachedProviderName.
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 property is FullName, so it makes sense to me that the backing property is _fullName.
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 tried to insert your comment to the name that it is a cached :-)
Awesome. Out of curiosity: Do you have time measurements of cases where this improvement is noticeable? |
@bergmeister It's mostly in allocations - it generates lots of extra strings, like 2 strings per item in the pipeline. I'm reasoning more in terms of steadily removing silly allocations, and fixing the worst offenders perf-wise, and eventually we will have a more snappy shell. The image below show the dotmemory view of strings allocated, grouped by callstack, when looking at the first 50000 items in the windows dir. It wasn't the biggest, but it was one of the bigger, and it was low hanging fruit. Does it make sense? |
Thanks @powercode. Yes, makes sense. I agree that continous improvement will make it better over time. :) |
@bergmeister It may seem like I'm just doing random changes, but I actually measure things now and then :) dotTrace and dotMemory are almost always running on my machine. |
@powercode I was not questioning it, I was just curious for my own education (because knowing this also means that one will know the scenarios where PSCore is stronger than Windows PowerShell meaning that I could recommend an upgrade to clients). |
@bergmeister Windows PowerShell is still more efficient than PowerShell Core |
@bergmeister I'm currently working on optimizations for the formatting system, the filesystem provider, etc that makes it out-perform windows powershell with a huge margin, often like 4x. And with a memory footprint reduction of the similar size. Import-Csv is problematic, since our property abstraction is heavier that I would like it to be. Especially in cases like that, where we actually have a table, we wouldn't need to store all the metadata for each object. I haven't given it so much thought - just passed by it last fall when doing related work. I have an idea that I like to try out for Import-CSV - could drastically reduce the memory footprint. |
PR Summary
Cache FullName property in the ProviderInfo class. Causes extra string allocations for every item in the formatting pipeline.
PR Context
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.[feature]
to your commit messages if the change is significant or affects feature tests