Add tools for PowerShell perf analysis #7595
Conversation
lzybkr
requested review from
TravisEz13,
adityapatwardhan,
anmenaga,
dantraMSFT and
daxian-dbw
as code owners
|
I'm so happy!! Maybe add some info regarding using builds with symbols (cross gened) to get usable stacks? |
tools/performance/GC.wprp
Outdated
There was a problem hiding this comment.
That's... 1 GB of buffers? Seems a bit excessive.
There was a problem hiding this comment.
Indeed. I forget where I copied that profile from and it looks like it's not actually being used so I'll just delete the file.
tools/performance/GC.wprp
Outdated
There was a problem hiding this comment.
Without a System collector this profile won't be usable by itself (it won't have process, thread, or loader events) - the user will always have to start at least one other session alongside this in order to make sense of the data in WPA.
There was a problem hiding this comment.
Yep, I'm removing this profile.
tools/performance/README.md
Outdated
There was a problem hiding this comment.
These wprp files aren't WPA regions files, they are WPR profiles.
There was a problem hiding this comment.
Yep, I clearly wasn't thinking when I wrote that - I'm removing the files.
There was a problem hiding this comment.
Your other profiles capture a small subset of the events this one does, but use a much larger set of buffers.
There was a problem hiding this comment.
This is the profile we care about. I don't recall seeing dropped events, so maybe this is fine?
There was a problem hiding this comment.
Despite what the outdated MSDN documentation says, the Name attribute has been optional since Windows 8.1, and should be considered deprecated.
There's probably no value targeting the WPR that ships with the Windows 8 ADK as opposed to the Windows 8.1 ADK. Newer versions of the ADK can be installed on older OS releases.
There was a problem hiding this comment.
Thanks, I appreciate the review. I agree, folks profiling PowerShell probably aren't running Win8 so I'll just remove the Name attribute.
There was a problem hiding this comment.
Should this be uncommented?
There was a problem hiding this comment.
No, it's expensive and not normally needed, but good to know about so I left it. I'll add a comment to explain.
There was a problem hiding this comment.
Can we make this a parameter? So we can launch a different pwsh.exe version than the one we are using.
tools/performance/GC.xml
Outdated
There was a problem hiding this comment.
Could you add a comment why sometimes the module name has .ni while sometimes it doesn't
There was a problem hiding this comment.
Uh, I'm not sure, and maybe not relevant to PowerShell Core. This file isn't really that refined at all, so the hope is folks will contribute and make it more useful,
There was a problem hiding this comment.
I remember now, comment added.
|
Sorry about amending instead of adding, mostly just renaming and reformatting, but all feedback should be addressed now. |
|
Can't hold breath much longer... |
|
FYI. 18 spelling errors in tools/performance/README.md are causing Travis CI build failure. |
|
I see 0 spelling errors and have no plans to make the tool happy, so a maintainer can fix or else close the PR. |
|
The tool fails on stuff like |
|
@powercode - yes, I reviewed what it complained about, it also complained about But my point - we have humans reviewing and didn't complain. The tool should not block PRs. |
|
Updated the spellings file. Once CI is complete, I will merge. |
|
@lzybkr Thanks for your contribution!! |
6 participants
PR Summary
These scripts and additional files are useful in analyzing PowerShell performance on Windows with either perfview or wpr/wpa.
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