[dotnet-linker] Use [DynamicDependency] attributes instead of manual marking of NSObjects.#25146
Conversation
|
Azure Pipelines: 2 pipeline(s) require an authorized user to comment /azp run to run. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…marking of NSObjects. Also update the preservation code to not handle "Xamarin.iOS.dll" or "Xamarin.Forms.Platform.iOS", since those assemblies aren't used anymore. This makes it easier to move this code out of a custom linker step in the future. Contributes towards #17693.
62163b1 to
deda69c
Compare
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…dependency-attributes-marknsobjects
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The trimmer doesn't want to inline methods if their class has a static constructor, so this makes sure the trimmer will still inline this method.
…dependency-attributes-marknsobjects
This comment has been minimized.
This comment has been minimized.
✅ [CI Build #2bc9ea6] Build passed (Build packages) ✅Pipeline on Agent |
✅ [PR Build #2bc9ea6] Build passed (Detect API changes) ✅Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
simonrozsival
left a comment
There was a problem hiding this comment.
Adding [DynamicDependency] attributes forces the trimmer to keep extra metadata and so Native AOT app sizes will increase non-trivially (in the case of the macOS app it's 4.5%). If I understand it correctly and this is only supposed to be used in the case when ILLink does not support custom trimmer steps anymore and trimmable type maps are not ready yet or are not supported on the platform (Mono) or the developer chooses not to use the trimmable typemap, then I don't have any problems with this approach.
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ [CI Build #2bc9ea6] Build passed (Build macOS tests) ✅Pipeline on Agent |
rolfbjarne
left a comment
There was a problem hiding this comment.
🤖 PR Review Summary
Overall: ✅ LGTM
Well-structured refactoring that extracts MarkNSObjects logic into an IMarkNSObjects interface + MarkNSObjectsImpl shared implementation, allowing both the existing MarkSubStepsDispatcher path (direct marking) and the new AssemblyModifierStep path ([DynamicDependency] attributes) to share the same logic. This is a clean stepping stone toward moving this code out of the trimmer entirely.
What the PR does:
- Introduces
IMarkNSObjectsinterface andMarkNSObjectsImplstatic helper to share NSObject preservation logic between two linker step implementations. - Adds
MarkNSObjectsStep(anAssemblyModifierStep) that uses[DynamicDependency]attributes instead of directAnnotations.Mark(). - Moves
ValidateObjectPointersfromClasstoRuntimeto avoidIsBeforeFieldInit=falsecausing the trimmer to refuse inlining (well explained in PR description). - Removes dead
Xamarin.iOS/Xamarin.Forms.Platform.iOSspecial cases fromIsProductType. - Fixes attribute constructor marking in
AppBundleRewriter.CreateAttribute()— needed when running as a custom linker step. - Adds
[DynamicDependency]for proxy interfaces inManagedRegistrarStepso the trimmer does not remove them beforeManagedRegistrarLookupTablesruns. - Gates the whole
MarkDispatcherbehind_UseDynamicDependenciesForMarkDispatcher, which is only enabled when all 3 sub-steps have their DynamicDependency equivalents active.
Positive notes:
- The
allMemberTypesconstant carefully avoidsDynamicallyAccessedMemberTypes.Allto prevent preserving nested types — good attention to detail. - The mutual exclusion via
DidRunMarkNSObjectsStepflag is clean. - Expected test output updates look consistent across all platforms.
No blocking issues found. A few minor observations below.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
🚀 [CI Build #2bc9ea6] Test results 🚀Test results✅ All tests passed on VSTS: test results. 🎉 All 156 tests passed 🎉 Tests counts✅ cecil: All 1 tests passed. [attempt 2] Html Report (VSDrops) Download macOS tests✅ Tests on macOS Monterey (12): All 5 tests passed. Html Report (VSDrops) Download Linux Build VerificationPipeline on Agent |
Add a new
MarkNSObjectsStepstep that runs before the mark stage in the trimmer, which uses[DynamicDependency]attributes to ensureNSObjectsubclasses aren't trimmed away when they shouldn't be. The logic is identical to the existingMarkNSObjectssub step, exceptMarkNSObjectsdirectly marks APIs. The advantage is that the new step can easily be moved out of the trimmer (which will be done in a later stage).The new behavior is the default, but keep the old behavior behind an opt-out flag for now.
Also update the preservation code to not handle "Xamarin.iOS.dll" or "Xamarin.Forms.Platform.iOS",
since those assemblies aren't used anymore.
This makes it easier to move this code out of a custom linker step in the future.
There's one non-obvious part here: I had to move
Class.ValidateObjectPointersto theRuntimeclass, because now theClasstype'sIsBeforeFieldInitflag isfalse(because otherwise the trimmer doesn't honor the[DynamicDependency]attributes we're now adding to theClass's static cctor), and then the trimmer deems the static cctor as having side effects:https://github.com/dotnet/runtime/blob/16b9ae91f456c65ffcc5b28afc97034d828a19ad/src/tools/illink/src/linker/Linker.Steps/UnreachableBlocksOptimizer.cs#L268
and it refuses to inline anything:
https://github.com/dotnet/runtime/blob/16b9ae91f456c65ffcc5b28afc97034d828a19ad/src/tools/illink/src/linker/Linker.Steps/UnreachableBlocksOptimizer.cs#L526-L529
Contributes towards #17693.