-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[release/8.0-rc1] Annotate System.Linq.Expressions with RequiresDynamicCode #90616
[release/8.0-rc1] Annotate System.Linq.Expressions with RequiresDynamicCode #90616
Conversation
All this ended up with an RUC on Expression.Compile due to new arrays. I could potentially silence this warning with a feature flag, but it is a real risk, and one that users could maybe work around if alterted to the problem.
…ons/Expression.cs Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
Co-authored-by: Stephen Toub <stoub@microsoft.com>
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
Tagging subscribers to this area: @cston Issue DetailsBackport of #90456 to release/8.0-rc1 /cc @agocke Customer ImpactTestingRiskIMPORTANT: If this backport is for a servicing release, please verify that:
|
@agocke do these changes have any app size implications? |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
The CI has failures. Is any of them related to this PR? If not, let me know so I can merge it (please don't click on "Update branch"). |
None of the failures seem related. I think this is good to merge. @ivanpovazan This shouldn't have any material impact on size either way. It'll root |
Thank you for verifying
Just for the reference: I did a quick check with a sample iOS application referencing |
Thanks for getting this into .NET 8, @agocke. |
@ivanpovazan That still seems negligible, but slightly more than I expected. Did you happen to confirm that the only difference is the types I mentioned? |
I will try to do a diff with sizoscope and post it here, but probably tomorrow. |
|
Hmm, weird, the sizoscope difference shows 8 KB, which is closer to what I expected, instead of 50. @MichalStrehovsky might have some insight when he gets back |
I repeated the measurements with a console app built on osx-arm64 and the difference is That being said, sorry for the false alarm regarding iOS size bump. |
No problem -- thanks for the info. I thought I fixed the linker size issue, I may still need to look into it more. |
Sizoscope/mstat doesn't capture everything in the executable but the size is usually proportional. You can get a closer view with IlcGenerateMapFile - the sizes of blobs that mstat cant account for might tell more. |
Backport of #90456 to release/8.0-rc1
/cc @agocke
Customer Impact
System.Linq.Expressions is a complicated area for Native AOT. Optimized compilation is not supported at all, but interpretation is. However, even with that, array creation and lifting of custom operators are not supported. This PR annotates array creation and disables custom operator lifting via a feature switch.
Notably, this does present a breaking change if you were relying on nullable lifting on custom operators in expression trees. There is no workaround in this situation. This feature is not supported in Native AOT.
Testing
Running existing unit tests and integration tests under Native AOT.
Risk
Low. This is an AOT-only change and mostly annotation-only.