Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge the remaining groups of runtime tests #71732

Open
16 of 33 tasks
Tracked by #79018
trylek opened this issue Jul 6, 2022 · 5 comments
Open
16 of 33 tasks
Tracked by #79018

Merge the remaining groups of runtime tests #71732

trylek opened this issue Jul 6, 2022 · 5 comments

Comments

@trylek
Copy link
Member

trylek commented Jul 6, 2022

Related to: #54512

As of 7/6/2022, about 1/3 of the total runtime test set has been converted to using merged wrappers and running most tests in-proc to improve Helix perf. This issue is about fanning out the effort to the various runtime feature teams to follow up by converting the remaining runtime tests.

Original conversions:

  • JIT/Methodical
  • JIT/HardwareIntrinsics

According to the analytic tool ILTransform I created for the purpose of the conversion, the biggest remaining buckets of non-converted tests are in the following subtrees:

By merging these subtrees we should be able to reach about 70% conversion rate after which we can focus on the remaining smaller test groups.

Leading JIT test directories (updated 03/08/2023):

  • JIT/BBT (1 test)
  • JIT/CheckProjects (1 test)
  • JIT/Generics (222 tests)
  • JIT/Intrinsics (31 tests)
  • JIT/Math (2 tests)
  • JIT/opt (304 tests)
  • JIT/Performance (100 tests)
  • JIT/PGO (5 tests)
  • JIT/RyuJIT (1 test)
  • JIT/SIMD (116 tests)
  • JIT/Stress (5 tests)

Merged test groups impact day-to-day work, and directories such as JIT\Regression are actively modified. We also need to address usability issues.

Highest priority - can cause tests to be skipped or impact the team's ability to disable failing tests:

High priority - impact day-to-day work or debugging issues with test merging:

  • Write the generated Main methods to disk. There is a csproj property for this. Ideally the filename printed in C# error messages would match, but that might be difficult.
  • Fix Failure to build pri-1 tests #81984
  • The logic for ilasm roundtripping testing doesn't work correctly. It is implemented in the scripting layer and just does an ildasm/ilasm on the entry exe, which means that test merging ends up just doing the roundtrip in the new wrappers and not the original tests. The logic should probably be changed to "for x in <any .net assembly in the folder, possibly recursively>, roundtrip x".

Lower priority - these can (somewhat) be worked around but most aren't sustainable (e.g., poor documentation means all testing issues will come to Mark/Tomas/Jeremy)

  • Can we leave behind Main methods in tests (in addition to [Fact] methods) to allow for extra parameterization (e.g,. test --version)? [ANSWER: sort of - You can keep a Main method, but you need to then set ReferenceXUnitWrapperGenerator to false or BuildAsStandalone will break. Main will need a [Fact] or [Theory], and arguments (if any) will need to match across the [xxxData] and csproj property for BuildAsStandalone=true/false to be consistent.]
  • Write broader documentation on merged test groups.
  • Wrapper doesn’t fail when tests fail. However, don’t confuse test failures with test harness, etc., failures.
  • Wrapper prints a “display name” which isn’t suitable for feeding back to disable.
  • Can we measure the impact?

This isn't a specific work item but a change we all need to make: We need to aggressively disable tests that take down the runtime (and hide other tests). We should consider having JIT assertions throw managed exceptions, but these can be caught. Ideally we want monitoring (either within the wrapper - with the cost of extra code - or external to it).

Partitioning of subtrees under JIT/Regression

This was originally intended for tracking conversion progress and is useful for understand the set of tests there. However, the entire batch was converted at once, so it won't be used for tracking progress.

  • clr-x64-JIT, CLR-x86-EJIT, Dev14, v4 (21 tests total)
  • Dev11 (34 tests total)
  • VS-ia64-JIT
    • VS-ia64-JIT/M00 (46 tests total)
    • VS-ia64-JIT/V1.2-Beta1, VS-ia64-JIT/V1.2-M01, VS-ia64-JIT/V2.0-Beta2, VS-ia64-JIT/V2.0-RTM (39 tests total)
    • VS-ia64-JIT/V1.2-M02 (35 tests total)
  • JitBlue
    • JitBlue/CoreFX*, JitBlue/DevDiv_1*, JitBlue/DevDiv_2*, JitBlue/DevDiv_3* (40 tests total)
    • JitBlue/DevDiv_4*, JitBlue/DevDiv_5*, JitBlue/DevDiv_6*, JitBlue/DevDiv_7*, JitBlue/DevDiv_8*, JitBlue/DevDiv_9* (50 tests total)
    • JitBlue/GitHub_<less than 10000> (33 tests total)
    • JitBlue/GitHub_<10000 through 15999> (45 tests total)
    • JitBlue/GitHub_<16000 through 19999> (57 tests total)
    • JitBlue/GitHub_<20000 through 24999> (46 tests total)
    • JitBlue/GitHub_<25000 and above>, JitBlue/GitHub_CoreRT_2073 (24 tests total)
    • JitBlue/Runtime_<less than 50000> (45 tests total)
    • JitBlue/Runtime_<50000 and above>, WPF_3226 (48 tests total)
  • CLR-x86-JIT
    • CLR-x86-JIT/dev10, CLR-x86-JIT/dev11, CLR-x86-JIT/V1-M09, CLR-x86-JIT/V1-M10 (51 tests total)
    • CLR-x86-JIT/V1-M13-RTM, CLR-x86-JIT/V1-M14-SP1, CLR-x86-JIT/V1-M15-SP2 (45 tests total)
    • CLR-x86-JIT/V1-QFE, CLR-x86-JIT/V1.1-M1-Beta1, CLR-x86-JIT/V1.2-M01 (55 tests total)
    • CLR-x86-JIT/V1.2-M02, CLR-x86-JIT/V2.0-Beta2, CLR-x86-JIT/V2.0-RTM (31 tests total)
    • CLR-x86-JIT/v2.1, CLR-x86-JIT/v2.2 (29 tests total)
    • CLR-x86-JIT/V1-M09.5-PDC/b<less than 15000> (47 tests total)
    • CLR-x86-JIT/V1-M09.5-PDC/b<15000 through 25999> (48 tests total)
    • CLR-x86-JIT/V1-M09.5-PDC/b<26000 through 29999> (42 tests total)
    • CLR-x86-JIT/V1-M09.5-PDC/b<30000 and above> (32 tests total)
    • CLR-x86-JIT/V1-M11-Beta1/b<less than 41000> (44 tests total)
    • CLR-x86-JIT/V1-M11-Beta1/b<41000 through 44999> (52 tests total)
    • CLR-x86-JIT/V1-M11-Beta1/b<45000 and above> (42 tests total)
    • CLR-x86-JIT/V1-M12-Beta2/b<less than 37000> (47 tests total)
    • CLR-x86-JIT/V1-M12-Beta2/b<37000 through 52999> (47 tests total)
    • CLR-x86-JIT/V1-M12-Beta2/b<53000 through 58999> (41 tests total)
    • CLR-x86-JIT/V1-M12-Beta2/b<59000 through 64999> (47 tests total)
    • CLR-x86-JIT/V1-M12-Beta2/b<65000 through 71999> (50 tests total)
    • CLR-x86-JIT/V1-M12-Beta2/b<72000 through 79999> (44 tests total)
    • CLR-x86-JIT/V1-M12-Beta2/b<80000 and above> (20 tests total)

Thanks

Tomas

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 6, 2022
@trylek trylek added this to the 8.0.0 milestone Jul 6, 2022
@ghost
Copy link

ghost commented Jul 6, 2022

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

Related to: #54512

As of 7/6/2022, about 1/3 of the total runtime test set has been converted to using merged wrappers and running most tests in-proc to improve Helix perf. This issue is about fanning out the effort to the various runtime feature teams to follow up by converting the remaining runtime tests.

According to the analytic tool ILTransform I created for the purpose of the conversion, the biggest remaining buckets of non-converted tests are in the following subtrees:

() JIT/Regression (1312 tests)
(
) JIT/jit64 (828 tests)
() JIT/CodeGenBringUpTests (641 tests)
(
) JIT/Directed (541 tests)
() JIT/IL_Conformance (378 tests)
(
) baseservices/threading (231 tests)

By merging these subtrees we should be able to reach about 70% conversion rate after which we can focus on the remaining smaller test groups.

Thanks

Tomas

Author: trylek
Assignees: -
Labels:

area-Infrastructure-coreclr

Milestone: -

@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jul 6, 2022
@agocke
Copy link
Member

agocke commented Jul 28, 2022

@trylek Is this still scheduled for 7.0? Never mind read the issue wrong.

@jkotas
Copy link
Member

jkotas commented Jul 30, 2022

Follows partitioning of subtrees under JIT/Regression for tracking conversion progress

Can we document how to do this work here? If I know nothing about this and I want to volunteer to pick up one of the categories, how do I go about it?

@ivdiazsa
Copy link
Member

Follows partitioning of subtrees under JIT/Regression for tracking conversion progress

Can we document how to do this work here? If I know nothing about this and I want to volunteer to pick up one of the categories, how do I go about it?

Count me in as well.

@trylek
Copy link
Member Author

trylek commented Sep 2, 2022

Basic conversion instructions

Typically, the conversion is easiest for csproj projects that are unconditional - they don't depend on the targeting OS, architecture and / or execution modes like GC stress. For ilproj test projects and for conditional csproj test projects the situation is slightly more complicated.

I have created an internal app to assist with a semi-automated conversion. You can find it in the private branch

https://github.com/trylek/runtime/tree/ILTransform

After you clone this branch (it's easier to clone it into a different tree than the one we're trying to convert), the new project ILTransform will appear under src/ILTransform. I'm typically opening and running the project from the Visual Studio. It has several execution modes but the one most interesting for the actual conversion is as follows:

<path to the built ILTransform app> -f <subfolder of src/tests to process>

This syntax causes the app to carry out the standard conversion steps for all csproj and ilproj projects in the specified subtree. In particular, it renames Main methods in csproj projects based on the project name and attaches the [Fact] attributes to them. For ilproj projects, it suffices to attach the [Fact] attribute, the entrypoint method remains in place so that the test can be run individually (this is a fundamental requirement for some of the low-level tests).

In the root folder of the subtree being converted, it's also necessary to modify the Directory.Build.props by including the following property group:

  <PropertyGroup>
    <BuildAsStandalone Condition="'$(BuildAsStandalone)' == ''">false</BuildAsStandalone>

    <AssemblyName Condition="'$(BuildAsStandalone)' != 'true'">$(MSBuildProjectName.Replace('_il_d', '').Replace('_il_r', ''))</AssemblyName>
  </PropertyGroup>

as seen in the file

The BuildAsStandalone property overrides the still default policy of building the tests individually, the AssemblyName caters for subtle shenanigans involving naming of ilproj tests and assembly names in them. Furthermore you need to add a new project (possibly multiple projects) to represent the merged test wrappers in your tree. Here is an example from the JIT/Methodical subtree where it's slightly more involved as due to the large number of tests we use seven different wrappers (Methodical_d1, Methodical_d2, Methodical_do, Methodical_r1, Methodical_r2, Methodical_ro, Methodical_others) to partition the large number of tests (~2000):

https://github.com/dotnet/runtime/blob/main/src/tests/JIT/Methodical/Methodical_d1.csproj

According to our experience from merging the first 1/3 of the test tree the sweet spot for test merging seems to be several hundred tests per merged wrapper. This may vary with complexity of the individual tests and might require a bit of local experimentation. If everything works as expected, these instructions should be sufficient to convert a particular subtree of the test tree. I'll add more instructions to cover more complicated cases when the described workflow fails.

markples added a commit that referenced this issue Mar 31, 2023
Remaining usability issues are listed in #71732

Conversion steps:
- static-none uses args - add comment to RequiresProcessIsolation
- remove unused arg in Runtime_80731.il
- Rewrite 4 IL tests to use newarr instead of args
- [cs-main] remove unused args in C# tests
- Update 9 tests with Main(args)
- Workaround: add RequiresProcessIsolation to LdfldaHack
- Move [Fact] to correct location in b12263
- In b06020, wrap $ class so that the C# wrapper can call it
- [ILTransform -public]
- [ILTransform -sr]
- Manually rename b598031/test and test2.
  These were missed by the previous ILTransform -n run.
  They no longer clash because run renamed all of their conflicts,
  but this renames them for consistency.
- [ILTransform -prociso] Rerun for new tests
- Wrappers
- [ILTransform -ilfact]
- Manually do missed -collapse-main-sig and fix placement of [Fact]
- Fix b89946 that referred to Main
- Fix Runtime_59444 reflection on Test*
* Fix TestSummary.cs to handle invalid XML characters
* [ILTransform -a]
* Manual fixes
* Update test groupings and add extern aliases
* Finish merge - incorporate changes to csproj/Dir.B.props - fix xunit.analyzers errors
* [ILTransform -public]
* [ILTransform -prociso]
* [ILTransform -ilfact] (undo GitHub_26491)
* Fix xUnit1003 in Runtime_83003
* Update new test Runtime_83941 for merged groups
* 59444 - Changing methods to internal broke reflection calls.  Use this
  opportunity to remove the reflection and simply mark the callees as [Fact]s.
- 64883/76273 - Suppress the xunit warning so that the code can continue
  using the normal (public-only) reflection search.
- Fix TestSummary.cs to handle invalid XML characters
- Add RequiresProcessIsolation for existing issues.target entry (AOT compiler fails)
- undo JIT/opt/And/Regressions/Regression1.csproj (possibly from manual merge from main)
@agocke agocke modified the milestones: 8.0.0, 9.0.0 Sep 11, 2023
@agocke agocke modified the milestones: 9.0.0, 10.0.0 Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

4 participants