COMPlus_PartialNGen is not being set since repo consolidation

[Anipik]

we are setting this flag _partialCrossgenFlag for some assemblies here https://github.com/dotnet/arcade/blob/master/src/Microsoft.DotNet.Build.Tasks.SharedFramework.Sdk/targets/framework.dependency.targets#L427-L431

Before this change these lines of code was never been executed dotnet/arcade#4911 becuase $(OSGroup) can never be linux in installer. we either set it to unix or osx https://github.com/dotnet/runtime/blob/master/src/installer/Directory.Build.props#L63

The failure due to this flag being set can be found in #32858

the failure message is

Error compiling /root/runtime/artifacts/bin/pkg/netcoreapp5.0/runtime/Linux-Debug-x64/System.ComponentModel.TypeConverter.dll: Cannot compile using the PartialNgen flag because no IBC data was found. (0x8013200C)

I checked the flag value and its being set to 1

This was introduced in dotnet/core-setup#5252
@MichalStrehovsky can you take a look and help me fix this ?

cc @ericstj @dagood @ViktorHofer @safern

[MichalStrehovsky]

This is either because CoreFX stopped embedding IBC data again like in dotnet/corefx#38343 (comment) (when Release builds of CoreFX were actually Debug builds), or because the optimization infra is not generating the IBC data for CoreFX to embed.

Cc @brianrob who own the area.

[Anipik]

@brianrob any pointers here to fix this ?

[Anipik]

cc @ericstj

[brianrob]

It looks like we are publishing IBC data for System.ComponentModel.TypeConverter.dll so this implies that the IBC data isn't being embedded into the IL image before the call to crossgen occurs. Note that this only happens for official builds so if you try this on your dev box you'll never see the IBC data.

I would recommend looking for the call to ibcmerge.exe in build logs and make sure that this is succeeding for System.ComponentModel.TypeConverter.dll.

[ericstj]

I believe @Anipik is seeing the failure on debug build in CI which would explain why the IBC data is missing.

I checked the binaries from a recent official build and confirm that IBC data is present:

It did get an OSGroup value of Linux when it was in core-setup, and the binaries it got from the corefx transport package would have always had IBC data (since they were release binaries)
dotnet/core-setup@0d80fe3

Regardless the fix would be to condition that code. I think we need to match these conditions:

runtime/eng/Tools.props

Line 46 in 2458930

<Import Project="$(RepositoryEngineeringDir)restore\optimizationData.targets" Condition="'$(DotNetBuildFromSource)' != 'true' and '$(EnableNgenOptimization)' == 'true'" />

@brianrob as far as I can tell it might just be a release/debug difference. I can't find anywhere else we condition this to be disabled on official vs not. To be sure, try a release build on Linux and make sure it is successful

Separately: @dagood do we really want library specific configurations to exist in Arcade? Seems to me this should live in the shared framework project. This is leaking repo-specific state into shared tooling: https://github.com/dotnet/arcade/blob/4d9720ccb07cc01068c2f7c19cb2ded64c44b1a1/src/Microsoft.DotNet.Build.Tasks.SharedFramework.Sdk/targets/framework.dependency.targets#L427-L431

[brianrob]

Historically, the conditioning has been on whether or not ibcmerge.exe is present, which only happens on release official builds since it is not publicly available.

[Anipik]

@ericstj I tried with the release flag still the same error

Error compiling /runtime/artifacts/bin/pkg/netcoreapp5.0/runtime/Linux-Release-x64/System.ComponentModel.TypeConverter.dll: Cannot compile using the PartialNgen flag because no IBC data was found. (0x8013200C)

[ericstj]

I see, we are probably missing another condition then as @brianrob suggests.

[ericstj]

Found it: https://github.com/dotnet/arcade/blob/f6121fcaf62777bc2288f7f0917b48a130a0174e/src/Microsoft.DotNet.Arcade.Sdk/tools/OptimizationData.targets#L59

[ericstj]

@brianrob I don't like copying a bunch of this conditional logic or running the targets. Is there a way for these installer targets to "observe" that IBCMerge was run and only then specify the param? Or for crossgen to ignore the option if the binary doesn't have IBC merge data?

[brianrob]

We specifically added this error message because release builds were making it out into the wild without IBC data, and thus partial NGEN resulted in no pre-compilation. I wouldn't want to go back to a world where that's a possibility. Without having dug too much into the targets here, I'm not sure there's a great answer here. The invariant we want to hold true is that we always call crossgen -partialNGEN for assemblies that we should partial NGEN and if that fails due to missing IBC data, then that's a bug we need to fix.

[ericstj]

So you want to test that all binaries have IBC data by failing if 3 of them are missing IBC data? I understand you'd want test coverage, but relying on this as a side-effect of cross-gen commandline seems a little busted.

Regardless, if you want this stuff in the installer targets to match what is done in the optimatizationdata targets you ought to agree upon a contract to communicate that rather than rely on folks getting both conditions to match. @dagood / @MichalStrehovsky can you help make these consistent?

[Anipik]

I took a look at the official build and it doesn't seem like we are hitting this code on the official build as well. Not sure which target is responsible for embedding the ibc data to dlls

[ericstj]

This target does: https://github.com/dotnet/arcade/blob/f6121fcaf62777bc2288f7f0917b48a130a0174e/src/Microsoft.DotNet.Arcade.Sdk/tools/OptimizationData.targets#L174-L177

And just to reiterate @Anipik's findings. We have been getting IBCData in official builds, but crossgen has not been getting the COMPlus_PartialNGen setting (and likely never was since the repo consolidation). So we can let this issue track that COMPlus_PartialNGen is never being set correctly. @Anipik unblocked the builds by adding SetPartialCrossgenFlag to disable the dotnet/arcade#4950, but this needs a closer look by @MichalStrehovsky and @dagood to determine how and when they'd like to best set this.

[brianrob]

@ericstj the reason that we do this in crossgen is because that's the natural place to detect whether or not specifying -partialNGEN is going to have the desired behavior. From a crossgen usage perspective, you would never want to specify -partialNGEN if there isn't IBC data embedded in the input IL image, as that would mean "please don't precompile this DLL", even though that's exactly what you're attempting to do. The key here is that we want a uniform way to make sure that no one ever specifies -partialNGEN on an IL image that has no IBC data.

[ericstj]

Changed the title of the issue. This needs to track re-enabling the COMPlus_PartialNGen option. That appears to have been broken since repo consolidation. It needs to be brought back with "awareness" of whether the rest of the build actually embedded IBC data. It didn't need this before because it was consuming transport packages in core-setup (which were always "official" "release" packages with IBC data).

[ViktorHofer]

@MichalStrehovsky @brianrob I assume this is required for 5.0? Is someone actively working on this?

[MichalStrehovsky]

Cc @mangod9 for IBC.

[davidwrighton]

@billwert @MichaelSimons has a point, does the perf team have a statement here?

[ViktorHofer]

Note to myself, talk with @brianrob about priority. We need to understand if this is needed for 5.0 which closes very soon.

[mangod9]

Based on the discussion above, my recommendation is to move to 6.0

[richlander]

This is "nice to have". We have lots to do in this space for 6.0. I agree with @mangod9 that we should move this to 6.0 at this point.

[hoyosjs]

@ViktorHofer @brianrob looks like this one flew under the radar once more. Is this something that should still be considered for 6.0?

[richlander]

Thanks for raising this!

We've been talking about this as part of the crossgen crew. I've been doing some perf testing for crossgen with this as one of the variables to consider. I'll have more to share soon.

[ViktorHofer]

Let's keep this onto 6.0 until we hear back from Richard.

[richlander]

I did some more analysis of the data I was collecting. This will be important for .NET 7 but I think we can consider the ship sailed on this for .NET 6.

[hoyosjs]

@mangod9 @brianrob @dotnet/crossgen-contrib is partial ngen still a concern? Deciding if this is an 8.0 issue, or if it should be closed.

[mangod9]

partialNGen is not a thing currently. I think this issue was capturing the fact that pgo was not being used in the runtime repro, which has now been fixed. Not sure if this needs to be tracked anymore?

[hoyosjs]

PGO doesn't consume IBC data in managed images (unless you mean dotnet-pgo. But that's newer than this issue).

[brianrob]

At a minimum, this issue can be moved to .NET 8. It is also possible that it can be closed - we should understand the scenario requirements around whether we want to support anything like partial NGEN on R2R. Historically, it was used to address VM constraints and to a lesser degree, size constraints. It's not clear that we have a strong scenario where this is required.

[mangod9]

we will be looking into supporting partial R2R scenario for the composite scenario, but will track it as a separate user story.

[agocke]

Closing as no longer relevant.