-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add mono performance runs #34825
Merged
Merged
Add mono performance runs #34825
Changes from 20 commits
Commits
Show all changes
38 commits
Select commit
Hold shift + click to select a range
da62751
WIP
DrewScoggins 52f6d0e
Add mono perf job
DrewScoggins 11ec5af
Move runtimeFlavor to the right level
DrewScoggins 4e4ac5a
chnage variable access
DrewScoggins ab9008e
Change condition
DrewScoggins 15a0eda
Add steps to download mono and build the patched dotnet
DrewScoggins d03fead
Change variable access
DrewScoggins f0a8eb6
Pass variables directly
DrewScoggins 623ec9b
Plumb through mono path to runner
DrewScoggins edec8c8
Fix parameter reference
DrewScoggins e0d0def
Fix variables access?
DrewScoggins 60f4dcf
Fix typo
DrewScoggins 6f09285
Fix pathing issue
DrewScoggins f30649e
Add log saving for publish to helix step
DrewScoggins 94936e9
Fix variable in shell script
DrewScoggins f7f1043
Fix up mv command and publish logs step
DrewScoggins e90947b
Change runkind and add Windows runs
DrewScoggins 01dc157
Add Windows mono build
DrewScoggins f959d0e
Fixup parsing
DrewScoggins 1becb9e
Remove Windows runs for now
DrewScoggins d69da0f
Switch to using build command to generate mono testhost
DrewScoggins 6ae7909
Fixup parens
DrewScoggins 58f8fde
Forgot to add a slash
DrewScoggins 6904d33
Add Windows runs and fix pathing issue
DrewScoggins 55f4acc
Add backslash for Windows and -r for cp on Linux
DrewScoggins 08ffc75
Switch to using corerun argument
DrewScoggins 163a1de
Switch to monopath
DrewScoggins 7ca0b35
Use CoreRun
DrewScoggins 82fa61e
Remove mono property and use corerun instead
DrewScoggins 9a973a4
Fixing closing tags
DrewScoggins edf722f
Change xcopy to copy
DrewScoggins 1c5c874
Add llvm config flag
DrewScoggins 19448eb
Add AOT and Interpreter config flags
DrewScoggins 08af789
Change order to ensure config gets added
DrewScoggins 21ee8fa
Change order
DrewScoggins bb816d2
Add exlusion filter for Perf_Image_Load tests
DrewScoggins 7916bd3
Add more to exclusion list
DrewScoggins b7e7a6c
Remove Windows legs for now
DrewScoggins File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should do this. It'd be better to just use self-contained build
/cc @directhex @akoeplinger
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this a little more? Why is using the patched mono dotnet not a good path forward? This is similar to what we do for the coreclr based runtime tests.
Also, the way that we currently run performance tests relies on having a version of dotnet to build and run the Benchmark DotNet tests, and as far as I know we cannot do a self-contained publish of the performance tests. (@billwert for confirmation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been doing that for months on my infrastructure so at least in theory it's possible. On the other hand it's not easy and useful for most cases because BDN by default triggers the compilation for the benchmarks and runs each of them out of process.
For non-AOT benchmarks I ended up using/patching
testhost
that is produced as output of the dotnet/runtime repository. It it guaranteed to be self-consistent unlike the combination of SDK version determined by global.json and the runtime built from the repository.For AOT I rely on self-publish and in-process mode of BDN. Not sure if I would recommend that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not set up we'll ever officially support and can break anything as it was a temporary hack. It also won't support the official AOT mode and possibly other configs. So if you are testing this hack then you are testing config which no customer will use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some context, we run the performance testing for the coreclr runtime in two ways. The first is on the pacakges from the installer repo. This is to make sure that we are running the performance tests in a way that the customer will actually use the product. We also run the tests here in the runtime repo by building the product and then making a corerun directory that contains all of our built binaries. This, while not the way our customers use or obtain the product, allows us to get much more granular performance data collected as we don't have to wait for code to flow all the way from here to the installer repo.
This work was designed to get mono to parity with what we are doing here in the runtime repo. So building the product from head and then making up a version of dotnet that contains the built binaries and running the tests. If using the dotnet-mono patch feature is not the right way to do this, please direct me to the canonical way to do this. I was just operating off of the steps given to me by @SamMonoRT.
@filipnavara:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it seems like a good place to land here is to move the performance runs over to the testhost construction, as that seems to be what y'all have been doing in the past. We can then go ahead and get this checked in and running for Windows and Linux and for LLVM and non-LLVM backend. Then we can file an issue that tracks doing the work to move over to the in tree runtime packs as soon as we have good support for that on desktop builds. Does that sound like a good way forward to everyone here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me - @steveisok @marek-safar thoughts ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AOT won't work this way for sure and any issues with this setup won't be fixed. I'm fine to land this but we should start working on a supported way to run the tests as this can break anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I did some digging and this is the step that seems to setup the testhost for running functional tests on mono, link. I would assume that since this is how the product is tested in CI that this should be a supported scenario, yes?
If this is the case I am just going to use the same command to setup the testhost and run the performance tests in this way for now.
If this is also unsupported does that mean that the functional CI tests are running in an unsupported manner or am I just missing something here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's as close to supported desktop scenario as it is currently available (until desktop runtime packs are introduced).