GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Reduce string allocations when formatting file system objects. by powercode · Pull Request #8831 · PowerShell/PowerShell

Conversation

@powercode

@powercode Staffan Gustafsson (powercode) commented Feb 5, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

PR Summary

Cache FullName property in the ProviderInfo class. Causes extra string allocations for every item in the formatting pipeline.

PR Context

PR Checklist

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Change is not breaking
  • Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
  • This PR is ready to merge and is not Work in Progress.
    • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
  • User-facing changes
    • Not Applicable
    • OR
    • Documentation needed
      • Issue filed:
  • Testing - New and feature
    • N/A or can only be tested interactively
    • OR
    • Make sure you've added a new test if existing tests do not effectively test the code changed
      • Add [feature] to your commit messages if the change is significant or affects feature tests

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vexx32, iSazonov, and bergmeister reacted with hooray emoji
All reactions
  • 🎉 3 reactions
@powercode
Reduce string allocations when formatting file system objects.
1509da0
@powercode
Staffan Gustafsson (powercode) requested review from Bruce Payette (BrucePay) and Dongbo Wang (daxian-dbw) as code owners February 5, 2019 16:41
adityapatwardhan
Aditya Patwardhan (adityapatwardhan) approved these changes Feb 5, 2019
View reviewed changes
@adityapatwardhan

Aditya Patwardhan (adityapatwardhan) commented Feb 5, 2019

Copy link
Copy Markdown
Member

Staffan Gustafsson (@powercode) Please have a look at the test failure.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

iSazonov
Ilya (iSazonov) approved these changes Feb 5, 2019
View reviewed changes
@powercode
Clear cache FullName when the module changes.
4874053
iSazonov
Ilya (iSazonov) reviewed Feb 6, 2019
View reviewed changes
Comment thread src/System.Management.Automation/engine/DataStoreAdapterProvider.cs Outdated
internal void SetModule(PSModuleInfo module)
{
Module = module;
_fullName = null; // clear the cached FullName

@iSazonov Ilya (iSazonov) Feb 6, 2019

Copy link
Copy Markdown
Collaborator

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@powercode Staffan Gustafsson (powercode) Feb 6, 2019

Copy link
Copy Markdown
Collaborator Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@iSazonov Ilya (iSazonov) Feb 6, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@powercode Staffan Gustafsson (powercode) Feb 6, 2019

Copy link
Copy Markdown
Collaborator Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@iSazonov Ilya (iSazonov) Feb 7, 2019

Copy link
Copy Markdown
Collaborator

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 :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@powercode
Removing comment
9e68f73
@bergmeister

Christoph Bergmeister (bergmeister) commented Feb 6, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Awesome. Out of curiosity: Do you have time measurements of cases where this improvement is noticeable?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@powercode

Staffan Gustafsson (powercode) commented Feb 6, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

Christoph Bergmeister (@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?

image

bergmeister reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bergmeister

Christoph Bergmeister (bergmeister) commented Feb 6, 2019

Copy link
Copy Markdown
Contributor

Thanks Staffan Gustafsson (@powercode). Yes, makes sense. I agree that continous improvement will make it better over time. :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@powercode

Staffan Gustafsson (powercode) commented Feb 6, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

Christoph Bergmeister (@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.

vexx32 and bergmeister reacted with laugh emoji
All reactions
  • 😄 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iSazonov Ilya (iSazonov) self-assigned this Feb 7, 2019
@iSazonov Ilya (iSazonov) added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label Feb 7, 2019
@iSazonov
Ilya (iSazonov) merged commit ec88045 into PowerShell:master Feb 7, 2019
@bergmeister

Christoph Bergmeister (bergmeister) commented Feb 7, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Staffan Gustafsson (@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).
Would you mind having a look at issue #7603 with Import-Csv please which is causing actual OutOfMemory problems in some workflows where CSV files are a couple of GB large.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iSazonov

Ilya (iSazonov) commented Feb 8, 2019

Copy link
Copy Markdown
Collaborator

Christoph Bergmeister (@bergmeister) Windows PowerShell is still more efficient than PowerShell Core
in many scenarios. Main reason that .Net Framework engine works differently than .Net Core one. Optimizing individual cmdlets would be fine, but maybe we need something more general.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@powercode

Staffan Gustafsson (powercode) commented Feb 10, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

Christoph Bergmeister (@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.
That is to generate a dynamic assembly, with a class containing the fields of the CSV.

bergmeister reacted with thumbs up emoji bergmeister reacted with hooray emoji bergmeister reacted with heart emoji
All reactions
  • 👍 1 reaction
  • 🎉 1 reaction
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iSazonov

Ilya (iSazonov) commented Feb 10, 2019

Copy link
Copy Markdown
Collaborator

I was thinking about compiling to classes too and found some problems in the approach. One problem could be solved by #8852 (not trivial). After finding IDataView #8855 I suppose this is preferred way we should start research.

powercode and daxian-dbw reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@SeeminglyScience Patrick Meinecke (SeeminglyScience) mentioned this pull request Jan 31, 2020
Get-ChieldItem fails with "Cannot find a provider ..." on version 6.2.1 #9840
Closed
This was referenced Feb 4, 2020
Avoid caching ProviderInfo.FullName at init time #11761
Closed
Set correct PSProvider full name #11813
Merged
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@powercode @iSazonov
Reduce string allocations when formatting file system objects. (Power…
88484f2
…Shell#8831)

* Reduce string allocations when formatting file system objects by caching FullName.
* Clear cache FullName when the module changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov Ilya (iSazonov) iSazonov approved these changes
@adityapatwardhan Aditya Patwardhan (adityapatwardhan) adityapatwardhan approved these changes
@BrucePay Bruce Payette (BrucePay) Awaiting requested review from BrucePay
@daxian-dbw Dongbo Wang (daxian-dbw) Awaiting requested review from daxian-dbw

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@iSazonov Ilya (iSazonov)

Labels

CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@powercode @adityapatwardhan @bergmeister @iSazonov
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.