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.

Sign up for GitHub

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

Jump to bottom

Add New Test to Monitor Number of Jitted Methods #90681

Closed
wants to merge 17 commits into from
Closed

Add New Test to Monitor Number of Jitted Methods #90681

Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
c7c02a5
Added my first draft for the JIT methods counter tests.
ivdiazsa Aug 3, 2023
c7df757
Began incorporating the HelloWorld with the test builds.
ivdiazsa Aug 11, 2023
9b9bb10
Added the Jitted Methods counting test to JIT/Performance.
ivdiazsa Aug 11, 2023
1713f45
Made more progress. Still doesn't work.
ivdiazsa Aug 14, 2023
e0080e9
Clean up.
ivdiazsa Aug 16, 2023
474d0aa
Added information to the test as per Tomas' feedback.
ivdiazsa Aug 16, 2023
908e50e
Forgot to uncomment some code.
ivdiazsa Aug 16, 2023
3707cf8
Removed the ReadyToRun and TieredCompilation env vars as per JanK's f…
ivdiazsa Aug 16, 2023
eec4acb
Fixed a couple semantic errors I overlooked in my previous commit.
ivdiazsa Aug 16, 2023
8fa21fb
Merge branch 'main' into jit-participants
ivdiazsa Aug 17, 2023
7d36e14
Made the test only run in ReadyToRun environments, and addressed some…
ivdiazsa Aug 17, 2023
34b0ded
Replaced the 'is null' check with 'string.IsNullOrEmpty()' to play it…
ivdiazsa Aug 17, 2023
1c9684c
Moved the JittedMethodsCountingTest to src/tests/readytorun as per Br…
ivdiazsa Aug 18, 2023
6029d89
Changed app paths to be absolute instead of relative.
ivdiazsa Aug 22, 2023
04d88e2
DEBUG-ONLY CHANGE: Helix just plain refuses to find the file that is …
ivdiazsa Aug 23, 2023
e3b96c8
DEBUG-ONLY CHANGE: Helix seems to not like '&&' to concatenate comman…
ivdiazsa Aug 24, 2023
f300f88
DEBUG-ONLY CHANGE: Maybe this will print the tree?
ivdiazsa Aug 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 28 additions & 4 deletions src/tests/JIT/Performance/JittedMethodsCountingTest/JittedMethodsCountingTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,30 @@

public class JittedMethodsCountingTest
{
private const int MAX_JITTED_METHODS_ACCEPTED = 50;
private const int MAX_JITTED_METHODS_ACCEPTED = 120;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

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 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?

Copy link
Member

Choose a reason for hiding this comment

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

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".

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@EgorBo EgorBo Aug 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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...?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.


[Fact]
public static int TestEntryPoint()
{
// If DOTNET_ReadyToRun is disabled, then this test ought to be skipped.
if (!IsReadyToRunEnvSet())
{
Console.WriteLine("\nThis test is only supported in ReadyToRun scenarios."
+ " Skipping...\n");
return 100;
}

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

// DOTNET_JitStdOutFile appends to the file in question if it exists.
// This can potentially cause issues, so we have to make sure it is
// created exclusively for this test's purpose.
if (File.Exists(jitOutputFile))
{
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);
Copy link
Member

Choose a reason for hiding this comment

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

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

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 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).

Expand All @@ -34,6 +50,12 @@ public static int TestEntryPoint()
return jits > 0 && jits <= MAX_JITTED_METHODS_ACCEPTED ? 100 : 101;
}

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

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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 👍

}

private static int RunHelloWorldApp(string appName, string jitOutputFile)
{
int appExitCode = -1;
Expand Down Expand Up @@ -61,8 +83,8 @@ private static int RunHelloWorldApp(string appName, string jitOutputFile)
startInfo.EnvironmentVariables.Add("DOTNET_JitDisasmSummary", "1");
startInfo.EnvironmentVariables.Add("DOTNET_JitStdOutFile", jitOutputFile);

Console.WriteLine("Launching Test App: {0} {1}", startInfo.FileName,
startInfo.Arguments);
Console.WriteLine("\nLaunching Test App: {0} {1}", startInfo.FileName,
startInfo.Arguments);

app.StartInfo = startInfo;
app.Start();
Expand All @@ -78,7 +100,7 @@ private static int RunHelloWorldApp(string appName, string jitOutputFile)

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

// Print out the jitted methods from the app run previously. This is
// mostly done as additional logging to simplify potential bug investigations
Expand All @@ -99,6 +121,8 @@ private static int GetNumberOfJittedMethods(string jitOutputFile)

string[] tokens = lines.Last().Split(":");
int numJittedMethods = Int32.Parse(tokens[0]);
Console.WriteLine("Total Jitted Methods: {0}\n", numJittedMethods);

return numJittedMethods;
}
}
Expand Down
Loading