Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[iOS] Fix trimming warnings in HttpClientHandler.AnyMobile #91520

Merged
merged 3 commits into from
Sep 8, 2023
Merged

[iOS] Fix trimming warnings in HttpClientHandler.AnyMobile #91520

merged 3 commits into from
Sep 8, 2023

Conversation

simonrozsival
Copy link
Member

@simonrozsival simonrozsival commented Sep 3, 2023 •
edited
Loading

While testing NativeAOT on iOS, I started noticed this warning from ILC:

ILC : Trim analysis warning IL2075: System.Net.Http.HttpClientHandler.InvokeNativeHandlerMethod(String,Object[]): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods' in call to 'System.Type.GetMethod(String)'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.

This PR fixes the warning.

@ghost ghost assigned simonrozsival Sep 3, 2023
@ghost
Copy link

ghost commented Sep 3, 2023

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

While testing NativeAOT on iOS, I started noticed this warning from ILC:

ILC : Trim analysis warning IL2075: System.Net.Http.HttpClientHandler.InvokeNativeHandlerMethod(String,Object[]): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods' in call to 'System.Type.GetMethod(String)'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.

All the methods which call InvokeNativeHandlerMethod are annotated with DynamicDependency attributes and so the warning we get from ILC can be suppressed.

Author: simonrozsival
Assignees: -
Labels:

area-System.Net.Http, os-ios
Milestone: -

@simonrozsival

This comment was marked as outdated.

Sign in to view
@azure-pipelines

This comment was marked as outdated.

Sign in to view
@simonrozsival
Copy link
Member Author

/azp run runtime-ioslike

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Sep 3, 2023
Open
Closed
@simonrozsival simonrozsival mentioned this pull request Sep 4, 2023
Closed
@simonrozsival
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky MichalStrehovsky added the linkable-framework Issues associated with delivering a linker friendly framework label Sep 7, 2023
@ghost
Copy link

ghost commented Sep 7, 2023

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr, @marek-safar
See info in area-owners.md if you want to be subscribed.

Issue Details

While testing NativeAOT on iOS, I started noticed this warning from ILC:

ILC : Trim analysis warning IL2075: System.Net.Http.HttpClientHandler.InvokeNativeHandlerMethod(String,Object[]): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods' in call to 'System.Type.GetMethod(String)'. The return value of method 'System.Object.GetType()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.

All the methods which call InvokeNativeHandlerMethod are annotated with DynamicDependency attributes and so the warning we get from ILC can be suppressed.

Author: simonrozsival
Assignees: simonrozsival
Labels:

area-System.Net.Http, linkable-framework, os-ios
Milestone: -

@MichalStrehovsky
Copy link
Member

We typically write targeted trimming tests if there is a suppression. Suppressions are not great because someone could add a new callsite and forget to add DynamicDependency (or DynamicDependency the wrong thing).

I think this can be rewritten to be correct by construction. All the text strings are there, they're just too far apart for analysis to figure them out.

Basically, I'd do this:

  • Delete all the DynamicDependency stuff.
  • Replace _nativeHandler!.GetType()!.GetMethod(name); in InvokeNativeHandlerMethod with Type.GetType(NativeHandlerType + ", " + AssemblyName)!.GetMethod(name);. The C# compiler will concatenate these strings because they're all consts.
  • This is already enough to make everything work. Trimming will keep all public methods on the type and there will be no warning. This is because the name of the type is visible within the InvokeNativeHandlerMethod method.
  • If there is a size concern (we don't want to keep all the public methods on this type), we could hoist the Type.GetType(NativeHandlerType + ", " + AssemblyName)!.GetMethod(name); part into a delegate (add an Action<MethodInfo> delegate parameter to InvokeNativeHandlerMethod and provide the right implementation at each callsite as a lambda). This should result in comparable size to what was achieved with DynamicDependency (the attributes are not cheap, size-wise - it's a lot of characters and none of them can be shared even though they're identical because custom attributes are encoded that way).

This was referenced Sep 7, 2023
Closed
Open
Open
Closed
Closed
Closed
@simonrozsival
Copy link
Member Author

@MichalStrehovsky thanks for the suggestion!

@simonrozsival simonrozsival marked this pull request as draft September 7, 2023 13:46
@simonrozsival
Copy link
Member Author

/azp run runtime-ioslike

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

MichalStrehovsky
MichalStrehovsky reviewed Sep 8, 2023
View reviewed changes
Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! (Reviewing from the trimmability perspective only.)

Cc @AaronRobinsonMSFT - I think this is a nice candidate for #90081. We'll be able to simplify all of this and get rid of the reflection.

@simonrozsival simonrozsival changed the title [iOS] Unconditionally suppress trimming warnings in HttpClientHandler [iOS] Fix trimming warnings in HttpClientHandler.AnyMobile Sep 8, 2023
@simonrozsival
Copy link
Member Author

/azp run runtime-android

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@simonrozsival simonrozsival mentioned this pull request Sep 8, 2023
Closed
@simonrozsival simonrozsival marked this pull request as ready for review September 8, 2023 08:46
@simonrozsival simonrozsival merged commit 7f06d1c into dotnet:main Sep 8, 2023
106 of 118 checks passed
@simonrozsival simonrozsival deleted the http-client-handler-anymobile-suppress-ilc-warning branch September 8, 2023 19:07
@simonrozsival
Copy link
Member Author

/backport to release/8.0-rc1

@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2023

Started backporting to release/8.0-rc1: https://github.com/dotnet/runtime/actions/runs/6130417198

@github-actions github-actions bot mentioned this pull request Sep 9, 2023
Closed
@stephentoub
Copy link
Member

Was release/8.0-rc1 correct or should it have been release/8.0 instead?

@simonrozsival
Copy link
Member Author

@stephentoub you're right, it shouldn't be RC1. I'll open a new backport.

@simonrozsival
Copy link
Member Author

/backport to release/8.0

@github-actions
Copy link
Contributor

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6145796878

@github-actions github-actions bot mentioned this pull request Sep 11, 2023
Merged
marek-safar
marek-safar reviewed Sep 12, 2023
View reviewed changes
...aries/System.Net.Http/src/System/Net/Http/HttpClientHandler.AnyMobile.InvokeNativeHandler.cs
InvokeNativeHandlerMethod(getMethod, parameters: new object?[] { value }, cachingKey!);
}

private object InvokeNativeHandlerMethod(Func<MethodInfo> getMethod, object?[]? parameters, string cachingKey)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it not static?

...aries/System.Net.Http/src/System/Net/Http/HttpClientHandler.AnyMobile.InvokeNativeHandler.cs

private object InvokeNativeHandlerGetter(Func<MethodInfo> getMethod, [CallerMemberName] string? cachingKey = null)
{
return InvokeNativeHandlerMethod(getMethod, parameters: null, cachingKey!);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add Debug.Assert instead of only !

...aries/System.Net.Http/src/System/Net/Http/HttpClientHandler.AnyMobile.InvokeNativeHandler.cs
[DynamicDependency("set_Credentials", NativeHandlerType, AssemblyName)]
private void SetCredentials(ICredentials? value) => InvokeNativeHandlerMethod("set_Credentials", value);
private void SetCredentials(ICredentials? value)
=> InvokeNativeHandlerSetter(() => Type.GetType(NativeHandlerType)!.GetMethod("set_Credentials")!, value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps Type.GetType(NativeHandlerType) could come as an argument to the lambda and you could cache it too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understand it, by keeping the GetType and GetMethod calls together with constant arguments will give the trimmer enough information to keep only the methods that are actually used. If we cached the type, I think it wouldn't be trimmable anymore.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true. We don't have a way to do this with caching the Type (without suppressing some warnings).
Once we have UnsafeAccessorType then this can be rewritten to use that and it will not use any reflection and be faster.

...aries/System.Net.Http/src/System/Net/Http/HttpClientHandler.AnyMobile.InvokeNativeHandler.cs
[DynamicDependency("set_Credentials", NativeHandlerType, AssemblyName)]
private void SetCredentials(ICredentials? value) => InvokeNativeHandlerMethod("set_Credentials", value);
private void SetCredentials(ICredentials? value)
=> InvokeNativeHandlerSetter(() => Type.GetType(NativeHandlerType)!.GetMethod("set_Credentials")!, value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the lambdas should be static (not sure why the analyzer does not complain).

...aries/System.Net.Http/src/System/Net/Http/HttpClientHandler.AnyMobile.InvokeNativeHandler.cs
@@ -156,5 +153,36 @@ private static HttpMessageHandler CreateNativeHandler()

return (HttpMessageHandler)_nativeHandlerMethod!.Invoke(null, null)!;
}

private object InvokeNativeHandlerGetter(Func<MethodInfo> getMethod, [CallerMemberName] string? cachingKey = null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we could use CallerLineNumberAttribute or some other key which is not a next string in the assembly

@simonrozsival simonrozsival mentioned this pull request Sep 14, 2023
Closed
@karelz karelz added this to the 9.0.0 milestone Sep 14, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@marek-safar marek-safar marek-safar left review comments

@eerhardt eerhardt eerhardt left review comments

@vitek-karas vitek-karas vitek-karas left review comments

@MichalStrehovsky MichalStrehovsky MichalStrehovsky left review comments

@akoeplinger akoeplinger akoeplinger approved these changes

@steveisok steveisok Awaiting requested review from steveisok

Assignees

@simonrozsival simonrozsival

Labels
area-System.Net.Http linkable-framework Issues associated with delivering a linker friendly framework os-ios Apple iOS
Projects
None yet
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.
8 participants
@simonrozsival @MichalStrehovsky @stephentoub @marek-safar @akoeplinger @eerhardt @vitek-karas @karelz