Need to figure out what to do with ILLink in source-build #580
Comments
|
@eerhardt I know corefx is using illink for avoiding zeroing stackalloced memory. Is it is used for other things? |
|
@erozenfeld @stephentoub any thoughts on implications of turning this off our source-builds? |
Yes, both coreclr and corefx use the linker to trim away dead code and to avoid zero'ing locals. (Though it appears from your initial comment the trimming was turned off for corefx... I wasn't aware of that. I know I've had cases in recent memory where things got trimmed away, so I'm not sure what's going on, maybe it's a very recent change? Or... maybe that's just for source build?) Implications of turning off the linker would be larger binaries and less-efficient code; in particular for corelib, the latter was very measurable in various microbenchmarks around things like string processing, integer formatting and parsing, etc., things involving sizeable stackallocs. Once https://github.com/dotnet/csharplang/blob/master/proposals/skip-localsinit.md is available in the compiler, we can use that instead of the linker, at least for the locals zero'ing. |
|
Dumb question about trimming dead code - now that we’ve brought back all the APIs in netstandard2.0, can’t we just delete any remaining dead code? Or at least #if it out for .NET Core? |
For the most part, yes, and it's in progress: |
|
Yes, ILLink is currently used only for trimming code and clearing initlocals. Note that some dead things can't be removed from source but can be removed by ILLink: e.g., inlined constants, private constructors. ILLink is also a place for any other future IL-to_IL transformations that either can't be done by Roslyn or can be done by Roslyn but may benefit from first testing them in ILLink (like we did with clearing initlocals). Having ILLink in the corefx and coreclr pipelines is convenient for that. @sbomer and I have been building ILLink.Tasks from source (including Cecil) and publishing the packages. We can help with integrating that into source-build if that is what is required. Is source build not allowed to use any packages? |
With the BuildTools and CoreCLR update, the package not existing made CoreCLR fail, and the priority was getting CoreCLR working. I didn't get a deep understanding of why it wasn't needed before and was after. |
dseefeld
added
triaged
area-prebuilts
|
My understanding is we're at a weird middle ground because I didn't know CoreFX had it disabled: CoreCLR uses it, but CoreFX doesn't. We should be able to disable it in CoreCLR as well and remove it again, with the perf cost stephentoub mentioned. (I don't know the build arg to do this.) If it's ok to have it as a prebuilt, we could enable it for CoreFX so that we're at least using what we already have. |
|
The performance cost to disable it in CoreCLR is prohibitive. It will cause performance regressions that are not going to be acceptable. (Unless we pick up the compiler with SkipLocalsInitAttribute and start using this feature.) |
Unless there are alternative ways to avoid the perf impact, that is what we need to do to get rid of the prebuilt dependency, yeah. Thanks for the offer. Here's a doc on the basics of the source-build repo if you want to see the general expectations ahead of time--it doesn't seem to me like we have bandwidth to get that in our imminent releases though: https://github.com/dotnet/source-build/blob/master/Documentation/2.1-update-process.md. /cc @dleeapho
Check out https://github.com/dotnet/source-build#goals for why we don't want prebuilt (downloaded) dependencies in source-build. This guideline in particular: https://fedoraproject.org/wiki/Packaging:Guidelines#No_inclusion_of_pre-built_binaries_or_libraries. The "area-prebuilt-reduction" label tracks exceptions we're aiming to get rid of.
Yep, just source-build. |
By what release could we change this? 2.1.2? |
|
@dagood Is this guideline about what is included in the resulting binaries or about everything used during the build? |
|
@jkotas We can bring back your workaround in the runtime to ignore initlocals in SPC if the linker has to be disabled... We'll still have regressions in corefx assemblies. |
@erozenfeld For the native toolchain, this is the case (since that comes from the OS packages, which meet source-build requirements). For the C# compiler, this is not yet the case. For this reason, the .NET Core packages are not shipping as part of the Fedora regular repositories (since we don't meet the guideline). To solve this, we need to either use a compiler from the previous release (since that was source-build), or we need to do a two-stage build. From the doc:
|
|
@erozenfeld which of the options mentioned so far are feasible to do as part of a 2.1 service release? |
|
For either of those we'd first have to start building For scheduling, I think that's more on us in source-build since I don't think anyone's been able to add a repo yet without deep repo knowledge. @dleeapho ? |
|
@dleeapho can you please look into this? |
dleeapho
added
the
area-product-experience
|
Per our discussion offline, @dagood can you investigate how we can get this in for 2.1.x? |
|
Will do, I put it at the top of my backlog. |
|
I had some time to look at this and got it building and consumed by CoreCLR, but the task makes the MSBuild node exit:
I'm using Docker image @erozenfeld @sbomer, do you have any suggestions on figuring out what's going wrong? Even better, could you look into this? (Should be easy to repro.) Adding this repo didn't bring in any new prebuilts that look like an issue. It looks like they just need to get on the right versions to use source-built stuff: |
|
@dagood very weird, not sure what that error is. |
|
Ah, sorry, it's on my fork, forgot the commit screen doesn't mention that. https://github.com/dagood/source-build/tree/add-illink/2.1 |
|
The pseudo-command CoreCLR runs is this:
I decompiled illink.dll from the package CoreCLR uses in official builds (https://dotnet.myget.org/feed/dotnet-buildtools/package/nuget/ILLink.Tasks/0.1.5-preview-1461378) vs. the source we're building, and I see this in the diff (among other changes): --- "a/.\\sb\\proj\\Mono.Linker\\Driver.cs"
+++ "b/.\\official\\proj\\Mono.Linker\\Driver.cs"
@@ -66,7 +66,6 @@ namespace Mono.Linker
Console.WriteLine(" -g Generate a new unique guid for each linked module (true or false)");
- Console.WriteLine(" -v Keep members needed by debugger (true or false)");
+ Console.WriteLine(" -v Keep memebers needed by debugger attributes (true or false)");
+ Console.WriteLine(" -h List of reflection heuristics separated with a comma.");
+ Console.WriteLine(" Supported heuristics:");
+ Console.WriteLine(" LdtokenTypeMethods: mark all methods of types whose token is used");
+ Console.WriteLine(" in an ldtoken instruction");
+ Console.WriteLine(" LdtokenTypeFields: mark all fields of types whose token is used");
+ Console.WriteLine(" in an ldtoken instruction");
+ Console.WriteLine(" InstanceConstructors: mark all instance constructors in types");
+ Console.WriteLine(" where an instance member has been marked but");
+ Console.WriteLine(" none of the instance constructors have been marked");
Console.WriteLine(" -l List of i18n assemblies to copy to the output directory");
Console.WriteLine(" separated with a comma: none,all,cjk,mideast,other,rare,west");The Console.WriteLine might not be making it to any logs and the I did a search and found this closed PR: dotnet/linker#60. I'm guessing the version of ILLink.Tasks used by CoreCLR official builds isn't the same as the linker repo's |
|
I read through the PR (my bad) and saw this:
@erozenfeld, where is the other version, so we can include it in source-build? |
|
Created PR #778 to add the linker repo with a patch to add the The builds of ILLink.Tasks currently come from a non-public repo (@sbomer and @erozenfeld helped me find this, thanks!) and I was able to make a patch for the feature we need. I expect the patch not to require much maintenance: there are only a handful of ILLink.Tasks package builds total. However, if it becomes a problem, we will start a dotnet/linker fork of mono/linker where the changes will be maintained. The internal repo doesn't have any other notable changes: just small things to support the signed Microsoft build infrastructure. If we make the fork, I expect those changes would be made public without any problems, to make sure maintenance can happen in just one place. |
|
Closing as resolved: the linker repo is in the build and the change has flowed through all branches. |
|
Thank you @dagood! |
Today, we turn off ILLink in corefx
source-build/repos/corefx.proj
Line 6 in ac7159e
But we are now dependent on ILLink with #376.
We should either not use ILLink at all. Or we should embrace it everywhere and figure out how to build it from source (it depends on Mono.Cecil), and then use that source-built version to build .NET Core.
/cc @weshaggard @dagood @dseefeld
The text was updated successfully, but these errors were encountered: