[NO-MERGE / DESIGN DISCUSSION ONLY] Infra fixes for Ivan's JIT count test #90647

trylek · 2023-08-15T23:20:38Z

This change combines Ivan's private branch 'jit-participants'
with a couple of my infra fixes to make his test work. The fundamental
remaining piece is figuring out how to get rid of the commented out
line in XUnitWrapperGenerator.cs, @jkoritzinsky please advise how
to check the ProjectReference item for the ReferenceOutputAssembly
so that we ignore the reference in the merged wrapper generator
when it's set to false - i.o.w. when the project doesn't intend to
use the reference as a direct component of its managed executable
so that it's rather intended as a payload that's manipulated in a special
manner at runtime.

I assume that, once we clarify this one remaining problem, I'll send
out the PR for the build script changes; that should subsequently
let Ivan adapt my changes to his test case based on his discretion
and make it fully functional both locally and in the lab.

Thanks

Tomas

@jkoritzinsky

This change combines Ivan's private branch 'jit-participants' with a couple of infra fixes to make his test work. The fundamental remaining piece is figuring out how to get rid of the commented out line in XUnitWrapperGenerator.cs, @jkoritzinsky please advise how to check the ProjectReference item for the ReferenceOutputAssembly so that we ignore the reference in the merged wrapper generator when it's set to false - i.o.w. when the project doesn't intend to use the reference as a direct component of its managed executable, it's rather intented as a payload that's manipulated in a special manner at runtime. I assume that, once we clarify this one remaining problem, I'll send out the PR for the build script changes; that should subsequently let Ivan adapt my changes to his test case based on his discretion and make it fully functional both locally and in the lab. Thanks Tomas

ghost · 2023-08-15T23:21:02Z

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

Issue Details

This change combines Ivan's private branch 'jit-participants'
with a couple of my infra fixes to make his test work. The fundamental
remaining piece is figuring out how to get rid of the commented out
line in XUnitWrapperGenerator.cs, @jkoritzinsky please advise how
to check the ProjectReference item for the ReferenceOutputAssembly
so that we ignore the reference in the merged wrapper generator
when it's set to false - i.o.w. when the project doesn't intend to
use the reference as a direct component of its managed executable
so that it's rather intended as a payload that's manipulated in a special
manner at runtime.

I assume that, once we clarify this one remaining problem, I'll send
out the PR for the build script changes; that should subsequently
let Ivan adapt my changes to his test case based on his discretion
and make it fully functional both locally and in the lab.

Thanks

Tomas

Author:	trylek
Assignees:	-
Labels:	`NO-MERGE`, `area-CodeGen-coreclr`
Milestone:	-

trylek · 2023-08-15T23:38:51Z

src/tests/JIT/Performance/JittedMethodsCountingTest/JittedMethodsCountingTest.csproj

+  </ItemGroup>
+
+  <PropertyGroup>
+    <ProjectAssetsFile>$(JitPackagesConfigFileDirectory)benchmark/obj/project.assets.json</ProjectAssetsFile>


Do we know if this is actually necessary? I don't think we're using it in other tests.

trylek · 2023-08-15T23:39:44Z

src/tests/JIT/Performance/JittedMethodsCountingTest/JittedMethodsCountingTest.csproj

+
+  <ItemGroup>
+    <ProjectReference Include="HelloWorld/HelloWorld.csproj">
+      <ReferenceOutputAssembly>false</ReferenceOutputAssembly>


This clause is what I believe should tell the merged wrapper generator that we should skip this reference as it's not supposed to be a direct project component.

You might also need to change the OutputItemType to Content to get it to not warn.

Hmm, is that somehow related to the next line or something completely different?

Sorry, didn't see that under the comment. You're right, nevermind.

trylek · 2023-08-15T23:42:20Z

src/tests/JIT/Performance/JittedMethodsCountingTest/JittedMethodsCountingTest.cs

+            startInfo.EnvironmentVariables.Add("DOTNET_JitDisasmSummary", "1");
+            startInfo.EnvironmentVariables.Add("DOTNET_TieredCompilation", "0");
+            startInfo.EnvironmentVariables.Add("DOTNET_ReadyToRun", "1");
+            startInfo.EnvironmentVariables.Add("DOTNET_JitStdOutFile", jitOutputFile);


I tried this without the stdout file redirection and it seemed to work for me when I specified RedirectStandardOutput=true in the ProcessStartInfo constructor and I used app.StandardOutput.ReadToEnd() before the call to WaitForExit. Having said that, using the DOTNET variable for explicit specification of JIT standard output seems easier at the very least because the app emits the Hello world! string at the last line and you'd need to modify the parser to cope with that.

I think using DOTNET_JitStdOutFile is the better way for these main reasons:

It's self-explanatory as to what we're doing with that file.

We don't have to parse the app's output out of the way, plus only having the jitted methods can make investigations easier.

The fact dotnet keeps eating the output on my machine is concerning. There might be some underlying bug that only seems to repro under specific circumstances, and it makes this test give incorrect results when it happens.

trylek · 2023-08-15T23:43:36Z

src/tests/JIT/Performance/JittedMethodsCountingTest/JittedMethodsCountingTest.cs

+        string[] lines = File.ReadAllLines(jitOutputFile);
+
+        Console.WriteLine("=========== App standard output ===========");
+        foreach (string line in lines)


I leave it up to you how verbose you decide the output should be but please keep in mind that sufficient diagnostic information can typically substantially speed up bug investigation.

Yeah I was planning to print everything afterwards. I only had the short output you saw first to make it easier to see whether the test was running appropriately.

trylek · 2023-08-15T23:46:12Z

src/tests/JittedMethodsCountingTest/JittedMethodsCounter/JittedMethodsCounter.cs

+public class JittedMethodsCounter
+{
+    [Fact]
+    public static int TestEntryPoint()


I'm not sure about this one, is that an earlier version of the test you're working on now so that we should ultimately get rid of it or are you envisioning two different modes of testing?

This is going to be removed. It slipped through. The actual test is the other file you reviewed.

trylek · 2023-08-15T23:49:34Z

src/tests/JIT/Performance/JittedMethodsCountingTest/JittedMethodsCountingTest.cs

+        int appResult = RunHelloWorldApp(appName, jitOutputFile);
+        int jits = GetNumberOfJittedMethods(jitOutputFile);
+
+        return jits > 0 && jits <= MAX_JITTED_METHODS_ACCEPTED ? 100 : 101;


Please note that I added a check that the number of JITted methods is nonzero. I firmly believe it's even beyond the scope of .NET 9 to achieve JIT runtime count being zero and, while I'll be the first person to celebrate the milestone, for now the zero value most likely means something has just malfunctioned. Please take it as a reviewer suggestion to consider.

I didn't know 0 meant an error in this case. Thanks for the observations Tomas!

Well, I'm not saying that zero is strictly wrong, a time may come when, say, "dotnet new webapi" will indeed runtime JIT 0 methods at startup (meaning that Crossgen2 precompilation has become "perfect" for this particular app), however that is not happening today and my feeling is that hitting zero today would much more likely signify an infra issue like the log not being generated or properly parsed. I'm looking forward to the time when 0 is the expected value and anything else is investigated as a Crossgen2 bug :-).

It will be part of the celebration. That day I adjust this test so that zero jitted methods can be a potentially expected result :)

trylek

I have made some tiny fixes around launching the managed app - in the Helix lab it's much easier and actually desirable to run managed apps via "corerun" than using "dotnet" especially as corerun is the freshly built thing, not some month-old preview drop; there were also a few tiny syntactic bugs but other than that your new test looks great to me, thanks Ivan!

jkoritzinsky · 2023-08-16T00:30:21Z

Is the generator warning on the HelloWorld project or on the merged test runner project?

If it's on the Hello World project, then the correct solution is to disable the reference to the generator from that project as it isn't a test project.

trylek · 2023-08-16T00:33:50Z

@jkoritzinsky - I believe it's no longer on the HelloWorld project, that gets now excluded via the CLRTestKind=BuildOnly clause I added a check for in src/tests/Directory.Build.targets (via _CLRTestProjectNeedsToRun). The problem is in the outer project (JittedMethodsCountingTest), I believe due to the fact that the ProjectReference it has to the HelloWorld isn't excluded from the generator based on the ReferenceOutputAssembly clause.

jkotas · 2023-08-16T04:58:22Z

src/tests/JIT/Performance/JittedMethodsCountingTest/JittedMethodsCountingTest.cs

+            Console.WriteLine("Launching: {0} {1}", startInfo.FileName, startInfo.Arguments);
+
+            startInfo.EnvironmentVariables.Add("DOTNET_JitDisasmSummary", "1");
+            startInfo.EnvironmentVariables.Add("DOTNET_TieredCompilation", "0");


Should this be testing the default config (ie DOTNET_TieredCompilation=1)?

ivdiazsa · 2023-08-16T18:31:00Z

Closing this one in favor of #90681.

ivdiazsa and others added 5 commits August 3, 2023 15:57

Added my first draft for the JIT methods counter tests.

c7c02a5

Began incorporating the HelloWorld with the test builds.

c7df757

Added the Jitted Methods counting test to JIT/Performance.

9b9bb10

Made more progress. Still doesn't work.

1713f45

trylek added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 15, 2023

trylek requested review from ivdiazsa, jkoritzinsky and markples August 15, 2023 23:20

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 15, 2023

ghost assigned trylek Aug 15, 2023

trylek added this to the 9.0.0 milestone Aug 15, 2023

trylek commented Aug 15, 2023

View reviewed changes

jkotas reviewed Aug 16, 2023

View reviewed changes

ivdiazsa mentioned this pull request Aug 16, 2023

Add New Test to Monitor Number of Jitted Methods #90681

Closed

ivdiazsa closed this Aug 16, 2023

trylek deleted the IvanDiaz-JitCountTest-InfraFixes branch September 5, 2023 19:27

ghost locked as resolved and limited conversation to collaborators Oct 6, 2023

[NO-MERGE / DESIGN DISCUSSION ONLY] Infra fixes for Ivan's JIT count test #90647

[NO-MERGE / DESIGN DISCUSSION ONLY] Infra fixes for Ivan's JIT count test #90647

Uh oh!

Conversation

trylek commented Aug 15, 2023

Uh oh!

ghost commented Aug 15, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trylek left a comment

Choose a reason for hiding this comment

Uh oh!

jkoritzinsky commented Aug 16, 2023

Uh oh!

trylek commented Aug 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivdiazsa commented Aug 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants