[release/7.0] Remove unnecessary runtime lookup for constrained callvirt #74523
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
In my change adding support for default static virtual interface method implementations I made a subtle bug that caused behavioral change for some pre-existing constrained virtual calls that newly started to require runtime lookup. This is unnecessary and perf-negative, I have modified the code so that my change kicks in only for static virtual methods. Thanks Tomas
ghost
added
the
|
Fixes: #73681 |
markples
approved these changes
Aug 25, 2022
jakobbotsch
approved these changes
Aug 25, 2022
|
@jeffschwMSFT can we get an approval? @trylek can you confirm the CI failures are unrelated? BTW the dotnet-linker-tests leg seems to be stuck. |
|
@carlossanlop - I don't see how the dotnet-linker-tests failure could be related to my change, I doubt the CoreCLR runtime was even running when the job got stuck, I retried the one failed job, I don't see any other failures in the testing. |
mangod9
approved these changes
Aug 25, 2022
|
Approved and signed off. CI is green. Ready to merge. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Backport of #73823 to release/7.0
/cc @trylek
(This should really go into 7.0-RC1; based on @carlossanlop's e-mail I've made the backport against release/7.0 as the 7.0-rc1 branch is currently closed, please let me know how to proceed here.)
Customer Impact
According to #73681, this change fixes a user-visible C# behavior change and perf regression.
Testing
Local and lab testing in the runtime repo - innerloop, outerloop; Jakob manually verified on a code sample that the change fixes the codegen regression described in the issue.
Risk
Low - there's no real algorithmic change, the fix just modifies a conditional statement so that the logic I added for static virtual methods only truly applies to static virtual methods, previously it was "leaking" to non-static virtuals too, that was the cause of the codegen regression.
IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.