Use static abstract interface methods in SymbolicRegexMatcher #63546
Conversation
ghost
assigned 
|
Tagging subscribers to this area: @dotnet/area-system-text-regularexpressions Issue DetailsRe-opening #61631 to get CI logs
|
|
/azp run runtime-coreclr crossgen2 outerloop |
|
cc: @tannergooding, @trylek |
|
Azure Pipelines successfully started running 1 pipeline(s). |
stephentoub
added
NO-MERGE
|
On second thought, while it's great to see that the Crossgen2 legs are passing, it actually doesn't say much about Crossgen2 and static interface functionality w.r.t. your change as I believe we still don't have library tests that would run Crossgen2 on the test IL code - we only do that for the framework assemblies. Still, I presume that if the relevant tests pass locally for you with Crossgen2 enabled, it should be fine. Some time ago @danmoseley offered help in finding someone to look into adding infra support for crossgenning library tests as I myself am not really an expert on library test build & execution infra but I'm not aware of any follow-up so far. |
Can you elaborate? What would I do locally that CI doesn't do? |
|
Well, maybe it's not a concern at all. The one thing we're basically unable to test today is running a Crossgen2-compiled library test. Considering you're only changing bits within the framework that does get Crossgen2-compiled by itself, it's probably not a big deal, however it's still a test gap especially considering that Tanner recently raised the concern that static virtual methods may not fully function in Crossgen2 context. |
Thanks. Even if it's not causing failures for crossgen, it sounds like it would still cause more of the assembly to fall back to JIT? Do we consider that a problem? Also, @lambdageek, this passed all mono legs, but presumably this still shouldn't be merged until mono's fixes are in? Is there some leg I should run to validate that? |
@agocke @ericstj is this on our infrastructure backlog somewhere (where it's relative priority lies I do not know)? |
|
@danmoseley it looks like this issue: #50127 |
|
It's not currently high on my backlog. It may help crossgen2 find bugs, but crossgen'ing the framework is already fairly expensive. I'm worried it could significantly degrade build perf. |
|
Could we enable it as an outerloop/on-demand job? |
|
I guess that running such a job once or twice a week or on demand should be sufficient. To be precise, I believe there are two aspects to the issue: One is using CG2-compiled framework - we're already doing that for CoreCLR CG2 testing (as part of generating CORE_ROOT) but I think that library tests use a different method to access the framework assemblies and that today doesn't involve CG2 compilation at all. (CG2 compilation does run on the framework assemblies during "packs", previously "installer", builds, but AFAIK the product of this compilation is only used to run the "packs"-specific tests, not library tests, in the runtime repo.) The second aspect is CG2 compilation of the tests themselves and that is supposed to require non-trivial modifications to the library test build scripts. I just checked a random CG2 outerloop run and composite compilation of framework takes about 3 minutes, non-composite about 5~6 minutes; I wouldn't be too worried about test compilation performance as library tests use less than two hundred merged test executables i.o.w. I would expect their compilation to take even less time than CG2-compiling the framework. And after all execution of the crossgenned apps should be faster, at the very least in terms of startup perf. In general, both aspects add code coverage of various CG2 compilation constructs including the static virtual methods. The existing code coverage in Crossgen2 pipelines amounts to validating that the compiler doesn't crash when presented with a new construct in the framework (like in this change) and that the CoreCLR tests run fine on top of the crossgenned framework but I suspect that targeted testing of SymbolicRegexMatcher in CoreCLR tests is very limited, certainly more limited than testing of the feature in the library test set. |
|
Adding it as part of an outerloop sounds fine to me, but I would expect that would be led primarily by the product team w/ assistance from infra where needed. |
|
Well, I can easily deal with the ceremony regarding proper setup of the YAML scripts and such, my only problem is fixing the library test build scripts themselves to support Crossgen2 compilation - I have never made changes in that space before; the last time we discussed this Viktor Hofer also suggested that support for Crossgen2 compilation may entail some generally desirable long-planned cleanup of the scripts; I must admit that due to my limited knowledge of these scripts I don't know what precisely he referred to, perhaps @safern or @jkoritzinsky may have a better idea. |
|
/azp run runtime-manual
@steveisok @akoeplinger Do we have any legs that run libraries tests with FullAOT that @stephentoub should pay attention to? |
|
No pipelines are associated with this pull request. |
|
To put it yet another way, I'll be happy to work with someone familiar with library test build scripts to get this fixed by providing Crossgen2-related expertise but I feel reluctant to drill into this just by myself as I simply won't be efficient in that space. I think it basically involves the following changes: (*) Identifying the place where the framework gets passed to Helix, probably as a correlation payload, and modifying that place to support optional CG2 compilation, probably by using the existing logic in the SDK repo. (*) Deciding whether CG2 compilation of the test assemblies should happen on the build machine or on the Helix execution machines. In the former case it should become a part of producing the test payloads for Helix, in the latter case it should be part of some execution-time scripting running on the Helix machines and we need to figure out how to make Crossgen2 built for the proper OS and architecture available to Helix, possibly as yet another correlation payload. (In CoreCLR testing it is passed via CORE_ROOT.) Crossgen2 is a managed app so we also need the .NET Host on the Helix machines. (*) Using the above components, typically controlled by some environment variables or by command-line arguments to the build scripts, to build the new pipeline. I can easily deal with that part once the library script changes are in place. Thanks Tomas |
|
@trylek I can definitely help with any questions/blockers you may find when enabling this crossgen'd mode with any related to running crossgen on the shared framework we generate to run our tests or as part of the libraries test build. |
|
Thanks Santi for your kind offer of help. I'll mention it in today Jeff's backlog meeting and we can then schedule a separate chat about the technical details I outlined. |
stephentoub
force-pushed
the
srmstaticinterfaces
branch
from
b497a3f
to
a570d72
Compare
stephentoub
force-pushed
the
srmstaticinterfaces
branch
from
a570d72
to
91f32e1
Compare
|
No pipelines are associated with this pull request. |
|
/azp list |
|
Sorry, it's |
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
I assume the failures like these are the indication of the lack of static abstract interface support (it would require falling back to JIT but falling back to JIT isn't allowed):
|
Yea it looks like we are missed AOTing an instance here. @vargaz |
|
Made a new issue for the the mono FullAOT fail #65002 |
|
Thanks, @lambdageek. |
|
@stephentoub merge/rebase with |
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
At this point inner loop passed, there are no regex-related failures in runtime-extra-platforms, and the Crossgen2 legs previously all passed. Any remaining static abstract interface method reasons this can't be merged? |
stephentoub
removed
the
NO-MERGE
|
Based on the fact that the known blockers are no longer blockers, I'm going to merge. Long live static abstract interface methods. |
Re-opening #61631 to get CI logs
Closes #62650