Skip to content

Conversation

@ThomasGoulet73
Copy link
Contributor

@ThomasGoulet73 ThomasGoulet73 commented Jun 17, 2022

Description

Removes SafeReflectionInvoker which was useful in .Net Framework because those methods were annotated with SecuritySafeCriticalAttribute for partial trust but since .Net Core does not support partial trust, this level of indirection is useless and only hurts performance because of the indirection and the methods are annotated to never be inlined by the JIT.

I removed IsInSystemXaml and IsSystemXamlNonPublic which are internal and unused but there are comments that it is by design that they still exist. Those comments dates back to the initial commit of System.Xaml in this repo so it probably comes from the .Net Framework version of WPF. I removed them in a separate commit so it's easy to revert if the WPF does not want to remove those methods.

Customer Impact

Should improve performance a bit (Though I wasn't able to properly measure it).

Regression

No.

Testing

Local build + CI + I tested a simple app.

Risk

Low. It is the same code but without indirection.

Microsoft Reviewers: Open in CodeFlow

@ThomasGoulet73 ThomasGoulet73 requested a review from a team as a code owner June 17, 2022 03:57
@ghost ghost assigned ThomasGoulet73 Jun 17, 2022
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Jun 17, 2022
@ghost ghost requested review from SamBent, dipeshmsft and singhashish-wpf June 17, 2022 03:57
@ghost ghost added the Community Contribution A label for all community Contributions label Jun 17, 2022
@dipeshmsft dipeshmsft self-assigned this Jul 19, 2022
@dipeshmsft
Copy link
Member

We have completed the test pass for this PR and the tests passed. However, we would like to keep IsInSystemXaml and IsSystemXamlNonPublic until we find out more details on that.

@ThomasGoulet73 can you please revert the change for removing IsInSystemXaml and IsSystemXamlNonPublic .

Once the changes are done, we will go ahead and merge the PR.

@ThomasGoulet73
Copy link
Contributor Author

@dipeshmsft Thank you, I'll try to do this later today.

@ThomasGoulet73
Copy link
Contributor Author

@dipeshmsft Done.

@pchaurasia14 pchaurasia14 merged commit 59b08c9 into dotnet:main Jan 19, 2023
@pchaurasia14
Copy link
Contributor

Thank you @ThomasGoulet73 !

@ThomasGoulet73 ThomasGoulet73 deleted the remove-SafeReflectionInvoker branch February 1, 2023 03:43
@ghost ghost locked as resolved and limited conversation to collaborators Mar 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Community Contribution A label for all community Contributions PR metadata: Label to tag PRs, to facilitate with triage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants