-
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
Improve performance by having better primitives on PSObject #8785
Conversation
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.
It's going to take a bit of time to review all the changes. @powercode did you test your changes remotely with WMI and CIM instances?
src/System.Management.Automation/FormatAndOutput/common/FormatViewManager.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/FormatAndOutput/common/Utilities/MshObjectUtil.cs
Outdated
Show resolved
Hide resolved
@SteveL-MSFT To be honest, I've done an embarrassingly small amount of testing. Except for running |
becd0d0
to
1f40247
Compare
Note to reviewers: @BrucePay @daxian-dbw I tried another route in this second PR, by adding another delegate for member lookup. It is less intrusive in the previous functionality, so the risk should be lower. I have refactored PSObject to use flags instead of Booleans. I also removed some unused fields, and have plans for using the available padding for speeding up the handling of the remoting properties. I added a field for the stream to use by the formatting system - this was previously magically named PSNoteProperties - and the lookup of these was expensive during formatting. As you can see, I've implemented a special case of a the Linq operator I'm a bit weak on the serialization, so that is an area that could need some scrutiny to see if I've messed anything up. |
It can break remoting/serialization and backward compatibility. |
I think that is just an implementation detail. We may need to handle that in the deserializer - but good to keep in mind when doing it. |
I have a sample implementation here - https://github.com/powercode/PowerShell/tree/PSObjectFlags Edit: |
Looks great at first short review! (We'll need measurement of CPU/memory benefits. And main complicity is that we have to manually test remoting and serialization.) |
Cleaning up PSObject, making the bools into flag fields, leaving room for using the remaing padding for additional flags.
@iSazonov @SteveL-MSFT Any chance we can turn of the hungarian notation warnings from Code Factor? It has nearly 100% false positives. I don't intend to fix the remaining code factor issues. |
@powercode I believe we're eventually moving off CodeFactor and over to Codacy. Agree the false positives on hungarian are annoying. |
Codacy reports tons of false-positives. We need to merge Dongbo's PR that resolve most CodeFactor issues. |
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.
A lot of this is way out of my depth, but I hope some of this is helpful! 😊
Nothing serious, mostly nitpicks.
src/Microsoft.PowerShell.Commands.Utility/commands/utility/CustomSerialization.cs
Show resolved
Hide resolved
src/System.Management.Automation/FormatAndOutput/common/FormatViewManager.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/FormatAndOutput/common/FormattingObjects.cs
Show resolved
Hide resolved
src/System.Management.Automation/cimSupport/other/ciminstancetypeadapter.cs
Outdated
Show resolved
Hide resolved
@vexx32 I don't look but if you comments is for copy-pasted (not new) code we shouldn't fix it in the PR - it is complicate code review. |
@powercode I submitted another PR to your branch for some fixes and refactoring: powercode#2. Please take a look when you have time. |
Some more fixes and refactor
Looks good! |
It seems we need rename the PR from
to
|
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.
LGTM. Now we need to add tests.
We can add xUnit tests to exercise PSObject.GetFirstPropertyOrDefault
.
Add [Feature]
tag to the last commit to kick off the feature tests on Linux and macOS.
67981d5
to
796f1aa
Compare
796f1aa
to
fa824a4
Compare
fa824a4
to
ed74852
Compare
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.
LGTM
Can we move style changes to #9149 to follow our best practice? |
@iSazonov I have reviewed all changes in this PR and all look good. Separating the refactoring changes will not only cost extra efforts to resolve conflicts but also require another pass of review to make sure no mistake made during those changes. I don't think it worth the effort. |
All issues reported by Codacy are false alarms. |
We agreed to write this in our guide. This allows us to maintain a clear history of functional changes without style ones. Of course it takes effort. |
I agree that will get us a clear view of the functional change history. But I think it's more appropriate to bring this up at very early stage of the PR life cycle. |
At early stage we haven't all style changes. The main benefit is that the commits are clean.
In previous powercode's PR you did this :-) |
@powercode Thanks for the great improvement! |
@powercode This is outstanding work here! |
By not doing excessive amounts of extra work, formatting can be sped up quite significantly.
The main change comes from adding new, more efficient, primitive to query an object for the existence of an instance member.
The formatting system has been checking for if an object has properties other than some decorated properties added by PS remoting, and it doesn't this by retrieving all properties which results in heavy allocations and wasted cycles.
By adding
GetFirstOrDefault
toPSObject
and similar primitives to the underlying Adapters, we are able to return early, without having to get all properties back.Test script:
PR Summary
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