-
Notifications
You must be signed in to change notification settings - Fork 127
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
Add compatibility attribute when linker is used as analyzer tool #1269
Comments
I considered opening a new issue for the write-up below, but I felt this proposal is close enough to this discussion, that I decided to add it to this issue: ProblemWith Blazor WASM in .NET 5 we shipped an untrimmed
If a trimmed app references any of the above assemblies that come from dotnet/runtime, they won't be trimmed by default. Solution alternativesWe should consider a model where a NuGet package can some how opt into being trimmed by default. That way a user of a package doesn't have to "guess" if an assembly is safe for trimming, and having to mark the assembly as "trimmable" themselves. The above is one proposal for how to do this. However, with the complete system we have today, we will have some problems with the assembly level attribute. The reason being that the .NET SDK marks every assembly that isn't marked as "trimmable" as Another potential issue with the top post's proposal is generating an assembly level attribute dynamically needs to be done in such a way that it works with "deterministic" builds. Since we don't have the ILLinker used as an analyzer tool yet, I'm not sure how this would work - if it would be a pre or post compile step. Just something to ensure we handle correctly. Another alternative is to use the MSBuild extensibility we added in 5.0: dotnet/sdk#12035. In this solution, each NuGet package would need to ship a Another alternative would be to add a new flag to the NuGet nuspec file that says "all the managed libraries in this package should be opted into trimming by default". We have similar metadata in NuGet packages - for example the serviceable attribute. However, it might not be great to add an ILLinker specific attribute directly into a nuspec file. We could also consider having a "sentinel" file in the nuget package that the SDK sees, and uses as "this assembly is trimmable". ThoughtsI think the assembly level attribute approach has the best possibility of working. However, we will need to figure out how to make it work correctly with the SDK targets and command line options. This may drastically change the way we invoke the ILLinker today. We can take baby-steps to get there:
Then eventually, when we have the ILLinker running as an analyzer, we can figure out how to dynamically generate this attribute, if necessary. I think we need to figure out a path forward here in .NET 6. There are more libraries that need to be trimmed by default than what is in the Microsoft.NETCore.App runtimepack today. |
Do we expect that going forward we'll keep the behavior that framework assemblies get trimmed by default, and everything else gets rooted? Trimmability is not a property of an assembly - attribution that declares "I don't dynamically use any of my own code" is not very useful - there might still be code in another assembly that dynamically uses code in this assembly. We applied such rule to framework assemblies because maybe it's a bit less likely that someone will be reflecting on the framework, but it's just a heuristic with no guarantees of actually helping. There are assemblies within framework where this is more likely to work, but also assemblies where it's less likely (e.g. assemblies that have classes that might be considered "data that is likely to be reflection-serialized" vs assemblies that have mostly code in them and don't represent data/state). For .NET 6 I would hope we'll look more into directions where linker can prove correctness, as opposed to investing in heuristics that may result in broken apps. |
I agree this setup is not ideal but we can certainly improve it. My suggestion would be to not treat the shared framework assemblies differently to any other library. If we standardized to assembly-level attribute we can add it to all libraries including shared framework assemblies. Then we don't pass any There is already attribute for that called LinkerSafeAttribute which we'll have to migrate to .NET6 for compatibility. I'm not saying we have to use this one but it'd make the migration for mobile easier.
No, we should make it possible to trim any library and in .NET6 we want to trim a couple of SDKs fully as well.
I don't think it would be a good experience to run analyzer before linking all the times on libraries which are "static" for the end developer. We should run the verification at the end of the library build, that means once. If there are no issues detected it could be automatically marked as fully trimmable. Presumably, this could be done during build time via one analyzer but this probably needs more thoughts and it should not block us to allows this to be achieved manually. |
To me it seems we're mixing two concepts here:
Blazor and Xamarin both want to use the ability to define a set of assemblies to trim - as a heuristic to improve application sizes without breaking them too much. We were hoping that in 6 we would be able to invest into framework and some nugets to actually fix the correctness issue - which is not addressed by any of the above. There will be some work, but probably not enough to get us to a place where we can have true correctness for some end to end scenario (like Blazor or Xamarin). As such it's likely we will need to keep the heuristics around even for 6. Based on the description above - I would stay away from names like "linker safe", "trim compatible" or anything similar for the proposed attribute. The attribute would be a technical way how to define the set of assemblies to apply aggressive trimming to in our heuristic scenarios. Nothing more, or less. I can see a world where we add it to even an assembly which still produces some linker warnings on its own, although it would be nice to not do that. Let me reiterate: adding such attribute to an assembly doesn't make it safe to trim - the question if something is safe to trim or not can only be answered globally, not on per-assembly level. So maybe a name like |
As for what the SDK should do - I think the current behavior is weird around assemblies like The really "weird" part comes for the rest of the assemblies. The default action is set to "link" and then it singles out every assembly it doesn't know about (anything but frameworks). That is a really weird approach given that "link" is the most aggressive action. I would do it the other way round - default to the "safest" action, so probably "copy" and then single out framework assemblies with Once we have the attribute discussed here, we could omit the If I understand how actions work in the linker, the |
I like the name or we could go with |
I see the action as a decision made about how to link a particular assembly at link time - not a property of the assembly. After all, we want different actions for the wasm runtime than for the desktop runtime. As such I find it a bit strange to bake it in as an attribute.
The issue with this is that the global Putting the action in an assembly attribute would also require us to crack open every assembly just in case its action is
We pass |
I realized it's worse than this - if we want to keep assemblies that haven't opted into trimming via an attribute, we will need to scan them all - regardless of how we pass options on the command-line. Considering some scenarios where SDK options interact with the attributes:
I'd like to keep this as simple as possible. I think that in the most common scenarios, a developer (framework author or external) simply wants to enable trimming of their assembly or of an assembly dependency of their app. Why don't we just support an unparameterized attribute like The semantics would be that explicit actions ( Either way we still need the ability to root and keep everything that isn't opted into trimming. |
To summarize your proposed logic the linker will be altered to follow these rules
Does it sound right? |
I would clarify a couple points. I think it's the same as what you are saying, but I want to be sure:
|
Sorry to start a bit high-level, but I think it will make sense that way: Goals:
So to this end (didn't think it through all the details yet, but in general):
It's very similar to how it works today, the main difference is the attribute which acts basically identically to the SDK's
We should agree on the behavior first, and then we can design how this maps to linker command line. We can basically start with the lists defined in SDKs the way it is now, an migrate over to having the attributes in the assemblies and SDKs not specifying the list as we go. Long term - SDKs will in time ideally migrate to |
I think that's generally a good summary of the behavior we want. This is pretty much the key point:
Note on terminology - the way I think about it, It sounds like there are two additional suggestions:
Both sound like good usability improvements. |
I'm not sure this is the right order. I think the assembly attribute should win over SDK so a new assembly version does not require SDK update if there is a problem with the assembly. I'm also a bit concern about the expansion of the settings. From what I gathered there will be following settings which can alter assembly trimming
Do we need all these to alter the trimming? |
I opened https://github.com/dotnet/sdk/issues/14642 about the extra config options - let's keep this discussion about the attribute behavior and existing options.
I don't follow - could you clarify what kind of problem you mean? I think it makes sense for
I'd propose making this just (present/not-present). Not-present means it will not be trimmed unless the developer explicitly opts in by setting
(There's no AssemblyTrimMode property - I think you mean Technically |
My thinking when proposing the priority order was:
The easy to use properties (include all, and item group) are just that, make it easier. The main driver for that is the current complexity of modifying the defaults. It's just too complex. I understand we don't need to make it super easy, but we're currently making it very hard. |
This to me is like brute force which can go wrong easily. SDK will probably not really deeply investigate the context of the assembly so let's use an example from Eric. Imagine we have
Are we going to deal with RIDs and NuGet versions to make this actually work or is the idea to patch everyone everywhere regardless of implementation? I wonder if we need the metadata IsTrimmable at all if we now have to send all assemblies to linker anyway for -p handling or attribute inspection.
That's what I understood as "Allow opting into trimming via an ItemGroup, not just via IsTrimmable metadata". I think it's crucial during the transition period to allow developers to easily experiment with enabling trimming for any assembly. It should be as simple as passing/adding msbuild property. Something like |
When I talk about
I see what you mean - I would move away from hard-coding any I think the |
Ideally we would move away from specifying
We're already sending all assemblies to the linker via I also don't understand why we need to read all assemblies to figure out the attribute. Maybe if the defaults are set to |
Assemblies without the attribute will need to be preserved, so we must look for the attribute to know if we are allowed to remove code from an assembly (or remove the whole assembly). There are two "defaults":
|
I see - makes sense. We could probably do something super fast using S.R.Metadata and file mapping instead of Cecil just to get the attribute. We'll have to measure it a bit, but I don't think it's going to be too noticeable if done right. |
This includes some background on what we added for .NET5, and a proposal for more options in .NET6 : - An assembly-level attribute to opt into trimming - Simplified trimming opt-in from the SDK - Simplified "trim all assemblies" flag This is a summary of the discussion in dotnet#1269 and https://github.com/dotnet/sdk/issues/14642, and the .NET 5 discussion in dotnet/sdk#12035.
* Add design doc for trimming opt-in/opt-out This includes some background on what we added for .NET5, and a proposal for more options in .NET6 : - An assembly-level attribute to opt into trimming - Simplified trimming opt-in from the SDK - Simplified "trim all assemblies" flag This is a summary of the discussion in #1269 and https://github.com/dotnet/sdk/issues/14642, and the .NET 5 discussion in dotnet/sdk#12035. * Add existing AssemblyMetadata example * PR feedback * Fix typo * Update docs/design/trimmed-assemblies.md Co-authored-by: Eric Erhardt <[email protected]> * Apply suggestions from code review Co-authored-by: Eric Erhardt <[email protected]> * PR feedback * Add notes about opt-out * Use present tense * Add more opt-out notes Co-authored-by: Eric Erhardt <[email protected]>
I think this has been basically solved via the |
I think so. Just to note differences from the original proposal:
The intent is for
The |
* Add design doc for trimming opt-in/opt-out This includes some background on what we added for .NET5, and a proposal for more options in .NET6 : - An assembly-level attribute to opt into trimming - Simplified trimming opt-in from the SDK - Simplified "trim all assemblies" flag This is a summary of the discussion in dotnet/linker#1269 and https://github.com/dotnet/sdk/issues/14642, and the .NET 5 discussion in dotnet/sdk#12035. * Add existing AssemblyMetadata example * PR feedback * Fix typo * Update docs/design/trimmed-assemblies.md Co-authored-by: Eric Erhardt <[email protected]> * Apply suggestions from code review Co-authored-by: Eric Erhardt <[email protected]> * PR feedback * Add notes about opt-out * Use present tense * Add more opt-out notes Co-authored-by: Eric Erhardt <[email protected]> Commit migrated from dotnet/linker@c45a25d
This is a proposal to add global assembly attribute for assemblies which linker processes as an analyzer. The attribute could be something like
ILLinkCompatibility ("{mode}", generated: true)
which would indicate that during the assembly processing illinker didn't find any problems and are safe to be linked. The rules what it means exactly would need to be defined but it should include things like no warnings from flow-analysis, no usage of linker incompatible APIs, etc. The linker would not add this attribute if added manually by the user.The mode value would reflect the highest linker compatibility level and would make to linker actions, specifically
The usage scenario would then be for any NuGet packages to be automatically linker enabled without doing any manual work on packages or references level.
The text was updated successfully, but these errors were encountered: