Add k-nucleotide to BenchmarkGames#9293
Conversation
|
Hi @guhuro, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! TTYL, DNFBOT; |
|
Please take a look. @DrewScoggins @brianrob @valenis |
|
@guhuro, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
| { | ||
| using (iteration.StartMeasurement()) | ||
| { | ||
| for (int i = 0; i < Iterations; ++i) |
There was a problem hiding this comment.
You should set the InnerIterationsCount via the BenchmarkAttribute. This will allow the harness to be aware of the count, but won't change the execution of the program. You can still keep the constants above, and just use them in the attribute. You can see an example of how to use InnerIterationsCount at https://github.com/microsoft/xunit-performance.
There was a problem hiding this comment.
You can blame me for that; I was looking for a pattern where the benchmarks would be usable running under xunit-perf and also running as standalone perf tests, and I think at the time there was no InnerIterationsCount.
There was a problem hiding this comment.
No worries. InnerIterationsCount is relatively new.
| { | ||
| for (int i = 0; i < Iterations; ++i) | ||
| { | ||
| PrepareLookups(); |
There was a problem hiding this comment.
Do you want to measure all of this code? You can put some of this outside of the StartMeasurement block if it's just prep that you don't want to measure.
| // and CoreRoot points at the test overlay dir | ||
| string[] pathPartsNormal = new string[] { | ||
| CoreRoot, "..", "..", "JIT", "Performance", | ||
| "CodeQuality", "BenchmarksGame", "k-nucleotide", inputFile |
There was a problem hiding this comment.
You need to have "k-nucleotide" in here twice.
Also @wtgodbe is working on changing how all this input file handling works, so sync up with him.
There was a problem hiding this comment.
@guhuro see #9281 - we need to change the way we're finding the input files. We can't rely on them always being in the same place relative to CORE_ROOT, since in Helix, the directory structure assumed here isn't maintained - so tests like this one are failing there (because of that assumption). I've tried looking relative to the executing assembly, but that doesn't work, since the executing assembly is CoreRun.exe. I've also tried finding the directory of the assembly defined in the class itself, but that didn't work either, because for whatever reason we don't have access to the ".Location" member of the "Assembly" class (not sure why that is...). If you have any bright ideas, I'm all ears - otherwise I'm still digging to find a solution.
There was a problem hiding this comment.
The reason that Assembly.Location does not work for at least the JIT perf tests is because they are build against .NETStandard 1.4, and that API was not added until 1.5. We build against 1.4 because we currently use a desktop harness, with corerun, to execute the tests. We would need to more to an all core solution for running the tests before we can move forward on what we are targeting for these tests.
There was a problem hiding this comment.
@DrewScoggins are you talking about Desktop.Coreclr.Testwrapper? I already have the tests running using the core xunit wrapper in Helix, and have an issue against myself to do the same in all cases - I could put that together today, if it'd solve this problem as well.
There was a problem hiding this comment.
@guhuro actually, don't worry about fixing finding the input file for now - I talked with @DrewScoggins, and we can't do that until we're able to move perf tests away from netstandard1.4, which we can't now. I've pre-emptively disabled this test for Helix (#9315), and opened an issue to re-enable it when possible (https://github.com/dotnet/coreclr/issues/9314).
|
|
||
| static string FindInput(string s) | ||
| { | ||
| string CoreRoot = System.Environment.GetEnvironmentVariable("CORE_ROOT"); |
There was a problem hiding this comment.
Are you guaranteed to have CORE_ROOT set?
|
Also, make sure you take a look at the CI failures. It looks like the input file wasn't found (at least for the OSX leg). You can see the full log at https://ci.dot.net/job/dotnet_coreclr/job/master/job/checked_osx_tst_prtest/2079/consoleText. |
|
I am curious as to why we are even running the JIT Performance tests during a functional test run? It seems like they don't really do much good and just add another place that these tests can break in. |
|
It is important to know that these tests actually pass. And it is much simpler for jit devs if they're built along with all the other tests, and can easily be run manually with various diagnostic switches and such. And they don't run for very long when run as standalone tests, by default (at least they're not supposed to), so there's no real impact on overall test throughput. |
|
Makes sense to me. |
|
Agreed with @AndyAyersMS on this. Ideally we have all of the perf tests run as functional tests (they just run 1 iteration by default), and that way breaks are caught in the functional runs. People understand that breaking a functional test is a no-no, but perf tests are currently hidden behind the scenes, which will just mean that we have to track down and fix breaks later on. |
| } | ||
|
|
||
| [Benchmark] | ||
| [Benchmark(InnerIterationsCount=Iterations)] |
There was a problem hiding this comment.
You'll also want to change the inner for loop to use the InnerIterationsCount:
for(int i=0; i<Benchmark.InnerIterationCount; i++)
There was a problem hiding this comment.
Great, I'll change that. There's also a typo, already fixed.
| return inputPath; | ||
| } | ||
|
|
||
| public static void Main(string[] args) |
There was a problem hiding this comment.
Main needs to be public static int and return 100 for success, -1 for failure.
Since you're currently not validating the results you can just return 100 from Main always. Though it might be nice to have the various Write methods return counts and check those against the correct values (which will differ for the different input files)
There was a problem hiding this comment.
I'll change that and add the checking.
There was a problem hiding this comment.
Yes, this is good. Having perf tests be functional tests as well helps us make sure that we aren't looking at perf results for functionally different code.
|
There are different input files depending on how the test is compiled -- so won't the validation checks depend on which input is actually read? |
|
Changed the main function so that the tests properly check the correct input file. |
|
Test Windows_NT_x64 perf |
|
All the tests passed, and it uploads to benchview correctly. |
|
LGTM |
brianrob
left a comment
brianrob
left a comment
There was a problem hiding this comment.
Just one question, but otherwise, LGTM.
| } | ||
|
|
||
| [Benchmark(InnerIterationCount=Iterations)] | ||
| public static void Bench() |
There was a problem hiding this comment.
Can you confirm that these methods show up with nice names in Benchview, so that it's clear what they are?
There was a problem hiding this comment.
They are listed behind knucleotide/.
Changed the names to Bench_Parallel and Bench_No_Parallel so that the difference is more clear.
Thanks!
Changed .gitattributes to set line endings to windows ones for the input files of this test. Added two different tests, one with and the other without parallelism. Used FileStream instead of BufferedStream, since the latter does not exist in netstandard 1.4
Add k-nucleotide to BenchmarkGames Commit migrated from dotnet/coreclr@e2b0cc3
7 participants
Changed .gitattributes to set line endings to windows ones for the input
files of this test.
Added two different tests, one with and the other without parallelism.
Used FileStream instead of BufferedStream, since the latter does not
exist in netstandard 1.4