Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 578
Move FixLegacyResourceDesignerStep to post-trim pipeline#11059
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
70f7f3077de75de9edefdf0059fd64d4ed1924649e8be3817f12d74783a8f646e870efa92e39f46912231eec909196e393File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -6,6 +6,7 @@ | ||
| using Microsoft.Android.Build.Tasks; | ||
| using Microsoft.Build.Framework; | ||
| using Mono.Cecil; | ||
| using Mono.Linker; | ||
jonathanpeppers marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| using MonoDroid.Tuner; | ||
| namespace Xamarin.Android.Tasks; | ||
| @@ -16,7 +17,7 @@ namespace Xamarin.Android.Tasks; | ||
| /// This opens each assembly once (via DirectoryAssemblyResolver with ReadWrite) and | ||
| /// runs all registered steps on it, then writes modified assemblies in-place. Currently | ||
| /// runs CheckForObsoletePreserveAttributeStep, StripEmbeddedLibrariesStep and | ||
| /// (optionally) AddKeepAlivesStep. | ||
| /// (optionally) AddKeepAlivesStep and FixLegacyResourceDesignerStep. | ||
| /// | ||
| /// Runs in the inner build after ILLink but before ReadyToRun/crossgen2 compilation, | ||
| /// so that R2R images are generated from the already-modified assemblies. | ||
| @@ -34,6 +35,8 @@ public class PostTrimmingPipeline : AndroidTask | ||
| public bool Deterministic { get; set; } | ||
| public bool UseDesignerAssembly { get; set; } | ||
| public override bool RunTask () | ||
| { | ||
| using var resolver = new DirectoryAssemblyResolver ( | ||
| @@ -100,6 +103,15 @@ public override bool RunTask () | ||
| }, | ||
sbomer marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| (msg) => Log.LogDebugMessage (msg))); | ||
| } | ||
| if (UseDesignerAssembly) { | ||
| // Create an MSBuildLinkContext so FixLegacyResourceDesignerStep can resolve assemblies | ||
| // and log messages. The resolver is owned by the outer 'using' block, so we intentionally | ||
| // do not dispose this context (LinkContext.Dispose would double-dispose the resolver). | ||
| var linkContext = new MSBuildLinkContext (resolver, Log); | ||
| var fixLegacyStep = new FixLegacyResourceDesignerStep (); | ||
| fixLegacyStep.Initialize (linkContext); | ||
| steps.Add (new PostTrimmingFixLegacyResourceDesignerStep (fixLegacyStep)); | ||
| } | ||
jonathanpeppers marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| foreach (var (item, assembly) in loadedAssemblies) { | ||
| var context = new StepContext (item, item); | ||
| @@ -118,3 +130,24 @@ public override bool RunTask () | ||
| return !Log.HasLoggedErrors; | ||
| } | ||
| } | ||
| /// <summary> | ||
| /// Thin wrapper around <see cref="FixLegacyResourceDesignerStep"/> for the post-trimming pipeline. | ||
| /// Calls <see cref="FixLegacyResourceDesignerStep.ProcessAssemblyDesigner"/> directly, matching the | ||
| /// behavior of the former ILLink path which processed all assemblies without StepContext flag filtering. | ||
| /// Assemblies without a resource designer are skipped internally by ProcessAssemblyDesigner. | ||
| /// </summary> | ||
| class PostTrimmingFixLegacyResourceDesignerStep : IAssemblyModifierPipelineStep | ||
| { | ||
| readonly FixLegacyResourceDesignerStep _inner; | ||
| public PostTrimmingFixLegacyResourceDesignerStep (FixLegacyResourceDesignerStep inner) | ||
| { | ||
| _inner = inner; | ||
| } | ||
| public void ProcessAssembly (AssemblyDefinition assembly, StepContext context) | ||
| { | ||
| context.IsAssemblyModified |= _inner.ProcessAssemblyDesigner (assembly); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖⚠️ Performance — The
preserve="all"descriptor prevents ILLink from trimming unused resource properties in the designer assembly. In the old ILLink custom step flow,FixLegacyResourceDesignerStepran beforeMarkStep, so ILLink could track which designer properties were actually referenced and trim the rest. With the post-trim approach, all properties must survive ILLink intact.The
BuildReleaseArm64XFormsDotNet.MonoVM.apkdescshows the designer assembly growing from ~19 KB to ~258 KB (~13× increase). For the simple app it's negligible (+400 bytes), but resource-heavy apps will see a larger impact.The TODO references dotnet/runtime
#126518for switching toTrimmerRootAssembly. However, the_FixRootAssemblytarget upgrades allTrimmerRootAssemblyentries toRootMode="All", so switching wouldn't reduce the preserved surface — it would just simplify the MSBuild XML. Is the size regression considered acceptable for the architectural benefits of removing the ILLink step, or is there a plan to recover trimming of unused designer properties later?Rule: Don't remove caches without measurement (Postmortem
#57)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is accurate, and by design per #11059 (comment).