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.

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

ILLink shoud keep all metadata for rooted members #81979

Closed
jeromelaban opened this issue Feb 10, 2023 · 11 comments · Fixed by #102850
Closed

ILLink shoud keep all metadata for rooted members #81979

jeromelaban opened this issue Feb 10, 2023 · 11 comments · Fixed by #102850
Assignees
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Tools-ILLink .NET linker development as well as trimming analyzers in-pr There is an active PR which will close this issue when it is merged needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Milestone

Comments

@jeromelaban
Copy link
Contributor

Background and motivation

In relation to 166ceb8, #73241 and #74141, one possible avenue to allow for System.Text.Json to work with the IL Linker is to allow for metadata to be kept. The linker is currently trimming all metadata, regardless of the presence of the type in the trimmer root configuration file.

Exposing the Linker task's KeepMetadata:

public ITaskItem[] KeepMetadata { get; set; }

would allow an easier migration path without resorting to generated serializers.

API Proposal

<TrimmerKeepMetadata>all</TrimmerKeepMetadata>

API Usage

<TrimmerKeepMetadata>all</TrimmerKeepMetadata>

or

<TrimmerKeepMetadata>parametername</TrimmerKeepMetadata>

Alternative Designs

No response

Risks

I can't think of any, since not specifying the keep metadata is introducing more issues than it solves (even if it reduces the size impact of the linker)

@jeromelaban jeromelaban added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Feb 10, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Feb 10, 2023
@ghost
Copy link

ghost commented Feb 10, 2023

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

Issue Details

Background and motivation

In relation to 166ceb8, #73241 and #74141, one possible avenue to allow for System.Text.Json to work with the IL Linker is to allow for metadata to be kept. The linker is currently trimming all metadata, regardless of the presence of the type in the trimmer root configuration file.

Exposing the Linker task's KeepMetadata:

public ITaskItem[] KeepMetadata { get; set; }

would allow an easier migration path without resorting to generated serializers.

API Proposal

<TrimmerKeepMetadata>all</TrimmerKeepMetadata>

API Usage

<TrimmerKeepMetadata>all</TrimmerKeepMetadata>

or

<TrimmerKeepMetadata>parametername</TrimmerKeepMetadata>

Alternative Designs

No response

Risks

I can't think of any, since not specifying the keep metadata is introducing more issues than it solves (even if it reduces the size impact of the linker)

Author: jeromelaban
Assignees: -
Labels:

api-suggestion, area-System.Reflection.Metadata

Milestone: -

@teo-tsirpanis
Copy link
Contributor

easier migration path without resorting to generated serializers

The migration path is exactly the one of the JSON source generator. Why can't you use it, and why don't trimmer root descriptors solve your problem?

@ghost
Copy link

ghost commented Feb 11, 2023

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

In relation to 166ceb8, #73241 and #74141, one possible avenue to allow for System.Text.Json to work with the IL Linker is to allow for metadata to be kept. The linker is currently trimming all metadata, regardless of the presence of the type in the trimmer root configuration file.

Exposing the Linker task's KeepMetadata:

public ITaskItem[] KeepMetadata { get; set; }

would allow an easier migration path without resorting to generated serializers.

API Proposal

<TrimmerKeepMetadata>all</TrimmerKeepMetadata>

API Usage

<TrimmerKeepMetadata>all</TrimmerKeepMetadata>

or

<TrimmerKeepMetadata>parametername</TrimmerKeepMetadata>

Alternative Designs

No response

Risks

I can't think of any, since not specifying the keep metadata is introducing more issues than it solves (even if it reduces the size impact of the linker)

Author: jeromelaban
Assignees: -
Labels:

api-suggestion, area-System.Text.Json, untriaged

Milestone: -

@jeromelaban
Copy link
Contributor Author

The migration path is exactly the one of the JSON source generator. Why can't you use it

Backward source compatibility with previous versions of .NET which do not have support for generators is one. Migration to later versions of .NET without changing code is about removing friction for developers.

and why don't trimmer root descriptors solve your problem?

Descriptors only impact types visibility, not metadata like parameters. This ILLink option is exactly about the scenario mentioned above.

The use case raising this proposal is for mobile apps using iOS/Android/Catalyst which require the use of the linker to not be large size regression from AOT-enabled Xamarin-based builds.

@MichalStrehovsky
Copy link
Member

The linker is currently trimming all metadata, regardless of the presence of the type in the trimmer root configuration file.

Can you expand on this? Type that is specified as a root should not have the parameter names trimmed on any of its method. If it does, that's a trimming bug.

The only methods that can have parameter names trimmed are those that are not visible targets of reflection. Methods that are not visible targets of reflection are fair game to optimize. Trimming can: inline such method and delete it, rename such method, strip parameter names, etc. Removing parameter names is only one of the optimizations trimmings does (or will do in the future).

@MichalStrehovsky MichalStrehovsky added area-Tools-ILLink .NET linker development as well as trimming analyzers and removed area-System.Text.Json labels Feb 11, 2023
@ghost
Copy link

ghost commented Feb 11, 2023

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

In relation to 166ceb8, #73241 and #74141, one possible avenue to allow for System.Text.Json to work with the IL Linker is to allow for metadata to be kept. The linker is currently trimming all metadata, regardless of the presence of the type in the trimmer root configuration file.

Exposing the Linker task's KeepMetadata:

public ITaskItem[] KeepMetadata { get; set; }

would allow an easier migration path without resorting to generated serializers.

API Proposal

<TrimmerKeepMetadata>all</TrimmerKeepMetadata>

API Usage

<TrimmerKeepMetadata>all</TrimmerKeepMetadata>

or

<TrimmerKeepMetadata>parametername</TrimmerKeepMetadata>

Alternative Designs

No response

Risks

I can't think of any, since not specifying the keep metadata is introducing more issues than it solves (even if it reduces the size impact of the linker)

Author: jeromelaban
Assignees: -
Labels:

api-suggestion, untriaged, area-Tools-ILLink

Milestone: -

@MichalStrehovsky MichalStrehovsky added the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 13, 2023
@ghost
Copy link

ghost commented Feb 13, 2023

This issue has been marked needs-author-action and may be missing some important information.

@jeromelaban
Copy link
Contributor Author

Can you expand on this? Type that is specified as a root should not have the parameter names trimmed on any of its method. If it does, that's a trimming bug.

The scenario is as follows. A record type present in a separate assembly, for which that full assembly is explicitly excluded in a TrimmerRootDescriptor definition file, has its constructor parameter names removed unless --keep-metadata all or --keep-metadata parametername is set on the linker parameters.

The Linker is running during the build of an net7.0-android or net7.0-ios app, though it's very likely that this behavior is the same for other targets.

@ghost ghost added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Feb 13, 2023
@agocke agocke added this to AppModel Jul 10, 2023
@agocke
Copy link
Member

agocke commented Aug 9, 2023

@sbomer Is this the expected behavior? Do we trim parameter names even for assemblies that are preserved using TrimmerRootAssembly?

@agocke agocke added this to the Future milestone Aug 9, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Aug 9, 2023
@sbomer
Copy link
Member

sbomer commented Aug 9, 2023

I've confirmed the behavior - using a root descriptor doesn't prevent parameter names from being stripped. I agree with @MichalStrehovsky's comment in the sense that I think we should treat members rooted by descriptor as reflection-visible (we currently don't).

@HenriquecerMatos
Copy link

I have the same problem

@sbomer sbomer changed the title [API Proposal]: Expose the Linker's KeepMetadata parameter ILLink shoud keep all metadata for rooted members May 2, 2024
@sbomer sbomer self-assigned this May 2, 2024
@sbomer sbomer moved this to High Priority in AppModel May 2, 2024
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label May 29, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jun 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Tools-ILLink .NET linker development as well as trimming analyzers in-pr There is an active PR which will close this issue when it is merged needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants