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

Default IsDynamicCodeSupported feature switch to true for CoreCLR #80398

Closed
eerhardt opened this issue Jan 9, 2023 · 5 comments · Fixed by #103594
Closed

Default IsDynamicCodeSupported feature switch to true for CoreCLR #80398

eerhardt opened this issue Jan 9, 2023 · 5 comments · Fixed by #103594
Labels
area-System.Reflection.Emit in-pr There is an active PR which will close this issue when it is merged size-reduction Issues impacting final app size primary for size sensitive workloads
Milestone

Comments

@eerhardt
Copy link
Member

eerhardt commented Jan 9, 2023

With #80246, we added a feature switch for RuntimeFeature.IsDynamicCodeSupported. Previously when you published a trimmed CoreCLR app, RuntimeFeature.IsDynamicCodeSupported was hard-coded to true, so any logic switching on that property was eligible to be trimmed. For example:

RuntimeFeature.IsDynamicCodeSupported ?
new ReflectionEmitCachingMemberAccessor() :
new ReflectionMemberAccessor();

When you publish a CoreCLR trimmed app, the ReflectionMemberAccessor class could be trimmed. Now that we have a feature switch, the property is no longer a constant.

We could enable this by setting featuredefault=true in the ILLink.Substitutions.xml file, but that runs into the issue described in #96539.

This issue is to track enabling this for trimmed CoreCLR applications. It is blocked by #96539.

@eerhardt eerhardt added area-System.Reflection.Emit size-reduction Issues impacting final app size primary for size sensitive workloads labels Jan 9, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 9, 2023
@ghost
Copy link

ghost commented Jan 9, 2023

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

Issue Details

With #80246, we added a feature switch for RuntimeFeature.IsDynamicCodeSupported. Previously when you published a trimmed CoreCLR app, RuntimeFeature.IsDynamicCodeSupported was hard-coded to true, so any logic switching on that property was eligible to be trimmed. For example:

RuntimeFeature.IsDynamicCodeSupported ?
new ReflectionEmitCachingMemberAccessor() :
new ReflectionMemberAccessor();

When you publish a CoreCLR trimmed app, the ReflectionMemberAccessor class could be trimmed. Now that we have a feature switch, the property is no longer a constant.

We could enable this by setting featuredefault=true in the ILLink.Substitutions.xml file, but that runs into the issue described in #96539.

This issue is to track enabling this for trimmed CoreCLR applications. It is blocked by #96539.

Author: eerhardt
Assignees: -
Labels:

area-System.Reflection.Emit, size-reduction

Milestone: -

@buyaa-n buyaa-n added this to the 8.0.0 milestone Jan 11, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jan 11, 2023
@steveharter
Copy link
Member

@eerhardt is this still relevant for 8.0?

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

ghost commented Jul 24, 2023

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

@eerhardt
Copy link
Member Author

@eerhardt is this still relevant for 8.0?

It is still relevant, but It is blocked by #96539.

@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 Jul 24, 2023
@steveharter steveharter removed the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Aug 2, 2023
@steveharter
Copy link
Member

Moving to v9; assuming #96539 won't be done in v8.

@steveharter steveharter modified the milestones: 8.0.0, 9.0.0 Aug 11, 2023
@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 Jun 17, 2024
@sbomer sbomer closed this as completed in ff0c538 Jun 21, 2024
rzikm pushed a commit to rzikm/dotnet-runtime that referenced this issue Jun 24, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jul 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Reflection.Emit in-pr There is an active PR which will close this issue when it is merged size-reduction Issues impacting final app size primary for size sensitive workloads
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants