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

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

Closed
mangod9 opened this issue Jul 7, 2023 · 13 comments · Fixed by #91235
Closed

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

mangod9 opened this issue Jul 7, 2023 · 13 comments · Fixed by #91235

Comments

@mangod9
Copy link
Member

mangod9 commented Jul 7, 2023

We have recently seen more methods being JITed for simple "helloworld" application, so getting visibility and ensuring we can minimize proliferation of these via automation would be helpful. Note that we are continuously working on improving R2R handling of new functionality, but always good to track this metric.

@mangod9 mangod9 added this to the 8.0.0 milestone Jul 7, 2023
@EgorBo
Copy link
Member

EgorBo commented Jul 7, 2023

note that hello world requires System.Console that is not prejitted in dotnet/runtime so maybe we instead can just test a completely blank app for simplicity

@jkotas
Copy link
Member

jkotas commented Jul 7, 2023

Should we measure this in the perf lab, in the same way as we are tracking other micro benchmarks?

It would be a good idea to track this for more than just a basic console app.

@EgorBo
Copy link
Member

EgorBo commented Jul 7, 2023

Should we measure this in the perf lab, in the same way as we are tracking other micro benchmarks?

It would be a good idea to track this for more than just a basic console app.

Sure, it's just that "hello world" in dotnet/runtime can block PRs which break R2R early - would not even allow a contributor to push such a change.

@jkotas
Copy link
Member

jkotas commented Jul 7, 2023

I do not think we would use this as a strict gate for blocking PRs.

@mangod9
Copy link
Member Author

mangod9 commented Jul 7, 2023

Perhaps we should have simple validation that majority of R2R methods are being used, and visualize #-of-methods jitted metric for existing benchmarks to trend across previews.

@ivdiazsa
Copy link
Member

I have a proposal for this problem. Firstly, let's choose which type of app we're going to test first (blank app?). We can always add more apps in the future as deemed necessary. Then, let's run a few experiments to see how many jitted methods we expect. From there, we can write the test that will fail if it finds more methods (one or range?) than expected.

However, we need this test to be non-blocking as it's not a big breaking deal, unless it's a huge regression. This, considering our .NET codebase is constantly changing and evolving.

As for the perf lab, I think that is a very good idea. We can consider that the next step after getting the number-of-jitted-methods test up and running.

I would like to hear everyone's thoughts on this @mangod9 @jkotas @trylek @EgorBo

@EgorBo
Copy link
Member

EgorBo commented Jul 17, 2023

Few notes:

  1. It seems that it's ok when we sometimes introduce more non-prejittable methods, e.g. extensive use of Static Virtual Methods (SVM) did so since R2R doesn't support them yet (PR is up?)
  2. Like Jan mentioned - the random ETW sessions problem (presumably, can be fixed for a test app by disabling events, but that will make it less real-world, right?)
  3. If something goes terribly wrong we might see > 1000 methods jitted for a blank app/hello world

It happened two times already and the first sign was a significant regression in startup time/time to first request in TE benchmarks (we probably want to add a graph for "methods jitted" there).

@jkotas
Copy link
Member

jkotas commented Jul 18, 2023

It happened two times already and the first sign was a significant regression in startup time/time

I do not think there is a way to disable ETW events in CoreCLR today.

that will make it less real-world, right?

The real-world performance is always influenced by many factors that produce too much noise. It is why we are running performance benchmarks in an isolated environments so that we can get stable results. It is not real world, but it is something that we can reason about.

It happened two times already and the first sign was a significant regression in startup time

Do you have links to these two instances? We should validate that any proposals here would be actually effective in catching these two regressions early.

@jkotas
Copy link
Member

jkotas commented Jul 18, 2023

As for the perf lab, I think that is a very good idea. We can consider that the next step after getting the number-of-jitted-methods test up and running.

What are the benchmarks that we track startup time for in the perflab? I think that the number of methods JITed should be tracked for the exact same set of benchmarks.

@EgorBo
Copy link
Member

EgorBo commented Jul 18, 2023

Do you have links to these two instances? We should validate that any proposals here would be actually effective in catching these two regressions early.

in both cases there was a change that silently broke r2r

@jkotas
Copy link
Member

jkotas commented Jul 18, 2023

These two changes broke r2r on x86 machines without avx2. adm64 and x86 machines with avx2 were not affected as far as I know. In order to catch these two breakages, we would need to run this test on a machine without avx2.

@ivdiazsa ivdiazsa modified the milestones: 8.0.0, 9.0.0 Aug 10, 2023
@ivdiazsa
Copy link
Member

Moving this to .NET 9 since most infrastructure improvements, like this case, shouldn't block releases.

@ghost
Copy link

ghost commented Aug 10, 2023

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

Issue Details

We have recently seen more methods being JITed for simple "helloworld" application, so getting visibility and ensuring we can minimize proliferation of these via automation would be helpful. Note that we are continuously working on improving R2R handling of new functionality, but always good to track this metric.

Author: mangod9
Assignees: ivdiazsa
Labels:

area-Infrastructure-coreclr, area-crossgen2-coreclr

Milestone: 9.0.0

@ivdiazsa ivdiazsa linked a pull request Aug 16, 2023 that will close this issue
@ivdiazsa ivdiazsa linked a pull request Aug 28, 2023 that will close this issue
@ghost ghost locked as resolved and limited conversation to collaborators Oct 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants