Support for multi-folder composite builds #37130

trylek · 2020-05-28T18:09:49Z

This change add support for Crossgen2 input files residing in a
directory structure; the same directory structure gets created
under the output path and the output composite image sits at its
root. The component R2R headers store reverse relative paths to
the composite image, e.g. for a composite image "composite-r2r.dll"
the standalone MSIL file FolderA/FolderA.dll would have
"../composite-r2r.dll" in the OwnerCompositeImage section of its
R2R header.

To avoid having to implement homebrew path normalization and to
make the entire mechanism more robust, I dropped the composite image
name comparisons. Instead we just construct the path and open the
composite image. In case of multiple components the OS guarantees
unification of the openend DLL's on the same path.

We can also use this mechanism for AssemblyLoadContext checking.
When we load a component assembly into two different ALC's, for the
second ALC (along the execution timeline) we cannot reuse the
composite image that got loaded into the first ALC and we cannot
load it a second time so we just pretend there's no image.

Manish suggested we might want some diagnostics that would crash
instead of silently returning null when loading a component
assembly into a second ALC with the rationale that using composite
images is a conscious decision and we don't want to lose perf
just because we accidentally loaded an assembly into two different
ALC's. I'll welcome thoughs on how to implement this - using
a COMPlus variable, perhaps?

Thanks

Tomas

/cc: @dotnet/crossgen-contrib

jkotas · 2020-05-28T18:28:33Z

src/coreclr/src/vm/nativeimage.cpp

