ILLink: Rebuild override info after custom step runs

[sbomer]

This modifies the tracking of overrides in TypeMapInfo to allow additional overrides to be discovered on a subsequent call to MapType. Then after a custom step runs for a marked type, we build the type info again to ensure that any additional overrides introduced by the custom step are taken into account.

Fixes #104266. However, note that there are still many kinds of modifications that a custom step could make which are not guaranteed to be taken into account, and there's no guarantee about the order in which different types in an assembly are processed by the custom step. However, the modifications done in FixAbstractMethodsStep should work.

We should be able to reintroduce the dependency analysis framework with this, but I will make that change in a separate PR.

[vitek-karas]

Does this mean that if the custom step registers a type action then we will recreate the mapinfo for all types we mark... well I guess we don't have a way to tell if the type action actually did anything... it's unfortunate.

[sbomer]

Yes unfortunately. One alternative is to build the type info for just one type at a time (instead of every type in the assembly) - so that for most types, hopefully MarkType is the first and only time we build the type info. But GetOverrides is visible to custom steps, so it's possible a custom step is relying on the current behavior to see all overrides of a virtual method from an assembly.

[jtschuster]

If we want to avoid significant perf degradation (and stability issues), I think we need to be able to guarantee the type map is read/init only. Type mapping is one of the more expensive parts of the linker. Maybe we could consider enforcing that no changes to IL can occur after MarkStep has started? Can the custom step with the issue run entirely before MarkStep rather than register a MarkTypeAction? It doesn't look like it depends on any marking info.

[vitek-karas]

that no changes to IL can occur after MarkStep has started

The trimmer itself violates this - the constant propagation and branch elimination happens during method processing in MarkStep - this was one of the big perf optimizations we've done in previous releases, to avoid running the constant prop on all methods in the input and instead only run it on "marked" methods. (This is a lot more complicated and the description is a bit fuzzy, but the core is true).

Re custom steps: I think this is similar - we want to optimize and only run the custom step on marked types, so that we avoid doing work for types which are going to be trimmed away. I don't know if there would be side effects if we did that.

We could do something similar to what the constant-prop does for methods - it also doesn't run on JUST marked methods. Instead, it starts from marked methods but then follows the callgraph to any method it needs to go to. The type map could do something similar, but it would be tricky to get it right - for example if we blindly collect all implementing methods for some core interface (like IEnumerable), we would end up processing the entire input. But then we could wire the custom step to run as part of the type map instead, so it would run "before" the type map sees the IL of a given type. It would not be the same as now - the custom step would run on more types, but hopefully still better then it running on everything. (and we would still have to check that it's OK to run the custom step on more types than necessary).

Overall I'm fine with the current behavior - the cost is only incurred for trimmer runs which use custom steps (so basically mono only) - and for a good reason (mono needs the custom steps currently).

I hope to get to do more work on mono interop in .NET 10 and hopefully truly start moving away from custom steps. But I expect that we will need to keep them around for a couple of years still (even if we figure out the correct solution, it's unlikely we would be able to adapt it for both iOS and Android in one release).

[sbomer]

I did some rough experimentation, using the repro @vitek-karas provided for this issue. I prototyped a change to the custom logic to make it run before MarkStep, load all referenced assemblies, and do the FixAbstractMethods logic for all types.

Time taken by ILLink:

• main: 7.56s
• with the change in this PR (using existing custom step that runs for marked types only): 8.60s
• with ILLink from main, but the custom step from Run FixAbstractMethodsStep before MarkStep android#9207: 7.71s

Based on just these numbers I would be in favor of changing the custom step, but I share this concern:

we want to optimize and only run the custom step on marked types, so that we avoid doing work for types which are going to be trimmed away. I don't know if there would be side effects if we did that.

Loading all assemblies in the custom step may have downstream impact beyond just the FixAbstractMethods logic.

The type map could do something similar, but it would be tricky to get it right - for example if we blindly collect all implementing methods for some core interface (like IEnumerable), we would end up processing the entire input. But then we could wire the custom step to run as part of the type map instead, so it would run "before" the type map sees the IL of a given type.

Sounds like a reasonable suggestion, but I agree it would be tricky, and I don't think introducing more extension points for custom steps is the right direction long term, since we're trying to move away from custom steps.

the cost is only incurred for trimmer runs which use custom steps (so basically mono only) - and for a good reason (mono needs the custom steps currently).

Note we also use a custom mark handler for discovering custom operators, but I could update this PR to only rebuild override info if there were externally supplied mark handlers.

[sbomer]

Now that main is .NET 10, I think we should take this change then reintroduce the dependency analysis framework.