Arcade: Generalize codeOptimization targets

[adiaaida]

The targets in codeOptimization should be generalized for use by other repositories than corefx. Currently, it stores versions and nuget package names/feeds that are specific to corefx and also directly accesses files from the PackagesDir, which makes the targets unusable by any other repository, forcing us to duplicate code in other repositories to do the same work, so we should fix it so it can be used by at least also coreclr, if not other repositories for which we may collect IBC data in the future.

[tmat]

Arcade SDK already has support for IBCMerge.
https://github.com/dotnet/arcade/blob/master/src/Microsoft.DotNet.Arcade.Sdk/tools/OptimizationData.targets

Is it sufficient for CoreFX?

[maririos]

FYI @ViktorHofer and @adiaaida

[adiaaida]

This seems to be available vi the OptimizationData.targets in Arcade. Closing.

[ViktorHofer]

@ericstj does that mean we can remove the buildtools one in corefx?

[ericstj]

@tmat it looks like this is expecting *.IBC files but you're providing PGOs from roslyn in https://dotnet.myget.org/feed/roslyn/package/nuget/RoslynDependencies.OptimizationData.

To close this issue I'd expect some documentation that describes how to use those targets along with packages which match the conventions expected by the targets (or issues tracking the production of such packages). As this is I think there are still missing pieces.

[tmat]

The data are no longer read from the nuget package. Roslyn fetches them from Azure DevOps drop, but you can download them from anywhere and put them to $(IbcOptimizationDataDir).

We are still working on the system and will move some of the infra to Arcade since it is gonna be needed by multiple repos that insert into VS. In any case, OptimizationData.targets will remain agnostic to where the data came from and not tied to VS infrastructure in any way.

[ericstj]

@tmat you're talking about this: https://github.com/dotnet/roslyn/blob/8b2707b2ca2baa4e4a5bfa16be886a3d8ed0e848/eng/build.ps1#L269-L293

Is that what you recommend as the contract between roslyn and CoreFx for ingesting the optimization data? Why is that better than the nuget package used today? Seems like we have mechanisms for flowing nuget packages between repositories but this would venting some way to flow the metadata to construct that drop string through BAR (as well as some new code to download and stage the files from the drop).

The transport isn't what I was trying to highlight here. AFAIK the transport being a nuget package is working fine, and should continue to work. The problem is that nuget has only *.PGO's and not *.IBC files and these targets only support IBC files as input. Sorry if I'm missing something simple here.

[tmat]

@tmat you're talking about this: https://github.com/dotnet/roslyn/blob/8b2707b2ca2baa4e4a5bfa16be886a3d8ed0e848/eng/build.ps1#L269-L293

Yes.

There is no need for a contract between Roslyn and CoreFX. It used to be that we published optimization data for SCI/SRM and other CoreFX binaries VS depends on and expected CoreFX to pick them up. That's not the case anymore. When inserting SCI/SRM into VS we remove all optimization data that were in the official assemblies, embed our own optimization data (produced by VS lab), re-sign the assemblies and then insert them to VS. VS is thus shipping with CoreFX assemblies with different optimization data than what was produced by CoreFX build.

In theory CoreFX could use optimization data produced by VS lab. If that's valuable (and from what I hear from Brian I don't think it is) you can have the build download the raw IBC data from VS drop. This would be completely independent process from dependency flow in BAR.

The reason why we do not use NuGet package for distribution of IBC data is that we want to be able to run a new build that applies a new set of IBC data given a specific commit sha, i.e. without updating anything in the repo (such as nuget package version).

The purpose of the ApplyOptimizations target in Arcade is to be reusable by any repo that needs to embed optimization data that come from IBC training. The training produces raw IBC files. The target then merges them to the assemblies they were built from and then applies them to the new assemblies. The codeOptimization.targets in buildtools is doing the same - the input is raw IBC data.
https://github.com/dotnet/buildtools/blob/master/src/Microsoft.DotNet.Build.Tasks/PackageFiles/codeOptimization.targets#L33. It should be thus possible to replace this target with ApplyOptimizations.

[ericstj]

There is no need for a contract between Roslyn and CoreFX

I see. That was the missing part of this picture for me. The sole reason we run IBCMerge in CoreFx today is to merge in the data provided by roslyn. In the future I agree that we can follow the pattern you've established.

For now I'll be deleting the functionality in CoreFx that merges in roslyn IBC data.

Thanks!

[ericstj]

Correction: we also have IBC data coming from https://dotnet.myget.org/F/dotnet-core-optimization-data/api/v3/index.json but I believe I can plumb that through the arcade targets.

[adiaaida]

Just an FYI: we started pushing PGO and IBC packages to https://dotnetfeed.blob.core.windows.net/dotnet-core/index.json, so if you end up plumbing things through the arcade target, I would start pulling from there now. We are hoping to stop pushing packages to myget asap.

[tmat]

@adiaaida Do these packages contain raw IBC data and the original binaries that were used for the training?

[adiaaida]

@tmat They do. They are exactly the same packages that come out of myget now.

Basically, right now, we are creaking the nupkg, then uploading it to both myget and dotnetfeed, updating the versions repo, and updating BAR. So all scenarios for pulling down the packages should work, at the moment. Once coreclr and corefx move to pulling the packages from the new location and pulling the version with the new method, we will turn off myget + versions.