Add New Test to Monitor Number of Jitted Methods #90681

ivdiazsa · 2023-08-16T18:28:43Z

Solution of issue #88533. This PR adds a new test to the JIT/Performace tree to track the number of methods jitted at runtime in a predefined app.

The test's function is straightforward. It sets up an environment where we can run a dotnet app and capture the methods jitted at runtime into a file. In this PR, a HelloWorld app is enough to fulfill our purposes. Then, it runs the app and if everything goes fine, it reads the file with the jitted methods written down to see how many there were. Based on this number, the test's result is defined successful or not.

For further information, the initial discussion took place in Tomas' design-only PR #90647.

ivdiazsa · 2023-08-16T18:29:02Z

cc @jkotas

ghost · 2023-08-16T18:29:28Z

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

Issue Details

Solution of issue #88533. This PR adds a new test to the JIT/Performace tree to track the number of methods jitted at runtime in a predefined app.

The test's function is straightforward. It sets up an environment where we can run a dotnet app and capture the methods jitted at runtime into a file. In this PR, a HelloWorld app is enough to fulfill our purposes. Then, it runs the app and if everything goes fine, it reads the file with the jitted methods written down to see how many there were. Based on this number, the test's result is defined successful or not.

For further information, the initial discussion took place in Tomas' design-only PR #90647.

Author:	ivdiazsa
Assignees:	ivdiazsa
Labels:	`area-Infrastructure-coreclr`
Milestone:	-

jkotas · 2023-08-16T18:37:14Z

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

+            // with DOTNET_JitStdOutFile.
+
+            startInfo.EnvironmentVariables.Add("DOTNET_JitDisasmSummary", 1);
+            startInfo.EnvironmentVariables.Add("DOTNET_TieredCompilation", 0);


Why are we disabling tiered compilation for this test?

The HelloWorld test should be very short, perhaps we could monitor both. I agree it doesn't make much sense to run it under stress.

The runtime tests are run in multiple configurations (tiered compilation on/off, etc.). The test itself should not need to explicitly cover different configurations.

Sounds reasonable, in such case it should suffice to remove this environment variable override and the child process (corerun) should automatically inherit the parent settings.

Oh, I didn't know we already had the environments set. In that case, I will remove the TieredCompilation and ReadyToRun environment variables then. However, we will also need to exclude this test from non-R2R runs since the goal is to monitor the jitted methods when DOTNET_ReadyToRun is enabled.

I was thinking on this yesterday at home, and I came up with another alternative I think might be cleaner. How about checking for DOTNET_ReadyToRun at the beginning of the test, and if it's not enabled, end it right there returning 100? Also, print out a message saying the test is not going to run because it's not supported outside R2R-enabled contexts. What are your thoughts on this Tomas?

That sounds good to me, in fact it's likely going to be faster in the lab because this way your test can run in process of the merged wrapper and it will just finish immediately in the absence of DOTNET_ReadyToRun. Please just remember that the default is 1 so that you need to test whether it's been explicitly overridden to 0.

Sounds good. I was thinking maybe check whether it's different from 1 instead?

Whatever you prefer, just keep in mind that "" (the variable not being set) means "1", not "0".

Alright got it. I'll then make the test run only if DOTNET_ReadyToRun is set to "1" or not set at all, since that implicitly means "1" as well.

jkotas · 2023-08-16T18:40:37Z

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

+
+  <ItemGroup>
+    <Compile Include="JittedMethodsCountingTest.cs" />
+  </ItemGroup>


Are there configurations that this test needs to be disabled for? I am thinking about JIT stress, etc.

Yeah, adding <JitOptimizationSensitive>true</JitOptimizationSensitive> to the PropertyGroup seems like a good idea.

jkotas · 2023-08-16T18:40:55Z

cc @dotnet/jit-contrib

trylek · 2023-08-16T18:41:27Z

src/tests/JIT/Performance/JIT.performance.csproj

@@ -1,6 +1,7 @@
 <Project Sdk="Microsoft.NET.Sdk">
  <ItemGroup>
    <MergedWrapperProjectReference Include="*/**/*.??proj" />
+    <MergedWrapperProjectReference Remove="JittedMethodsCountingTest/HelloWorld/Helloworld.csproj" />


You can fix this more cleanly by modifying the following line:

runtime/src/tests/Directory.Build.targets

Line 513 in 7a11cff

<PropertyGroup Condition="'$(Language)' == 'C#' and ('$(BuildAsStandalone)' == 'true' or '$(RequiresProcessIsolation)' == 'true' or '$(InMergedTestDirectory)' == 'true' or '$(IsMergedTestRunnerAssembly)' == 'true')">

as follows:

<PropertyGroup Condition="'$(Language)' == 'C#' and $(_CLRTestNeedsToRun) and ('$(BuildAsStandalone)' == 'true' or '$(RequiresProcessIsolation)' == 'true' or '$(InMergedTestDirectory)' == 'true' or '$(IsMergedTestRunnerAssembly)' == 'true')">

Once you do that, it should suffice to state in your HelloWorld.csproj project that it's <CLRTestKind>BuildOnly</CLRTestKind> and it should do the same thing under the covers.

trylek · 2023-08-16T18:46:05Z

@jkoritzinsky - I was mistaken in my yesterday assessment, the error indeed comes from the compilation of HelloWorld as you expected, we don't check the transitive references for entry points. That ultimately simplified the change because we can just conditionally skip emitting the wrapper - we already have a few places in the tree that explicitly set ReferenceXUnitWrapperGenerator to false but that seemed a bit dirty to me so I proposed to Ivan to make a change to src/tests/D.B.targets that will suppress the generator for <CLRTestKind>BuildOnly</CLRTestKind> projects.

…eedback, and cleaned up some stuff, adding the CLRTestKind property to HelloWorld.csproj, as per Tomas' feedback.

jkotas · 2023-08-16T22:05:30Z

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

+
+    private static int GetNumberOfJittedMethods(string jitOutputFile)
+    {
+        string[] lines = File.ReadLines(jitOutputFile).ToArray();


Suggested change

string[] lines = File.ReadLines(jitOutputFile).ToArray();

string[] lines = File.ReadAllLines(jitOutputFile);

… minor fixes from received feedback.

trylek · 2023-08-17T22:36:41Z

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

+        {
+            File.Delete(jitOutputFile);
+        }
+
        // For adding any new apps for this test, make sure their success return
        // code is 0, so we can universally handle when they fail.
        int appResult = RunHelloWorldApp(appName, jitOutputFile);


Should we include a check for the appResult value in the expression for the exit code?

I don't think that would be necessary. We're already marking the test as failed and returning 101 if the app's exit code is different from 0 (i.e. something went wrong there).

trylek · 2023-08-17T22:38:36Z

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

+    private static bool IsReadyToRunEnvSet()
+    {
+        string? dotnetR2R = Environment.GetEnvironmentVariable("DOTNET_ReadyToRun");
+        return (dotnetR2R is null || dotnetR2R == "1") ? true : false;


I have a vague feeling (you probably know that better than I do) that on Linux you can end up with either null or empty string both of which should probably have the same behavior i.o.w. we should probably use string.IsNullOrEmpty (or return to my original lame suggestion that inequality to "0" is good enough).

It also returns null on Linux. However, since any kind of empty is equivalent to "1" in this case, we can play it safer by going the string.IsNullOrEmpty() route instead 👍

trylek

LGTM, thanks Ivan! I have shared just a couple of non-blocking nitpicks, overall your change looks great to me provided it actually passes in the lab.

… safer, as per Tomas' feedback.

AndyAyersMS · 2023-08-17T23:07:50Z

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

+
+public class JittedMethodsCountingTest
+{
+    private const int MAX_JITTED_METHODS_ACCEPTED = 120;


120 seems like a pretty high number to allow -- do we want to set this lower?

Empty app on Linux/macOS x64/arm64 is expected to produce just 1 jit compilation (for Main) 🙂
On windows there can be more due to background ETW sessions, but not too much (like 16 in total or so)

For Console.WriteLine it's tricky because System.Console is not prejitted in dotnet/runtime, hence, all methods in it are always jitted

I must admit I'm not too familiarized with the expected jitted methods counts. What would be a good expected range for a HelloWorld app like the one added in this project?

I think we just need a baseline and go from there. I recall that in my experiments with startup of the "dotnet new webapi" I observed about 50 methods being jitted at runtime when compiled as composite with the MIBC optimization data i.o.w. in more or less "the best possible configuration".

(Without the MIBC data it was almost 200 IIRC.)

We already have similar smoke-tests in dotnet/runtime, right? like that NAOT test where we check that an empty app's size is not terribly big in dotnet/runtime repo - is it real-world? No, but better than nothing and, afair, it already helped once. I agree that this test as is won't help us to find cases when something goes wrong but not terribly (e.g. 100 methods regressed). For that we still need to apply efforts elsewhere: I'd want to have a separate graph for "methods jitted" counter at https://aka.ms/aspnet/benchmarks so once startup regresses again I will be able at least quickly validate that it's not because of some obvious R2R issue. A visualization on dotnet/performance would work too.

tldr: the test is ok to have IMO, additional tests in dotnet/performance and PerfLab would be nice to have too (where we work with SDK)

As for where the runtime repo version should be placed, considering what you explained about src/tests/JIT/Performance, then I agree it's not the best place. In that case, where would you suggest placing it?

I agree that "JIT/Performance" is not the right place.

How about src\tests\readytorun...?

Why didn't you provide these ideas when you gave your first feedback?

I have provided similar feedback in the issue discussion - look for my comments in #88533.

the test is ok to have IMO

Agree - this test as a smoke test that will catch things going terribly wrong. It is not a perf gate.

We already have similar smoke-tests in dotnet/runtime, right? like that NAOT test where we check that an empty app's size is not terribly big in dotnet/runtime repo - is it real-world? No, but better than nothing and, afair, it already helped once. I agree that this test as is won't help us to find cases when something goes wrong but not terribly (e.g. 100 methods regressed). For that we still need to apply efforts elsewhere: I'd want to have a separate graph for "methods jitted" counter at https://aka.ms/aspnet/benchmarks so once startup regresses again I will be able at least quickly validate that it's not because of some obvious R2R issue. A visualization on dotnet/performance would work too.

tldr: the test is ok to have IMO, additional tests in dotnet/performance and PerfLab would be nice to have too (where we work with SDK)

Thanks for your feedback @EgorBo. I agree having a visual graph would certainly help us catch regressions-in-the-making sooner. Then I have a follow up question for you and @AndyAyersMS. What would be a good number of jitted methods that if exceeded, we can confirm something went terribly wrong and therefore fail this test?

As for the dashboard and graphs, I will reach out to Sebastien to get it to run periodically in TE alongside the other perf tests we already run (e.g. ASP.NET Composite Containers).

How about src\tests\readytorun...?

I think this is a good place to put this one @BruceForstall. I'll move it and rerun the lab tests.

What would be a good number of jitted methods that if exceeded, we can confirm something went terribly wrong and therefore fail this test?

It should be the lowest number that makes the test pass reliably with some margin. What is the highest number of methods that you see being JITed across different configurations? Add 20 to it and use that as the limit.

I will reach out to Sebastien to get it to run periodically in TE alongside the other perf tests we already run (e.g. ASP.NET Composite Containers)

A trivial Console.WriteLine startup perf test has nothing to do with ASP.NET. It does not sound right to add it to ASP.NET perf infra for TE.

We have startup-oriented scenario perf measurements under https://github.com/dotnet/performance/tree/main/src/tools/ScenarioMeasurement. We should have a guidance for when to add a scenario measurement to this location vs. to the TE infrastructure. Please check with Sebastien and Scott Blomquist about that.

AndyAyersMS · 2023-08-17T23:10:36Z

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

+
+  <ItemGroup>
+    <Compile Include="JittedMethodsCountingTest.cs" />
+  </ItemGroup>


Yeah, adding <JitOptimizationSensitive>true</JitOptimizationSensitive> to the PropertyGroup seems like a good idea.

…uce's feedback.

ivdiazsa · 2023-08-21T19:00:52Z

/azp run runtime-coreclr outerloop

azure-pipelines · 2023-08-21T19:01:09Z

Azure Pipelines successfully started running 1 pipeline(s).

ivdiazsa · 2023-08-22T17:27:09Z

Hmmm seems like the test can't find the file in the lab runs. Looking into it right now.

markples · 2023-08-22T17:45:14Z

@ivdiazsa This sounds like the kind of thing where the specialized test logic does something in a subdirectory. Perhaps the current working directory and test binary directory don't match and the write/read of the file then don't match?

ivdiazsa · 2023-08-22T18:13:51Z

@ivdiazsa This sounds like the kind of thing where the specialized test logic does something in a subdirectory. Perhaps the current working directory and test binary directory don't match and the write/read of the file then don't match?

That seems to be the case, although I'm having trouble figuring out why. I can't seem to reproduce it on my machine so it must be some Helix configuration that is messing up with the paths. I'll try using the current working directory explicitly as reference instead of assuming and hopefully that should help.

trylek · 2023-08-22T18:59:16Z

src/tests/readytorun/JittedMethodsCountingTest/JittedMethodsCountingTest.cs

+        }
+
+        string appName = "HelloWorld.dll";
+        string jitOutputFile = "jits.txt";


I think you should generate a full path akin to what I did in #90647,

string testAppFolder = Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location); string appName = Path.Combine(testAppFolder, "HelloWorld.dll"); string jitOutputFile = Path.Combine(testAppFolder, "jits.txt");

The scripts may be tampering with the current directory.

Thanks for the suggestion Tomas! Will try it this way. I don't know why I didn't bring those changes here. Must've slipped through the cracks.

…supposedly there. Added an 'ls -R' command to see its tree structure and try to figure out why it can't find the file.

…ds, so switched to a ';' instead.

ivdiazsa · 2023-08-25T21:45:57Z

/azp run runtime-coreclr outerloop

azure-pipelines · 2023-08-25T21:46:09Z

Azure Pipelines successfully started running 1 pipeline(s).

ivdiazsa added 7 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

Clean up.

e0080e9

Added information to the test as per Tomas' feedback.

474d0aa

Forgot to uncomment some code.

908e50e

ivdiazsa requested review from jkoritzinsky, trylek and mangod9 August 16, 2023 18:28

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 16, 2023

ghost assigned ivdiazsa Aug 16, 2023

ivdiazsa added area-Infrastructure-coreclr and removed area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Aug 16, 2023

dotnet deleted a comment Aug 16, 2023

ivdiazsa added this to the 9.0.0 milestone Aug 16, 2023

ivdiazsa linked an issue Aug 16, 2023 that may be closed by this pull request

R2R: Add a CI or perf validation for # of methods jitted #88533

Closed

ivdiazsa mentioned this pull request Aug 16, 2023

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

Closed

jkotas reviewed Aug 16, 2023

View reviewed changes

trylek reviewed Aug 16, 2023

View reviewed changes

ivdiazsa added 2 commits August 16, 2023 13:22

Removed the ReadyToRun and TieredCompilation env vars as per JanK's f…

3707cf8

…eedback, and cleaned up some stuff, adding the CLRTestKind property to HelloWorld.csproj, as per Tomas' feedback.

Fixed a couple semantic errors I overlooked in my previous commit.

eec4acb

jkotas reviewed Aug 16, 2023

View reviewed changes

build-analysis bot mentioned this pull request Aug 17, 2023

Tracking issue for CI build timeouts #76454

Closed

ivdiazsa added 2 commits August 17, 2023 11:10

Merge branch 'main' into jit-participants

8fa21fb

Made the test only run in ReadyToRun environments, and addressed some…

7d36e14

… minor fixes from received feedback.

trylek reviewed Aug 17, 2023

View reviewed changes

trylek approved these changes Aug 17, 2023

View reviewed changes

build-analysis bot mentioned this pull request Aug 17, 2023

BinderTracingTest_ResolutionFlow test fails #90593

Closed

Replaced the 'is null' check with 'string.IsNullOrEmpty()' to play it…

34b0ded

… safer, as per Tomas' feedback.

AndyAyersMS reviewed Aug 17, 2023

View reviewed changes

Moved the JittedMethodsCountingTest to src/tests/readytorun as per Br…

1c9684c

…uce's feedback.

trylek reviewed Aug 22, 2023

View reviewed changes

Changed app paths to be absolute instead of relative.

6029d89

This was referenced Aug 23, 2023

Insufficient memory of docker containers on CI dotnet/dnceng#450

Open

Build Failure on MacOS ARM64 Rid issue with sqlite #90434

Closed

ivdiazsa added 3 commits August 23, 2023 14:56

DEBUG-ONLY CHANGE: Helix just plain refuses to find the file that is …

04d88e2

…supposedly there. Added an 'ls -R' command to see its tree structure and try to figure out why it can't find the file.

DEBUG-ONLY CHANGE: Helix seems to not like '&&' to concatenate comman…

e3b96c8

…ds, so switched to a ';' instead.

DEBUG-ONLY CHANGE: Maybe this will print the tree?

f300f88

dotnet deleted a comment from azure-pipelines bot Aug 25, 2023

ivdiazsa closed this by deleting the head repository Aug 28, 2023

ghost locked as resolved and limited conversation to collaborators Sep 27, 2023

	string[] lines = File.ReadLines(jitOutputFile).ToArray();
	string[] lines = File.ReadAllLines(jitOutputFile);

Add New Test to Monitor Number of Jitted Methods #90681

Add New Test to Monitor Number of Jitted Methods #90681

Conversation

ivdiazsa commented Aug 16, 2023

ivdiazsa commented Aug 16, 2023

ghost commented Aug 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivdiazsa Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Aug 16, 2023

trylek Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

trylek commented Aug 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trylek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EgorBo Aug 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivdiazsa commented Aug 21, 2023

azure-pipelines bot commented Aug 21, 2023

ivdiazsa commented Aug 22, 2023

markples commented Aug 22, 2023

ivdiazsa commented Aug 22, 2023

Choose a reason for hiding this comment

ivdiazsa Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

ivdiazsa commented Aug 25, 2023

azure-pipelines bot commented Aug 25, 2023

ivdiazsa Aug 16, 2023 •

edited

Loading

trylek Aug 16, 2023 •

edited

Loading

EgorBo Aug 18, 2023 •

edited

Loading

ivdiazsa Aug 22, 2023 •

edited

Loading