-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Run illink task with at the end of build #31712
Comments
It can absolutely be explored. We just need to be cognizant with such a change of inner-loop development, where a developer iterates on an individual library. If illink doesn't run as part of that, it's more likely we'll see invalid tests and the like get written. |
Also, we can do this only for assemblies that ship in netcoreapp and not in a NuGet package. Can we estimate the benefits so that this can be prioritized against the other work? |
@danmosemsft could you please prioritize this, we'd like to start building in this mode ASAP to test the changes. |
@eerhardt is this something you can help with? |
I'm not sure this is a blocker though. The same situation applies with cross-platform. ex. I have my inner-loop development on Windows, but I just wrote a test that only fails on Linux. I think it is important that we run our CI tests on the output of the linker, though. That way we ensure what we ship to our customers works the same as what we test. I looked into it a bit today with the help of @safern @ViktorHofer and @joperezr. A problem that came up with this work is that there isn't a single place where we can run the linker that will produce netcoreapp assemblies that get picked up for both:
So doing this work with our current infrastructure means we would need to illink twice during our build. Once for the |
It's not a blocker per se, but I don't agree it's the same situation. I can iterate locally and know that the tests are passing on whatever OS I'm working on locally; I can iterate locally on multiple OSes and know the tests are passing on those; etc. With this change, I can iterate locally to my heart's content, but unless I'm doing a full build, I may still hit failures in the tests in CI on that exact OS I was using. |
I think you are on right path except for the fact the illink will be run once as part of the build and then on libraries-illink-test lane as a new testing configuration (this should have been done for 3.1). This is something that could leverage the in-place runtime packs for mobile testing we just added fro Mono runtime or you could write your own version. |
We may want to explicitly rethink the approach and guarantees of the dev inner loop, and accept that the inner-loop and whole product build are not the same and you may see a failure in one that you do not see in the other. This and other issues (#35112) suggest that we should:
|
I looked into doing this today, and I think we would only need to link one time during the full build. My current thinking is the following:
Now when our tests run, they will run against the trimmed assemblies. And the trimmed assemblies will get picked up in the runtime pack and shared framework installer. @ViktorHofer @ericstj @stephentoub @jkotas @joperezr @safern @marek-safar - Thoughts? |
[Aside since you mention overwrite, it is generally fragile to overwrite files in a build since if the build doesn't complete successfully the next build can be bad.] |
Yep. Do we still have the cases where the same binary ships both in OOB NuGet package and in the shared framework? I assume they would need to be special-cased somehow; or we may just get rid of them completely.
Agree. Stay aware from overwriting files. |
I didn't think we were building a shared framework for Mono. #35127 says:
But if we were to ever add a shared framework for mono, yes we would need to build 2 shared frameworks and link them separately.
Good point. I'll investigate what to do for CoreLib. |
Ok, replace "shared framework" with "runtime pack" in my comment. Runtime packs have the same problem. |
I think there are two paths which need fixing
We'll also run linker second time (in selected configurations) on the output as part of the testing matrix.
/cc @steveisok |
To clarify re: #35127... That is true right now in an installer context. The rest of the build for desktop mono behaves the same as you're used. We do plan on making desktop self contained at some point. |
Where should we move crossgening of CoreLib? The logical place feels like crossgening it with the rest of the libraries when we build the shared framework (during the src\installer build). But that would mean it wouldn't be crossgen'd for the tests. Will that be an issue? |
Yes, the tests will run noticeably slower, especially if you are on the checked runtime. It is one of the reasons why I have suggested #31712 (comment) |
I agree #31712 (comment) is where we will want to get, but I'm not sure we can complete it all in one shot. To make incremental progress toward that goal, and make progress on our size goals, what if we did the following:
This should allow us to get rid of a bunch of ILLinkTrim.xml files in src\libraries. One that can't be removed with this plan is the I have a prototype of the above plan here: https://github.com/eerhardt/runtime/commits/ILLinkEndOfBuild. @marek-safar @jkotas - do you think this is an acceptable, incremental step? After this is done, we can also make the step ILLink the Runtime Packs that are created when |
Make sure that the inner loop (iterating build + test on a single library without full build) still works after the change. Otherwise, it sounds reasonable to me. |
Since inner loop already requires one full build pass, can we allow incremental build to rerun illink and crossgen on the library as it's rebuilt? This way we still test the same thing that we ship? |
On my machine ILLink on the entire CoreCLR shared framework takes about 30 seconds. I know @vitek-karas and company are working on making it faster, but I don't believe this is something we want to do inside of the inner loop. (Note: you need to analyze the entire shared framework because some other assembly may have a |
This solution was used to solve similar problem for .NET Native and there may be still some leftovers in the tree. It is indeed very simple, but it was frowned upon as non-pure. |
I think that's an understatement. There are two areas where the current setup is problematic. The reflection/dynamic calls inside framework and size. The first one is undoubtedly more problematic, and unless we can remove all reflection/dynamic calls in src/libraries it'll be very tricky to ILLink only individual assemblies and ensure consistency and meet the size constraints at the same time. Saying that we don't care about ILLinker in the shared framework is also not that simple. The early complain @stephentoub raised was that the inner dev loop on the single assembly will be different. Making a shared framework build different to runtime packs is the other side of the same coin. Many developers develop for shared framework model only which means the same problem happens when they dev-loop on shared framework breaks self-contained version. |
Can you give some examples to help me understand where trimming the whole shared framework en mass rather than with individual libraries will net size wins? As noted above, I'm still unclear on that. |
Try to look at this from the angle of runtime packs. We can compile runtime pack for AOT on platform X, we could do it the way we stub for example |
Even better example may be hardware intrinsics. This would allow us to strip all e.g. ARM codepaths for x64 runtime pack or shared framework; and vice versa. |
I've updated the top comment on this issue to be more like a one-page spec of what we are trying to solve with this work. I hope it better describes the goals we are trying to achieve, and why we should do this. Please take a look and let me know if I've missed something or if you have other feedback. |
@stephentoub - The reason we are proposing to trim the whole shared framework en mass rather than individual libraries isn't for net size wins of the shared framework. It is because:
The main point is the last one. We want to enable trimmng as much code as possible for Xamarin/Blazor. (The main "goal" here IMO.) |
Great summary. That helped me understand better. |
Yes, this is the reflection case I already commented on.
What does that help other than net size wins?
Why is that only relevant when linking the whole framework? Isn't the only case it helps with cross-assembly private reflection?
That's a good example. |
My understanding is that the main goals we are trying to achieve with doing this work are:
The problem is the usage of ILLinkTrim.xml files to achieve goal (1) is interfering with goal (2). The cross-assembly private reflection is one main reason we are using ILLinkTrim.xml files, and doing this work allows us to remove most of them. Should shared framework size be a goal here? It was not on my list for reasons to do this originally, but it may change the approach we decide to take to achieve the two goals above. |
Is it really most of them? From #35199 (comment) it looks like it's a minority. Regardless, I'm no fan of the ILLinkTrim.xml files. I'm simply seeing that this cross-assembly private reflection is not a huge problem, showing up in just a handful of cases, so "duplication" to me isn't a huge downside (I'd also love to understand how much trimming these specific methods vs not is impacting app size). And as you noted in your revised intro post above, there are alternatives. For example, an attribute on internal/private members that need to be kept when the library is built (i.e. the first linker pass) but that themselves can be stripped during that build such that the subsequent app-build trimming could shake those out. All that said, if shared framework size is a goal, then I agree, we should change how we do the build, as we'd have to change something significant about the build anyway (e.g. we don't currently build assemblies other than corelib per architecture).
I would think so, otherwise I question our doing this at all. If it's not, changing the workflow to account for a handful of cases here that could be addressed in other ways seems like overkill to me. I also pointed out offline to @eerhardt another issue that will need to be addressed as part of this work. Today the linker is used not only for trimming code but also for stripping initlocals. If the linker moves to only be applicable when processing the whole shared framework, then so does its stripping of initlocals, which does have a very measurable impact on perf testing in inner loop for a good number of our assemblies and scenarios. Thus, if we do change to a model where the linker doesn't run as part of inner loop, before committing that change it'll be important to switch the repo to using [SkipLocalsInit]; that's something we should do anyway, but whereas previously it would just have been a nice-to-have, from my perspective doing it must be a precursor to any change here. I opened #35527 for that. |
SkipLocalsInit wouldn't do this either, right? Could we imagine a compiler directive to do it? Those also make lots of noise when we generate the dead code reports. |
Nope. It's unrelated. |
Right... would it be reasonable for the compiler to do it? |
That's being tracked in #35199 and right now it's over 50% for small apps for self-contained setups (this is not a scientific measurement and it's tricky to measure will all the scheduled work).
I think the option of running it at multiple places (during inner library dev loop + during bcl build) is also a possibility. |
Because of the few internal methods being accessed here via cross-assembly reflection?? I'm having a hard time believing that. Is it mainly the XML ones? |
For reference #35547 |
I just saw the update to the original issue comment and wanted to share a bit of a comment / clarification on my suggestion.
I disagree with this. It is not a hack to declare on an internal or private method that such a method may be called by another assembly. I actually think it is desirable to represent that API as something more than internal/private, which is why I initially suggested maybe making it public would be the right thing to do. If folks don't want to make things public it doesn't seem unreasonable to mark things as preserved in the public case. I honestly think the thing we are doing when we trim the shared framework to publics is a distinctly different task then when we trim an application. One thing that is interesting is to look at the limited usage of ILLinkTrim in the framework today: https://gist.github.com/ericstj/b7da2181d5f098a7421d425c6667ad16 |
At this point, to make forward progress, #35547 is taking the If we agree that is a valid solution, we can use the exact same solution for the following assemblies:
After those, the remaining ILLinkTrim.xml usages fall into one of the following buckets:
The first 2 could be eliminated if we added new features to mono/linker. XElement is a special case that might need to be handled specially. This would allow us to remove all the ILLinkTrim.xml files from the compiled libraries and allow the produced assemblies to be linker safe. Would moving forward with this plan be acceptable to everyone? If we later decided to link the entire shared framework en mass during the build, all we would need to do is remove the ILLinkTrim.xml files. |
That sounds like a good path forward to me. The shared framework size issue can still be followed-up on, but as a separate issue that need not block forward progress here with this approach. And we can always revise/revert this approach later if we end up doing the whole framework linking, anyway, for shared framework size reasons. |
The bucketing makes sense to me but I disagree that we should use the exact same solution for all the problems (e.g. COM is supported on a small subset of platforms so the solution should reflect that). I'd suggest creating 3 different tracking issues or PRs where we can discuss the right solution for each of them. |
Done.
|
Since it sounds like the consensus is to not pursue this approach for now, I'm going to close this issue. To address #35199, we can follow approach |
Problem Statement
Today when linking a Xamarin or Blazor application using the
dotnet/runtime/src/libraries
we are not able to trim a lot of unused code from the libraries. One major reason unused code is left in the libraries is because the libraries are usingILLinkTrim.xml
files in order to preserve members that are invoked dynamically across assemblies.Goals
dotnet/runtime/src/libraries
and be as small as possible.Background
Take an example case where
AppDomain
is calling a private method inSystem.Security.Claims
.runtime/src/libraries/System.Private.CoreLib/src/System/AppDomain.cs
Lines 389 to 400 in 9af1c5e
runtime/src/libraries/System.Security.Claims/src/System/Security/Claims/GenericPrincipal.cs
Lines 86 to 87 in 9af1c5e
A simple usage of Reflection like this can be understood by the
mono/linker
when both assemblies are given to it at the same time. It sees a hard-coded Type and MethodInfo is being retrieved, and it understands that whenAppDomain.GetThreadPrincipal()
is called, it has a dependency onGenericPrincipal.GetDefaultInstance()
. Thus, whenAppDomain.GetThreadPrincipal()
must be kept, it must also keepGenericPrincipal.GetDefaultInstance()
. And in the reversed case, if an application doesn't need to executeAppDomain.GetThreadPrincipal()
, the linker can also safely removeGenericPrincipal.GetDefaultInstance()
.For cases that the linker can't figure out itself, we are adding a new attribute
DynamicDependency
(#30902), which we can place onAppDomain.GetThreadPrincipal()
to annotate that it uses Reflection to callGenericPrincipal.GetDefaultInstance()
. So even if the linker can't recognize the pattern, we can still be safe.Today, when we run linker on the
dotnet/runtime/src/libraries
, we are linking a single assembly at a time. So when we linkSystem.Security.Claims
, the linker has no idea thatAppDomain.GetThreadPrincipal()
requiresGenericPrincipal.GetDefaultInstance()
(sinceAppDomain
is in a different assembly). To prevent the linker from removingGenericPrincipal.GetDefaultInstance()
, we use anILLinkTrim.xml
file that tells the linker to keep the private method. We also then embed theILLinkTrim.xml
file into the assembly so when other apps usingSystem.Security.Claims
are linked, the linker will still know thatGenericPrincipal.GetDefaultInstance()
is needed and not remove it.The problem is that
ILLinkTrim.xml
files tell the linker to ALWAYS leave a member in the assembly. It doesn't know WHY the member is needed. Thus, even if the application doesn't useAppDomain.GetThreadPrincipal()
, the linker is still leaving inGenericPrincipal.GetDefaultInstance()
.Proposed Solution
Instead of linking a single assembly at a time, we should link in the context of the entire "distributable". We can then remove the
ILLinkTrim.xml
files, and instead add the correct annotations to the code directly. This will give the linker the whole picture of what code is required dynamically by what other code. And when using the library in a different "distributable", the linker can better tell what code can be trimmed safely.The one "distributable" we are linking for today is the
Microsoft.NETCore.App
's shared framework. This would mean to link the entire shared framework at the end of our build, and not during each individual library.One drawback of this approach is that when incrementally building a single library we won't be trimming it anymore during the inner dev loop. It currently takes ~30s to link the entire shared framework, which we wouldn't want to do during the inner dev loop. Unless the linker can support incremental linking, we won't be trimming the individual library. This may result in differences in behavior between the inner dev loop and a full build. Code will be left in the assembly during the inner dev loop that may be trimmed away in the full build. However, these situations wouldn't be able to get past CI since it does a full build.
Alternative Solutions
Still link individual assemblies, but only use the
ILLinkTrim.xml
files when linking for the shared framework.ILLinkTrim.xml
. We will still need to add attributes to the code in order for the libraries to be linker safe when linking for Xamarin applications. And the linker won't be able to see the attributes in other assemblies when linking a single assembly - so we will also need to keep the information in theILLinkTrim.xml
.Don't trim the libraries in the shared framework at all.
master
branch here are the size differences in the end shared framework on win-x64:t:GenerateReferenceSource
). Now the developer will have to remember to go remove these "fake" public methods in the ref assembly every time it is re-generated.Original comments
This will allow it to apply more optimizations than when running with reference assemblies (e.g. constant propagation). It'll be required to reduce framework size when built-in substitutions are applied (e.g. intrinsics).
Another benefit we get is that
PreserveDependency
will work correctly as it will be applied to final assembly, not some intermediate reference.The text was updated successfully, but these errors were encountered: