Skip to content

[Trimming] Fix trimming warnings related to image source service provider#20058

Merged
rmarinho merged 28 commits intodotnet:net9.0from
simonrozsival:simpler-image-source-service-provider
Feb 12, 2024
Merged

[Trimming] Fix trimming warnings related to image source service provider#20058
rmarinho merged 28 commits intodotnet:net9.0from
simonrozsival:simpler-image-source-service-provider

Conversation

@simonrozsival
Copy link
Copy Markdown
Member

@simonrozsival simonrozsival commented Jan 22, 2024 •
edited
Loading

Description of Change

This PR removes trimming-unsafe patterns from ImageSourceServiceProvider (MakeGenericType, iterating over implemented interfaces).

Notes:

  • The FindImageSourceToImageSourceServiceTypeMapping method uses linear search to find the mapping. Linear search is fine in this case for two reasons: there are usually few image source types and we're caching the results.
  • I originally thought we'll need a new API on the IImageSourceServiceCollection interface but I came up with a workaround which doesn't need any changes to the public API and it will work with any customer implementation of the interface out of the box.
  • The IImageSourceServiceProvider.GetImageSourceType and IImageSourceServiceProvider.GetImageSourceServiceType methods now aren't used anywhere in the app. The main method, GetImageSourceService, doesn't use them and it uses the new mapping class instead. The implementation of these two methods is still trimming-unsafe and we might want to annotate them with [RequiresUnreferencedCode] and [RequiredsDynamicCode] once we enable trimming for the whole assembly. I think it is OK as is for the time being, customers would get trimming/AOT warnings when they call these methods themselves.
    • We should consider making these methods obsolete.

Issues Fixed

Contriubtes to #19397
Alternative to #19641

@simonrozsival simonrozsival added the perf/app-size Application Size / Trimming (sub: perf) label Jan 22, 2024
@simonrozsival simonrozsival requested a review from a team as a code owner January 22, 2024 10:54
@simonrozsival simonrozsival requested review from PureWeen, StephaneDelcroix, jfversluis, jonathanpeppers, mattleibow and mdh1418 and removed request for a team January 22, 2024 10:54
@simonrozsival simonrozsival force-pushed the simpler-image-source-service-provider branch from 63e7432 to 7f4d26c Compare January 23, 2024 12:00
@simonrozsival simonrozsival marked this pull request as draft January 23, 2024 19:44
mdh1418
mdh1418 reviewed Jan 25, 2024
View reviewed changes
@simonrozsival simonrozsival force-pushed the simpler-image-source-service-provider branch from 8fce729 to e62fedd Compare January 29, 2024 18:43
@simonrozsival simonrozsival force-pushed the simpler-image-source-service-provider branch from e62fedd to 3cf0b7f Compare January 29, 2024 19:36
@simonrozsival simonrozsival marked this pull request as ready for review January 30, 2024 16:05
simonrozsival and others added 2 commits January 31, 2024 08:52
@simonrozsival
Update tests
aa61c33
@simonrozsival @mdh1418
Update src/Core/src/Hosting/ImageSources/ImageSourceToImageSourceServ…
97a0c1c 
…iceTypeMapping.cs

Co-authored-by: Mitchell Hwang <16830051+mdh1418@users.noreply.github.com>
mattleibow
mattleibow reviewed Jan 31, 2024
View reviewed changes
src/Core/src/Hosting/ImageSources/ImageSourceServiceProvider.cs
return (IImageSourceService?)GetService(imageSourceService);
}

public Type GetImageSourceServiceType(Type imageSource) =>
Copy link
Copy Markdown
Member

@mattleibow mattleibow Jan 31, 2024

Choose a reason for hiding this comment

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

Should this method now be calling _imageSourceMapping.FindImageSourceServiceType?

Copy link
Copy Markdown
Member Author

@simonrozsival simonrozsival Feb 1, 2024

Choose a reason for hiding this comment

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

I'm still trying to figure out if it's better to keep the old behavior of this method but make it obsolete (but GetImageSourceServiceType(imageSource) might not necessarily match GetImageSourceService(imageSource)?.GetType()) or to change the way this method behaves and break someone's expectations. I'm going back and forth on this.

@mattleibow
Copy link
Copy Markdown
Member

mattleibow commented Jan 31, 2024

This looks very nice and the added tests is also good.

I did have a question about the method that used to be used GetImageSourceServiceType and shoukd that now also use the new logic?

We chatted about the fact that this potentially breaks people who bypass the AddService method, but this is probably not such a big issue because the real goal is to remove the base interfaces and types from IServiceCollection so that none of the extension methods appear. And some of those methods, like keyed and scoped services are not actualy supported.

Overall, this is a good change and helps clean up some of the old code that we wrote.

@simonrozsival
Copy link
Copy Markdown
Member Author

simonrozsival commented Jan 31, 2024

Honestly, I wasn't sure what to do with the GetImageSourceServiceType and GetImageSourceType. I think we agreed that it would make sense to obsolete them (they're not used anywhere in MAUI codebase anymore) so maybe I should just do this in this PR. I thought it might be best not to touch these methods if somebody uses them for some reason.

@rmarinho rmarinho deleted the branch dotnet:net9.0 January 31, 2024 11:33
@rmarinho rmarinho closed this Jan 31, 2024
@simonrozsival simonrozsival reopened this Jan 31, 2024
@mdh1418 mdh1418 mentioned this pull request Feb 1, 2024
Merged
@simonrozsival simonrozsival mentioned this pull request Feb 9, 2024
Closed
19 tasks
@rmarinho rmarinho self-assigned this Feb 9, 2024
@rmarinho rmarinho merged commit 4fbffd7 into dotnet:net9.0 Feb 12, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Mar 13, 2024
@Eilon Eilon added the area-publishing Issues with the app packaging/publishing process (ipk/apk/msix/trimming) label May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jonathanpeppers jonathanpeppers jonathanpeppers left review comments

@mattleibow mattleibow mattleibow left review comments

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

@mdh1418 mdh1418 mdh1418 left review comments

@rmarinho rmarinho rmarinho approved these changes

@StephaneDelcroix StephaneDelcroix Awaiting requested review from StephaneDelcroix StephaneDelcroix was automatically assigned from dotnet/dotnet-maui-reviewers

@jfversluis jfversluis Awaiting requested review from jfversluis jfversluis was automatically assigned from dotnet/dotnet-maui-reviewers

@PureWeen PureWeen Awaiting requested review from PureWeen

+1 more reviewer

@MichalStrehovsky MichalStrehovsky MichalStrehovsky left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@rmarinho rmarinho

Labels

area-publishing Issues with the app packaging/publishing process (ipk/apk/msix/trimming) fixed-in-9.0.0-preview.2.10247 fixed-in-9.0.0-preview.2.10293 perf/app-size Application Size / Trimming (sub: perf)

Projects

MAUI SDK Ongoing
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@simonrozsival @mattleibow @jonathanpeppers @rmarinho @vitek-karas @MichalStrehovsky @mdh1418 @Eilon @samhouts