dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.16k stars 4.71k forks source link

Productize MSTAT files #85806

Open MichalStrehovsky opened 1 year ago

MichalStrehovsky commented 1 year ago

MSTAT has been useful to troubleshoot size. Several repos now parse MSTAT to track size:

Might be more I don't know about. It is likely more will follow.

Mstat is currently exposed as an undocumented switch. Should we formalize this more?

The "productization" questions are:

We can choose to not address all (or any) of these.

Cc @dotnet/ilc-contrib @eerhardt for thoughts/opinions

ghost commented 1 year ago

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas See info in area-owners.md if you want to be subscribed.

Issue Details
MSTAT has been useful to troubleshoot size. Several repos now parse MSTAT to track size: * .NET Performance repo * ASP.NET crank benchmarks repo * Npgsql Might be more I don't know about. It is likely more will follow. Mstat is currently exposed as an undocumented switch. Should we formalize this more? The "productization" questions are: - [ ] Document the format and way to parse it in a versionable manner - [ ] Have an "official" parser NuGet. Maybe on a similar plan as ILCompiler.Reflection.ReadyToRun which is a ReadyToRun parser exposed on a "best effort" basis (doesn't go through API review process and we may make breaking changes, but we try to avoid them) - [ ] Expose as an unprefixed/supported switch (this may include changing IlcGenerateMstatFile from a boolean true/false to something that forces one to specify file name - looking for the file currently involves a heuristic search in `\obj`) We can choose to not address all (or any) of these. Cc @dotnet/ilc-contrib @eerhardt for thoughts/opinions
Author: MichalStrehovsky
Assignees: -
Labels: `area-NativeAOT-coreclr`
Milestone: 8.0.0
vitek-karas commented 1 year ago

To me the priority order is:

Overall I think it's a good idea to have this, but I think we should be very intentional about us potentially breaking the format going forward. I don't think the value of this outweighs the complexity of maintaining strict backward compatibility around it.

eerhardt commented 1 year ago

I agree with @vitek-karas that having a dedicated parser library makes the most sense. I don't think it needs to go through API review or have the same restrictions as the normal src\libraries libraries. Maybe we don't even publish it to nuget.org to start - but instead just the daily feeds like https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8/nuget/v3/index.json. And direct people to that feed if they want to consume it.

Expose as an unprefixed/supported switch

Is our heuristic/rule that anything prefixed with Ilc is an "unsupported" switch? I don't think that is obvious to typical .NET developers. The general rule in MSBuild is to prefix unsupported switches with _. For example in https://learn.microsoft.com/en-us/archive/msdn-magazine/2009/february/msbuild-best-practices-for-creating-reliable-builds-part-1

When you create items and properties, the convention is to start the name for "internal" values with an underscore. Items and properties whose names do not start with an underscore indicate to users that they are free to override them.

MichalStrehovsky commented 1 year ago

I think the ILCompiler.Reflection.ReadyToRun model I mentioned in the top post would be the best to follow: it is a NuGet package that is in the same repo as the ReadyToRun format generator (so it's easy to keep it in sync), and we don't follow a rigid API review process around it because it's provided as-is.

Is our heuristic/rule that anything prefixed with Ilc is an "unsupported" switch? I don't think that is obvious to typical .NET developers. The general rule in MSBuild is to prefix unsupported switches with _.

Underscore didn't feel appropriate because we want people to be able to override them (that's why they're there - the underscored ones are typically set in props/targets and then read later - our Ilc ones are typically not set anywhere). But they're not documented on learn.microsoft.com or in MSBuild schema and we're not going to do a breaking change notice if we ever break them. Most of them were inherited from the runtimelab/CoreRT repo where any sort of distinction didn't make sense.