-    LoaderAllocator *pLoaderAllocator,
-    AllocMemTracker *pamTracker)
+    AssemblyLoadContext *pAssemblyLoadContext,
+    LoaderAllocator *pLoaderAllocator)
 {
    STANDARD_VM_CONTRACT;

    NewHolder<PEImageLayout> peLoadedImage = PEImageLayout::LoadNative(fullPath);


Does this work on non-Windows? I do not think that PEImageLayout::LoadNative will keep returning the same address for multiple loads of the same image.

Resolved in 2nd commit by changing the dictionary to be keyed by simple file names of the composite images. This is also related to DavidWr's feedback - I have reverted encoding the reverse relative paths in the OwnerCompositeExecutable R2R sections of the component assemblies and instead I added a new COMPlus variable NativeImageSearchPaths to let the runtime locate the native image in an arbitrary location (in addition to the containing folder of the MSIL assembly).

jkotas · 2020-05-28T18:31:21Z

src/coreclr/src/vm/nativeimage.cpp

-    return image.Extract();
+    NewHolder<NativeImage> image = new NativeImage(pAssemblyLoadContext, peLoadedImage.Extract(), nativeImageFileName);
+    image->Initialize(pHeader, pLoaderAllocator);
+    pExistingImage = AppDomain::GetCurrentDomain()->SetNativeImage(peLoadedImage->GetBase(), image);


The regular R2R images use READYTORUN_HELPER_Module fixup to claim the image. Look for AcquireImage in readytoruninfo.cpp. Would be make sense to use the same scheme here instead of maintaining a hashtable?

Hmm, I'm afraid that, according to your other comment, we still need some table on Linux to cater for the fact that repeated dll loads allocate separate instances. In combination with David's suggestion it seems to me that it would be easiest to keep the dictionary in AppDomain (to let us detect and fail fast upon an attempt to load the composite DLL into a secondary ALC on Linux), just key it off the composite image simple name and apply the path search when opening the dll for the first time.

Resolved in 2nd commit as described in my other comment.

jkotas · 2020-05-28T18:32:32Z

relative paths

Do we have confirmation that the relative paths are really required to make the workload work?

trylek · 2020-05-28T20:03:59Z

Based on our recent discussions with partner teams I got the impression that, at least short-term, it can speed up our related perf and other work if we allow the folder hierarchy. Thanks for pointing out the Module fixup in AcquireImage, that's a great idea, I'll do that. I'll look into the executable image unification on Unix, thanks for pointing that out.

davidwrighton · 2020-06-01T18:17:09Z

I think we should do something much simpler.

Add a COMPLUS variable that provides a path to look up native image files. So the workflow would go from:

Look in the directory of the PE file that references a native image for a native image with name X.

to

Look in the directory of the PE file that references a native image for a native image with name X.
Iterate through the paths specified in COMPLUS_CompositeImagePath, attempting to find the right PE file.

This has the side benefit of if we tweak native image generation to generate a naturally 2MB aligned PE we could load the native image using huge pages out of the hugetlbfs on Linux.

trylek · 2020-06-01T18:27:10Z

Thanks @davidwrighton, that sounds reasonable; I can easily modify the change as you suggest. For the remaining problem regarding multiple ALC's, that would be probably solvable via AcquireImage as @jkotas proposed.

davidwrighton · 2020-06-01T18:32:27Z

In my opinion multiple ALCs simply should not be supported from one composite image file. That should be a hard fail, and we should not attempt to make it work.

trylek · 2020-06-01T18:32:46Z

One other aspect is the Crossgen2 part of the change - the CompositeRootPath switch is needed if we want to keep the functionality of producing native compiled dll's into a set of output folders matching the input tree. @nattress, do we know whether the module dll's are loading some side data that needs to be located in the folders next to them?

nattress · 2020-06-01T20:42:28Z

There is side-data in each package folder. It seems to all be loaded by the main engine and looked up by string and resolved centrally. I suspect with the current implementation we'll need this ability to generate the composite-finder DLLs into a matching output tree.

trylek · 2020-06-02T22:44:11Z

@davidwrighton - I believe I have addressed the gist of your feedback by reverting the relative path manipulation related to locating the composite image and by introducing a new COMPlus_NativeImageSearchPaths variable for this purpose per your suggestion. I have not yet made the multiple ALC issue a hard error (as of the 2nd commit, NativeImage::Open will gracefully bail out pretending there's no native image at all) but I'm open to suggestions how to harden this, @mangod9 also suggested we shouldn't silently fall back to JIT in this case. Could you please take another look when you have a chance?

nattress · 2020-06-03T21:47:37Z

src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunCodegenCompilation.cs

@@ -270,6 +274,8 @@ public sealed class ReadyToRunCodegenCompilation : Compilation
            _dependencyGraph.AddRoot(new Import(NodeFactory.EagerImports, instructionSetSupportSig), "Baseline instruction set support");
        }

+        private readonly static string s_folderUpPrefix = ".." + Path.DirectorySeparatorChar;


Will this work on Unix? Should we choose DirectorySeparatorChar based on our target platform?

Actually, I think you can disregard this. I was thinking about the relative path we embed in the non-composite images.

The Path.DirectorySeparatorChar is platform specific.

Yeah I was more worried about future cross-platform bug if we generate a Linux composite image from Windows. The change description indicates we burn relative paths in the component assemblies to the composite image but it seems like we actually just put the composite image name and look it up by simple name instead. If that's the case, then we should be fine.

The first version of my change used reverse relative paths to go from the component assemblies to the composite native image. Based on David's PR feedback I simplified this logic by throwing away the relative path magic and just allowing explicit composite image path lookup which seems more general for additional reasons.

Sigh, that's what I get for reviewing in a 2 day old browser tab 🙄 Thanks for clarifying

nattress

LGTM 👍

davidwrighton

With the move to the search path approach for finding the composite image, I think this change is good. I'm not entirely convinced that the approach of building a directory tree of output is great, but I'm willing to see if it improves the workflow.

davidwrighton · 2020-06-08T22:29:03Z

src/coreclr/src/vm/nativeimage.cpp

    m_pImageLayout = pImageLayout;
    m_fileName = imageFileName;
    m_eagerFixupsHaveRun = false;
 }

-void NativeImage::Initialize(READYTORUN_HEADER *pHeader, LoaderAllocator *pLoaderAllocator, AllocMemTracker *pamTracker)
+void NativeImage::Initialize(READYTORUN_HEADER *pHeader, LoaderAllocator *pLoaderAllocator)


Why did you remove the memory tracking here? It opens up a possibility for a memory leak if there are racing loads of the native image in NativeImage::Open

Hmm, you're obviously right, I have apparently messed this up in my third refactoring of the native image dictionary during the review, will fix.

Fixed in 5th commit.

This change add support for Crossgen2 input files residing in a directory structure; the same directory structure gets created under the output path and the output composite image sits at its root. The component R2R headers store reverse relative paths to the composite image, e.g. for a composite image "composite-r2r.dll" the standalone MSIL file FolderA/FolderA.dll would have "../composite-r2r.dll" in the OwnerCompositeImage section of its R2R header. To avoid having to implement homebrew path normalization and to make the entire mechanism more robust, I dropped the composite image name comparisons. Instead we just construct the path and open the composite image. In case of multiple components the OS guarantees unification of the openend DLL's on the same path. We can also use this mechanism for AssemblyLoadContext checking. When we load a component assembly into two different ALC's, for the second ALC (along the execution timeline) we cannot reuse the composite image that got loaded into the first ALC and we cannot load it a second time so we just pretend there's no image. Manish suggested we might want some diagnostics that would crash instead of silently returning null when loading a component assembly into a second ALC with the rationale that using composite images is a conscious decision and we don't want to lose perf just because we accidentally loaded an assembly into two different ALC's. I'll welcome thoughs on how to implement this - using a COMPlus variable, perhaps? Thanks Tomas

