Initial Crossgen2 changes for composite R2R support

[trylek]

This change adds the new command-line option "--composite",
a new type of input files "--unrooted-input-file" for files that should
be made part of the composite image but only rooted as hit by the
dependency analysis, not automatically as the normal input files
(to reduce the size of framework built along with the app) and new
Crossgen2 logic for producing the additional R2R metadata needed
by composite images including rewriting of input MSIL by injecting
a "component R2R header" on them that forwards the native code
to the composite image. With a set of runtime changes out for
a separate PR I'm able to make first steps in running composite R2R
in the CoreCLR runtime.

Thanks

Tomas

[davidwrighton]

I'd like us to have a discussion about how we expect arguments to be passed to the composite generator before we go down this route.

[davidwrighton]

@trylek
About your points..
2) 10 minutes to compile 142 MB of code is ridiculous. I think we need to look at that, as that is the build mode we'll need for some of our hyperscale customers.
3) As noted in my review, please file an issue.
4) Could you please contact someone on the infrastructure or SDK side of the world? We're going to start having previews for .NET 5 in the not too distant future, and I'd like to make sure this is fixed when that happens.
5) Resources are a very interesting conundrum which we haven't explored in detail. Some resources could be merged, but most can't and it doesn't make sense. We may need to add some sort of scheme for recording fileversion data into the R2R format so that we can as developers understand what binaries are going into the merged binary.

[trylek]

@davidwrighton - thanks a lot for your immediate feedback, here are my initial thoughts:

2. 10 minutes vs. 142 MB - I'll take a closer look what happens there. I've mostly been concentrating on functionality as opposed to performance right now so I haven't spun up any perf runs or instrumented experiments. I suspect that even the default PerfView analysis can provide more data as to what's the longest pole.

3. Filed Merge multiple IBC data readings for the same MethodDesc #31687;

4. Started an e-mail thread about this with Jeff and Tom;

5. Hopefully not blocking initially but something worth thinking about in more detail.

Thanks

Tomas

[davidwrighton]

@trylek, I don't think you need to address the time to compile issue as part of this checkin, but that's a concern as we build out this feature. (Although if its a really obvious fix, feel free to stuff it in this changeset) I'm more interested to understand why you were merging all of the reference set into the input set in the general sense. That didn't make sense to me, and I'd like to understand your thinking there.

[trylek]

@davidwrighton - for the 10 minutes compilation time, admittedly this is in debug mode I'm mostly using these days to debug the compiler; it's just 6 minutes when Crossgen2 itself is built in release mode and presumably even less with a release build of JIT so it's probably reasonable to leave more precise measurements for later.

For adding references to inputs, I guess that was more of an experiment rather than having any deep design foundations on my part - I've mostly been thinking that for typical apps we'd probably want something like this (only rooting the app and only compiling those parts of the framework found by the dependency analysis) rather than always carrying around the dead weight of the complete framework so I just tried that as that also lets me minimize the counterpart SuperIlc changes.

Ultimately, I tend to think that we should probably have two flavors of reference options in Crossgen2 to refer to "assemblies to be compiled into the composite file but without automatic rooting we're doing for input files" vs. "assemblies external to the composite file" (that will come into play once we start implementing layered composite files). At that moment we'll also need to figure out what to do about Crossgen2 options for actually compiling the framework - right now I'm not clear on whether in such case we should also root everything or for instance just the public API surface.

[trylek]

FYI, during my work on the counterpart R2RDump changes for the composite format I realized an interesting open question regarding import cells. I believe there are basically three ways to adjust import cells to fit the composite format and I'm interested in opinions on the preferable one:

1. The current working version randomly collects all import cells. This means they are heterogeneous - their signatures are encoded relative to different caller modules. Import cells are potentially duplicated even though actual local methods aren't - two different submodules of the composite R2R generally need two different encodings of a given target.

2. Keep module-relative import cells but separate them into per-module import sections. This is easier for R2RDump and perhaps even for data access locality; there's still duplication in encoding the same import cell relative to two different modules.

3. Change the runtime signature behavior for composite R2R to avoid import cell duplication, either by enforcing the presence of module override on each signature or by arbitrarily choosing a "root" module and encoding / decoding all signatures within the composite file relative to this root module.

Choice of the root module doesn't matter - manifest metadata covers the entire version bubble of the composite image so that we can encode signatures relative to any of the modules present, a smart choice might just yield a minor size optimization in making some of the signatures a byte or two shorter.

[jkotas]

3 - "Change the runtime signature behavior for composite R2R to avoid import cell duplication" looks like the best choice.

[trylek]

Thanks Jan, I agree with you, I have started implementing option 3. In the meantime I have found another funny thing: in my current implementation I'm using the manifest metadata to represent the assemblies within the composite R2R file. After implementing export table directory support in R2RDump and becoming able to parse RTR_HEADER, I've immediately hit a new issue: as we're using AssemblyRef entries to represent the reference assemblies, this naturally doesn't work when the assembly name differs from the module name. I hit this just by lucky chance as I'm still using the regression test b426654 for the initial experiments and that's exactly an IL-written test where the module is named "b426654" but the assembly name is "cs1". I'm wondering whether we should use ModuleRefs instead in the composite case.

[jkotas]

We do not have multimodule assemblies in .NET Core. Why does this matter?

[trylek]

Well, it matters at least for CoreCLR tests that involve IL assemblies with hand-written assembly vs. module names. We may decide to not care about them but I'm worried it will make our life harder w.r.t. testing and it seems to me like an overal debt in the implementation.

[jkotas]

Ok, but how does it matter for them? I am not following.

[jkotas]

The assembly name should be used for both assembly ref and assembly def. The filename should not need to be stored anywhere.

[trylek]

OK, you're probably right it's merely an annoyance for R2RDump, not for the runtime as such, it just sucks that we cannot use the manifest metadata to easily identify the appropriate standalone MSIL files corresponding to the composite R2R components. For the "b426654" test the main app module entry in the manifest metadata is "cs1" so I'll need to try to open all managed modules in all reference paths looking for the one "r426654" that happens to contain the assembly "cs1".

[jkotas]

I'll need to try to open all managed modules in all reference paths looking for the one "r426654" that happens to contain the assembly "cs1".

I do not think you need to do that. You can just declare this case unsupported. There are number of places in the .net tooling that assume assembly name == file name.

[trylek]

OK, that sounds reasonable or at least reasonably simplifying for now. Thanks Jan for sharing your detailed feedback!

[trylek]

OK, with the latest changes I seem to be getting consistent R2RDump info for composite images (except for the EHClauses that are currently commented out due to a design bit I have yet to clarify). I believe I'm ready to start working on the relevant runtime changes but I'd prefer to review and merge in the existing changes separately as the change is already big. I'll start out by sending out a separate review for the R2RDump parts of this change that are relatively straightforward. I'll follow up by preparing separate PR's for SuperIlc and Crossgen2 changes after applying @davidwrighton's feedback towards my "non-rooting input" suggestion.

[nattress]

Nit: Make this an assert and have the real check when we're setting up compilation and can write an error out to the user?

[trylek]

I have changed the LINQ expression to use Single() to throw when the number of elements is not 1.

[nattress]

I like this method! - it shows how flexible our system is to add a thing to a module and save it in ~20 lines of code.

[trylek]

Me too :-).

[nattress]

LGTM - I made a couple nits about comments

[trylek]

Thanks Simon for your feedback. In the latest (15th) commit I have addressed "conservative" parts of your PR feedback like improved comments and minor code reorganizations. I haven't yet addressed PE header feedback including DavidWr's feedback. I'm not sure how to approach the deterministic ID thing, it looks like a new functionality we don't have any support for in the compiler just yet.

My current plan is to just play with the runtime over the weekend and hopefully get to the point I'm able to actually call [non-generic] methods in the composite R2R executable. Ideally early next week I should be able to review and merge in the initial support so that we can fan out further work like shared framework support, diagnostic support, additional performance tuning knobs etc.

[jkotas]

it looks like a new functionality we don't have any support for in the compiler just yet.

So open a new issue on it and address it in separate PR.