Implement correct handling of new constraint in ilc #82818
Conversation
This is done by treating it as a data flow annotation PublicParameterlessConstructor (which are supposed to be identical). The rest falls out from this change since all of the validation and marking will automatically kick in.
Also fixes the rest of the Generic tests which were brought along with the test unification with ILLink. Disabled some of the tests which need more investigation.
Adds a comment about potential optimizations to reduce number of data flow processing done.
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsThis is done by treating the new constraint as a data flow annotation This change causes more methods to go through data flow (since that's what will actually perform the constraint validation/marking). I tested this on the ASP.NET API AOT template. Before this change it ran ~620 methods through data flow. With this change that number goes up to ~2100. The problem is tracked by #82603. I measured compiler perf but didn't see any noticeable changes. That said, the compiler perf is extremely noisy on my local machine (variations of almost 10%), so it's hard to say for sure what the perf impact is. Current thinking is that ~2100 is still not that much and most of those methods are pretty small and thus cheap to run data flow on. Fixes #81720 - note that the repro still fails on AOT with this fix, but the failure is different (
|
Could you try to apply #81205 and use the ILC from ilc-published directory to do the measurement? I saw similar amounts of noise when doing perf measurements in the past. I don't know how to measure perf on top of the JIT-based CoreCLR anymore. The results with NativeAOT were a lot more stable, plus it is what we intend to ship anyway. |
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
The details are in the comments in the code, in short: If there's a array with itself as an array element the dataflow would stack overflow.
|
@sbomer could you please take a quick look at the last commit which fixes a bug in array value dataflow - also in illink and analyzer. |
|
/azp run |
|
You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list. |
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ghost
locked as resolved and limited conversation to collaborators
This is done by treating the new constraint as a data flow annotation
PublicParameterlessConstructor(which are supposed to be identical). The rest falls out from this change since all of the validation and marking will automatically kick in.This change causes more methods to go through data flow (since that's what will actually perform the constraint validation/marking). I tested this on the ASP.NET API AOT template. Before this change it ran ~620 methods through data flow. With this change that number goes up to ~2100. The problem is tracked by #82603.
I measured compiler perf but didn't see any noticeable changes. That said, the compiler perf is extremely noisy on my local machine (variations of almost 10%), so it's hard to say for sure what the perf impact is. Current thinking is that ~2100 is still not that much and most of those methods are pretty small and thus cheap to run data flow on.
Fixes #81720 - note that the repro still fails on AOT with this fix, but the failure is different (
System.InvalidOperationException: Sequence contains no matching element). I verified that the missing .ctor is present in the app with the fix.