{{ message }}
Move performance testing YAML from dotnet/runtime to dotnet/performance#4639
Merged
caaavik-msft merged 48 commits intoFeb 3, 2025
Merged
Conversation
ghost
reviewed
Jan 16, 2025
ghost
reviewed
Jan 16, 2025
ghost
reviewed
Jan 16, 2025
ghost
reviewed
Jan 16, 2025
ghost
reviewed
Jan 16, 2025
LoopedBard3
reviewed
Jan 17, 2025
LoopedBard3
left a comment
Member
There was a problem hiding this comment.
General high level functionality check seems to look good to me, will take a closer look at changes individually soon.
Contributor
Author
kotlarmilos
reviewed
Jan 22, 2025
LoopedBard3
previously approved these changes
Jan 30, 2025
Member
There was a problem hiding this comment.
This looks good to me, and everything seems to be accounted for. For the runtime-perf-job.yml question on what should go where, I don't have a strong preference for a particular approach. I think keeping as much in the performance repo, even if strongly coupled to dotnet/runtime, is preferable from a retesting capabilities standpoint.
Contributor
There was a problem hiding this comment.
Copilot reviewed 7 out of 19 changed files in this pull request and generated no comments.
Files not reviewed (12)
- eng/performance/benchmark_jobs.yml: Language not supported
- eng/performance/gc_jobs.yml: Language not supported
- eng/performance/scenarios.yml: Language not supported
- eng/performance/send-to-helix.yml: Language not supported
- eng/pipelines/runtime-perf-jobs.yml: Evaluated as low risk
- eng/pipelines/runtime-slow-perf-jobs.yml: Evaluated as low risk
- eng/pipelines/runtime-wasm-perf-jobs.yml: Evaluated as low risk
- eng/pipelines/templates/build-machine-matrix.yml: Evaluated as low risk
- eng/pipelines/templates/download-artifact-step.yml: Evaluated as low risk
- eng/pipelines/runtime-ios-scenarios-perf-jobs.yml: Evaluated as low risk
- scripts/run_performance_job.py: Evaluated as low risk
- eng/pipelines/templates/run-performance-job-script-step.yml: Evaluated as low risk
DrewScoggins
approved these changes
Feb 3, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

This PR implements the dotnet/performance portion of the work to move the performance testing YAML to do the dotnet/performance repository. You can find the corresponding change for the dotnet/runtime repository here: dotnet/runtime#111454.
Summary
The goal of this work is to move as much of the CI logic as possible out of the dotnet/runtime repository and into the dotnet/performance repository. The reason we want to make this change is to decouple what is being tested from the code that does the testing.
We have had many occurrences over the last few years where there would be a bug in the performance testing logic that caused invalid/skipped data or we have had to make a breaking change. Having this logic live in the dotnet/runtime repository meant we were unable to re-run performance tests when the bug was present or before a breaking change was made. With the changes from this PR, it means that if we ever have a bug that causes invalid/skipped data, we can make the change in the dotnet/performance repo and then re-run the performance tests with those fixes and correct any data issues.
Implementation
It is not practical to move everything out of the dotnet/runtime repository as there are some steps of our performance tests that may be strongly coupled to the version of the runtime. In particular, these would be any jobs that involve building the runtime or building test applications. To handle this, we have made extensive use of cross-repository template references.
If you look at the dotnet/runtime portion of this PR, you will see that
eng/pipelines/coreclr/perf.ymlwill include the following lines:There is a repository resource named
performancewhich describes where the performance repository is located. For more information about repository resources, see the documentation here. Then we are able to reference a template from the performance repository by putting@performanceat the end. When manually running the pipeline in Azure DevOps, under "Advanced options" there is a section called Resources which allows you to customise a branch/commit of the performance repository you want to run against, which is useful when testing changes that need to be made against both the runtime and performance repositories.If you look at
/eng/pipelines/runtime-perf-jobs.ymlin this PR, you can see the following line:This line is how the performance repository is able to reference back into the runtime repository and call the perf-build-jobs.yml template inside it. In the dotnet/runtime repository PR, it sets
runtimeRepoAliastoselfwhich means that it will reference the exact version of the runtime repository that called it. In a future PR, we will be able to add an additional pipeline to the dotnet/performance repository which will has aruntimerepository resource defined so that we can verify that a PR isn't going to cause the runtime performance tests to break.YAML Convergence
Before this PR, there was a lot of duplicated but slightly differing YAML files that did similar things. As part of this PR we are able to unify all this duplicate logic so that everything is only defined once and doesn't require making changes in lots of files. In particular I want to point out the following two files which are worth paying close attention to (both in the
/eng/pipelines/templates/directory):run-performance-job.yml: Defines a job that clones the runtime and performance repositories, runs the performance job python script, and sends the helix job. Every performance test will go through this YAML file including those that run against the SDK or a build of the runtime.runtime-perf-job.yml: A wrapper aroundrun-performance-job.ymlwhich sets up all the necessary parameters and additional steps for running performance tests against a build of the runtime. Most of this is just downloading the build artifacts and arranging them into the correct directories so that they can be used by the python scripts.Other Changes
Performance osx x64 release iOSMono JIT ios_scenarios perfiphone12mini NoJS True False True net10.0Performance ios_scenarios iOSMono JIT iOSLlvmBuild osx x64 perfiphone12mini net10.0additionalJobIdentifierparameter can be specified as extra information to include in the job name if needed to disambiguate two jobs.eng/pipelines/coreclr/perf.ymlyou will see I added a parameter calledonlySanityCheckwhich uses the newjobParametersparameter to set theonlySanityCheckparameter on every job that gets run.Validation
Since this change is large, it is likely to introduce bugs. To validate that things have been ported correctly, I looked at the
Run performance job scriptstep of the job to see the command line arguments that were passed to the python script to ensure that they are identical. There are still potentially other classes of bugs due to variables being different which are harder to detect, but from my testing I expect any bugs to be minimal.We should also address in a future PR ensuring that
onlySanityCheckis properly implemented for all scenarios and test cases as it is only be active for our microbenchmarks. This should be fine for now though as our microbenchmarks are the ones that take the longest.