I have temporarily disabled the new multifolder test with an issue on Windows ARM64 to unblock merging this change and using it for X64 development. Thanks Tomas

trylek added the area-crossgen2-coreclr label May 28, 2020

trylek requested review from nattress, jkotas, janvorli and davidwrighton May 28, 2020 18:09

jkotas reviewed May 28, 2020

View reviewed changes

trylek force-pushed the CompositeMultipleFolders branch from c073ba7 to c7f2119 Compare June 2, 2020 22:32

nattress reviewed Jun 3, 2020

View reviewed changes

nattress approved these changes Jun 3, 2020

View reviewed changes

davidwrighton approved these changes Jun 4, 2020

View reviewed changes

davidwrighton reviewed Jun 8, 2020

View reviewed changes

trylek force-pushed the CompositeMultipleFolders branch from 0561844 to e48f27e Compare June 8, 2020 23:08

trylek closed this Jun 9, 2020

trylek reopened this Jun 9, 2020

trylek force-pushed the CompositeMultipleFolders branch from 2af8710 to aeb2004 Compare June 10, 2020 20:53

nattress mentioned this pull request Jun 10, 2020

R2RTest - compile-serp large composite images #37722

Merged

trylek force-pushed the CompositeMultipleFolders branch from d74f7c0 to 938511b Compare June 11, 2020 14:05

trylek added 4 commits June 16, 2020 21:04

Address David's PR feedback

8e54554

Fix wide character constant on Unix

b9fce7d

Fix output path on Unix

e33663a

trylek added 10 commits June 16, 2020 21:04

Fix NativeImage memory leak per David's PR feedback

219da7d

Fix propagation of target architecture in the multifolder test

f87bae0

Fix Crossgen2 target architecture for the multifolder test

e3b31fb

Instrumentation for investigating the Windows arm64 failure

c14eb59

Fix incorrect conflict resolution

76cc528

More instrumentation

734458a

More instrumentation

958f453

Fix instrumentation

0158a15

Still trying to make the instrumentation work

70ca89b

Remove instrumentation; disable the multifolder test on arm64

92778e3

I have temporarily disabled the new multifolder test with an issue on Windows ARM64 to unblock merging this change and using it for X64 development. Thanks Tomas

trylek force-pushed the CompositeMultipleFolders branch from f0549a5 to 92778e3 Compare June 16, 2020 19:16

Fix exclusion clause in issues.targets

2949446

trylek force-pushed the CompositeMultipleFolders branch from d909ca5 to 2949446 Compare June 17, 2020 13:04

trylek merged commit 4a22294 into dotnet:master Jun 17, 2020

trylek deleted the CompositeMultipleFolders branch June 17, 2020 22:32

ghost locked as resolved and limited conversation to collaborators Dec 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for multi-folder composite builds #37130

Support for multi-folder composite builds #37130

trylek commented May 28, 2020

jkotas May 28, 2020

trylek Jun 2, 2020

jkotas May 28, 2020

trylek Jun 1, 2020

trylek Jun 2, 2020

jkotas commented May 28, 2020

trylek commented May 28, 2020

davidwrighton commented Jun 1, 2020

trylek commented Jun 1, 2020

davidwrighton commented Jun 1, 2020

trylek commented Jun 1, 2020

nattress commented Jun 1, 2020

trylek commented Jun 2, 2020

nattress Jun 3, 2020

nattress Jun 3, 2020

janvorli Jun 3, 2020

nattress Jun 3, 2020

trylek Jun 3, 2020

nattress Jun 3, 2020

nattress left a comment

davidwrighton left a comment

davidwrighton Jun 8, 2020

trylek Jun 8, 2020

trylek Jun 8, 2020

Support for multi-folder composite builds #37130

Support for multi-folder composite builds #37130

Conversation

trylek commented May 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented May 28, 2020

trylek commented May 28, 2020

davidwrighton commented Jun 1, 2020

trylek commented Jun 1, 2020

davidwrighton commented Jun 1, 2020

trylek commented Jun 1, 2020

nattress commented Jun 1, 2020

trylek commented Jun 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nattress left a comment

Choose a reason for hiding this comment

davidwrighton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment