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
SemanticChecks: Avoid repeated type resolution of [ordered] #17328
SemanticChecks: Avoid repeated type resolution of [ordered] #17328
Conversation
Type resolution failure is costly, since it triggers repeated attempts "upwards" until the type resolver has enumerated every single assembly in the hosting app domain, and failures are non-cacheable In the special case of `[ordered]` as the lhs in a cast expression, we currently trigger such a cascading failure in multiple places where its entirely unnecessary This commit seeks to reduce the overhead incurred by the parser in these cases. Initial testing shows a ~45% reduction in wall-clock time for parsing 12.000 `[ordered]@{}` expressions in a single script file
If I understand correctly the intention was to do optimization for all non-resolvable type names:
|
I must have misunderstood @daxian-dbw |
@IISResetMe No, you are not 😄 Resolving the root issue will of course be the ultimate goal, but that will obviously require more time and yet still turn out to result in behavior changes. Also, it's possible that no one will pick up that work (or even the work to make The short term fix you discovered will largely mitigate the immediate problem right away with very minimal changes. I think it's reasonable to take it now. |
Gotcha, thanks @daxian-dbw! There's a couple other mitigation ideas that came up while discussing the issue earlier today:
I think we should be able to implement both short-term with minimal change in behavior (beyond improving performance of course). I'll play around with it and see if it works :) |
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 add a few tests about [ordered]
:
- define a
Ordered
type byAdd-Type
- check that
[ordered]@{a = 1; b= 2}
still returnsOrderedDictionary
. - check that
[ordered]<other-expr>
still fails with a parsing error. - check that
<somevealue> -is [ordered]
and<somevalue> -as [ordered]
continue to treat[ordered]
as a normal type name, which resolves to the type we defined.
src/System.Management.Automation/engine/parser/SemanticChecks.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/parser/SemanticChecks.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/parser/SemanticChecks.cs
Outdated
Show resolved
Hide resolved
Please use a separate PR for further optimization, especially if the target is to solve the perf issue regarding non-resolvable type in general. If the ideas are to further optimize how But but, just want to call out, don't feel obligated to address the root issues, and it's not urgent at all! |
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.
Looks good! Please add some tests based on the comment I left above. They are helpful to make sure this quick fix doesn't break the current behavior.
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
🎉 Handy links: |
PR Summary
Avoid unnecessary attempts to resolve
[ordered]
's underlying type (which isn't there) in post-parse checksPR Context
As highlighted in #17308, parsing the cast expression
[ordered]@{}
is exactly as slow as parsing[NonExistingType]@{}
!Most of this overhead is incurred during post-parse analysis conducted by the
SemanticChecks
visitor, when repeated attempts are made to resolve the underlying runtime type represented by[ordered]
. Since[ordered]
is a special attribute and not really a type literal, the type resolution fails, which in itself is a relatively costly (and non-cacheable) operation.This PR makes no functional changes to the flow of analysis, but reduces calls to
TypeName.GetReflectionType()
in situations where we've already assess the target type name is[ordered]
.This alone cuts the wallclock time for parsing
[ordered]@{}
compared to[nonexisting]@{}
by 45-50%: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.(which runs in a different PS Host).