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

Testmerging JIT/Directed #81969

Closed
wants to merge 31 commits into from
Closed

Conversation

BrianBohe
Copy link
Member

@BrianBohe BrianBohe commented Feb 10, 2023

This another pr merging JIT tests as PR80597. It starts with these commits that prepare the directory to use later ILTransform like:

Then cs-main and ILTransform projects are used with the arguments specified on commits in the directory. This is the version of the tool I used. Some observations:

  • "ILTransform -n" is an heuristic approach to deduplicate project files, so it may need to run more than once to actually deduplicate project files, in this case 3 times.
  • Tester test on debuginfo does not need a transformation so this commit changes it back.
  • ILTransform -public is too naive, so it may add duplicated "public" tokens when they are, this commit removes them. It could be that private methods, that could still be private, are changed to public exposing private types, this commit fix these cases. It seems it also missed a few cases added later on this commit.

@ghost
Copy link

ghost commented Feb 10, 2023

Tagging subscribers to this area: @dotnet/area-system-reflection-metadata
See info in area-owners.md if you want to be subscribed.

Issue Details

This another pr merging JIT tests as PR80597. It starts with these commits that prepare the directory to use later ILTransform like:

Then cs-main and ILTransform projects are used with the arguments specified on commits in the directory. This is the version of the tool I used. Some observations:

  • "ILTransform -n" is an heuristic approach to deduplicate project files, so it may need to run more than once to actually deduplicate project files, in this case 3 times.
  • Tester test on debuginfo does not need a transformation so 8c964b changes it back.
  • ILTransform -public is too naive, so it may add duplicated "public" tokens when they are, this commit removes them. It could be that private methods, that could still be private, are changed to public exposing private types, this commit fix these cases. It seems it also missed a few cases added later on this commit .
Author: BrianBohe
Assignees: BrianBohe
Labels:

area-System.Reflection.Metadata

Milestone: -

@BrianBohe
Copy link
Member Author

BrianBohe commented Feb 10, 2023

Before adding the wrapper, the test run looks is:

Time [secs] | Total | Passed | Failed | Skipped | Assembly Execution Summary
============================================================================
    311.617 |   648 |    648 |      0 |       0 | JIT.Directed.XUnitWrapper.dll
----------------------------------------------------------------------------
    311.617 |   648 |    648 |      0 |       0 | (total)

this can be get running git checkout .; git clean -xdf; ./build clr+libs -rc debug -lc Release;.\src\tests\build debug generatelayoutonly; .\src\tests\build tree JIT\Directed -priority=1 rebuild; .\src\tests\run; on this branch

With the last commit on this PR, the tests run look like this:

Time [secs] | Total | Passed | Failed | Skipped | Assembly Execution Summary
============================================================================
     86.431 |    48 |     48 |      0 |       0 | JIT.Directed.Directed_cs_r
     70.659 |    48 |     48 |      0 |       0 | JIT.Directed.Directed_cs_ro
    118.592 |   131 |    131 |      0 |       0 | JIT.Directed.Directed_d
     99.982 |    73 |     73 |      0 |       0 | JIT.Directed.Directed_do
    163.975 |   216 |    215 |      0 |       1 | JIT.Directed.Directed_others
     39.468 |    94 |     94 |      0 |       0 | JIT.Directed.Directed_r
     36.185 |    36 |     36 |      0 |       0 | JIT.Directed.Directed_ro
      0.477 |     0 |      0 |      0 |       0 | JIT.Directed.Directed_unix
      7.562 |     1 |      1 |      0 |       0 | JIT.Directed.Directed_windows
----------------------------------------------------------------------------
    623.332 |   647 |    646 |      0 |       1 | (total)

There are multiple failures on this last output:

  1. Test JIT\Directed\rvastatics\RVAOrderingTest.ilproj is skipped on Directed_others now with no apparent reason (<reason><![CDATA[No Known Skip Reason]]></reason>) and run fine before merging tests.
  2. It seems I am missing 2 tests but the test run has a diff of 1, which I found really confusing. Two tests are being built but not run now (and run before merging tests):
  1. Time of the test run is twice as before. 1/5 of the total amount of tests need to be executed isolated but there should be an improvement. (Although the table summary displays a total run of ~610 seconds, I am clocking it and run in less than ~170 seconds. The table might be adding each test group time as if they run sequentially. Time is no longer a problem.)

@BrianBohe
Copy link
Member Author

Related issue 71732

@BrianBohe BrianBohe marked this pull request as draft February 14, 2023 22:43
@BrianBohe
Copy link
Member Author

There are errors building tests on different non-windows platforms but I cannot reproduce them. I tried emulating the same lines the pipeline is executing in a docker container following these steps:

  1. running docker on wsl2: docker run -it -v /<path_to_runtime>:/runtime -w /runtime mcr.microsoft.com/dotnet-buildtools/prereqs:centos-7 (this is the image used in pipeline "runtime (Build coreclr Common Pri0 Test Build AnyOS AnyCPU checked)")
  2. Launch bash inside the docker console session and run: ./build.sh libs.sfx+libs.oob+clr.iltools /p:RefOnly=true -c Release -ci --clang9 (command taken from logs on build coreclr step)
  3. Then build the tests running: ./src/tests/build.sh -log:Managed allTargets skipnative skipgeneratelayout skiptestwrappers checked x64 ci /p:TargetOS=AnyOS (command taken from logs on build managed test components)

That is not failing, and tests can be run then with ./src/tests/run.sh

@trylek
Copy link
Member

trylek commented Feb 16, 2023

@BrianBohe - I assume you're talking about the "entry point is global code" error. Unfortunately @jkoritzinsky is OOF until next Tuesday, I think he would be best suited to comment on that, I guess it's some shenanigans around Roslyn language version - as I see it, the global entrypoint is the Roslyn-generated wrapper, not some hand-written code. Having said that, it's quite weird that it only affects just a handful of tests, if the simple test wrappers generating global entry points are the real problem, it should be affecting 3/4 of our tests and I don't see that in the log.

@markples
Copy link
Member

nullable.cs now has a [Fact] Main method, but it should have a different name. Note also that I think you can undo the change of moving the test entry out of invokecommon.cs. I'm guessing that this was done to dedup the entry point's namespace, but it is no longer necessary to do that.

It is exactly the same as JIT/Directed/StrAccess/straccess3.cs with
straccess3_cs_d.csproj
Duplicated from src\tests\JIT\Directed\shift\int16_cs_d.csproj
Duplicated from src\tests\JIT\Directed\shift\int16_cs_do.csproj
Duplicated from src\tests\JIT\Directed\shift\int32_cs_d.csproj
Duplicated from src\tests\JIT\Directed\shift\int32_cs_do.csproj
Duplicated from src\tests\JIT\Directed\shift\uint16_cs_d.csproj
Duplicated from src\tests\JIT\Directed\shift\uint16_cs_do.csproj
Duplicated from src\tests\JIT\Directed\shift\uint32_cs_d.csproj
Duplicated from src\tests\JIT\Directed\shift\uint32_cs_do.csproj
Duplicated from src\tests\JIT\Directed\shift\uint8_cs_d.csproj
Duplicated from src\tests\JIT\Directed\shift\uint8_cs_do.csproj
Replacing arg string with empty string
It seems there a few cases where namespaces and clases
are listed twice, like a declaration and definition.
IL_Transform tool does not like it and there is no need
to have this so I am removing.

Related PR dotnet#64837.
IL_Transform does not like the actual include path expression.
We keep the reference but rewrite as it as in PR dotnet#66157 on Methodical,
TestLibraryProjectPath is defined in
src/tests/Directory.Build.targets.
Actual dedup strategy groups projs by finishing _r, _d, etc.
This file does not hav a _d pair and inside
src/tests/JIT/Directed/coverage/importer/Desktop there is its
analogous and a _d variation which won't be dedup.
This tests was modified by ILTransform -p.
@BrianBohe
Copy link
Member Author

BrianBohe commented Feb 22, 2023

This is how the test run summary looks like now (the host machine is different, so times are different):

Time [secs] | Total | Passed | Failed | Skipped | Assembly Execution Summary
============================================================================
    219.418 |    48 |     48 |      0 |       0 | JIT.Directed.Directed_cs_r
    187.806 |    48 |     48 |      0 |       0 | JIT.Directed.Directed_cs_ro
    262.771 |   131 |    131 |      0 |       0 | JIT.Directed.Directed_d
    236.093 |    73 |     73 |      0 |       0 | JIT.Directed.Directed_do
    293.788 |   127 |    126 |      0 |       1 | JIT.Directed.Directed_others_group1
     86.407 |    92 |     92 |      0 |       0 | JIT.Directed.Directed_others_group2
    102.000 |    94 |     94 |      0 |       0 | JIT.Directed.Directed_r
     92.298 |    36 |     36 |      0 |       0 | JIT.Directed.Directed_ro
----------------------------------------------------------------------------
   1480.582 |   649 |    648 |      0 |       1 | (total)

Tests Directed\IL\leave\leave2.il and Directed\Misc\function_pointer\MutualThdRecur-fptr.il were fixed wrapping the entry point, which was a "global static public int32 main", with a "public class".

The skipped test is Directed\rvastatics\RVAOrderingTest.ilproj.

@BrianBohe
Copy link
Member Author

/azp run runtime-coreclr outerloop

@BrianBohe BrianBohe marked this pull request as ready for review February 22, 2023 19:44
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@markples
Copy link
Member

RVAOrderingTest appears to be excluded in issues.targets:

        <ExcludeList Include="$(XunitTestBinBase)/JIT/Directed/rvastatics/RVAOrderingTest/*">
            <Issue>https://github.com/dotnet/runtime/issues/55966</Issue>
        </ExcludeList>

I would expect it to be skipped in the current runs as well. Are you not seeing that?

@BrianBohe
Copy link
Member Author

There are many tests that ended up running and passing after being renamed, I have updated those paths on issues.targets but may be a good moment to unable them back. Here is one of the targeted issues

@steveharter steveharter added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed area-System.Reflection.Metadata labels Feb 23, 2023
@ghost
Copy link

ghost commented Feb 23, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

This another pr merging JIT tests as PR80597. It starts with these commits that prepare the directory to use later ILTransform like:

Then cs-main and ILTransform projects are used with the arguments specified on commits in the directory. This is the version of the tool I used. Some observations:

  • "ILTransform -n" is an heuristic approach to deduplicate project files, so it may need to run more than once to actually deduplicate project files, in this case 3 times.
  • Tester test on debuginfo does not need a transformation so this commit changes it back.
  • ILTransform -public is too naive, so it may add duplicated "public" tokens when they are, this commit removes them. It could be that private methods, that could still be private, are changed to public exposing private types, this commit fix these cases. It seems it also missed a few cases added later on this commit.
Author: BrianBohe
Assignees: BrianBohe
Labels:

area-CodeGen-coreclr

Milestone: -

@@ -3633,8 +3981,7 @@
IL_0006: ret
} // end of method ClassImpl::.ctor

.property instance callconv(8) class BaseClass
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the whole file change and seems to be ok but I am not sure about losing this. Do you know @jakobbotsch ?

@BrianBohe
Copy link
Member Author

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@BrianBohe
Copy link
Member Author

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@BrianBohe
Copy link
Member Author

@trylek
I can see on logs that most of failures looks like (example):
Unhandled exception. System.IO.FileNotFoundException: Could not load file or assembly 'C:\h\w\AE57097E\w\AD9C0917\e\JIT\Directed\Directed_others_group2\Directed_others_group2.dll'. The system cannot find the file specified.
Have you seen something like that before with Methodical?

Also, in step Generate test wrappers of coreclr Pri0 Runtime Tests Run windows x64 checked I can see that JIT.Directed.XUnitWrapper.dll is still being built in place of the Directed_r/ro/d/*.dlls . Could it be related to the way we are referencing the test groups?

@trylek
Copy link
Member

trylek commented Mar 1, 2023

Thanks Brian for the heads-up. I guess I'll need to play with this for a bit locally to see what's gone awry. For generation of the legacy wrapper, that typically signifies that not all tests have been switched over - basically, the legacy XUnit wrapper tests are generated based on the previously generated cmd / sh test execution scripts, the MSBuild script just collects them and passes them to the XUnit wrapper generator (unless they're specifically marked as out-of-process items of a merged test wrapper).

@markples
Copy link
Member

Other failures were

  • I suggested using DefineConstants in C# to combine a Windows/Linux test, but we used an AnyOS C# build
  • Missing updates in issues.targets for project renames

This PR will be replaced by #83256 so that I can write to it.

@BrianBohe BrianBohe closed this Mar 10, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Apr 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants