-
Notifications
You must be signed in to change notification settings - Fork 272
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 Regex perf tests from industry benchmarks #2125
Conversation
src/benchmarks/micro/libraries/System.Text.RegularExpressions/Perf.Regex.Industry.cs
Show resolved
Hide resolved
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.
LGTM assuming the run time is acceptable to the perf folks.
How long does all this take to run locally?
src/benchmarks/micro/libraries/System.Text.RegularExpressions/Perf.Regex.Industry.cs
Show resolved
Hide resolved
src/benchmarks/micro/libraries/System.Text.RegularExpressions/Perf.Regex.Industry.cs
Outdated
Show resolved
Hide resolved
I generated these locally with |
A while. It's ~200 benchmarks per platform target, e.g. when I run them comparing main vs a pr, it's ~400 benchmarks to be run. |
I used 7zip with gzip selected and compression level "Ultra" ... 3200.txt goes from 6.21 MB to 5.93 MB. 7z and bz2 formats are about 4.9 MB, of course we can't read those.. |
@DrewScoggins how do you feel about run time here? @kunalspathak if we add 200 more scenarios, does that significantly affect triage (eg., if we regress 50 of them together)? |
Something that @DrewScoggins would know exactly, but we have fewer arm64 machines and currently backlogged with existing benchmarks itself. With that said, I don't think we should stop ourselves from adding more benchmarks. I think we should just increase the machine capacity.
It depends on how flaky these are. (Eventually) when we improve the noise filtration logic, it shouldn't be a problem. |
Believe me, I would also love to get more machines! In the meantime, I am running the tests locally to get an idea of the total amount of time they will be adding (26 minutes). Like @kunalspathak said, the only real place where we are resource constrained is on Arm64 machine, but I don't believe that this will be prohibitive, and if it is we can revisit what tests we run on Arm64.
Having a bunch regress all at once from a product change will not be a big deal. We can already handle that scenario. |
thanks @DrewScoggins . Also, after this is in, is it possible to get a 6.0 (and ideally 5.0) baseline number that shows up in the graphs, or does that happen automatically? |
If we want to get baseline numbers for 5.0 and 6.0, we will need to backport these tests. It will be easy to do for 6.0, just make a PR to the release/6.0 branch and it will get picked up. For 5.0 it will require some more work, as we didn't have the system we have now for release branches when we forked for 5.0. I have made this issue, #2129, to track the work that we would need to do. In the meantime, maybe we could do a one off run on some lab machines to get some comparison numbers for 5.0 vs 6.0 vs today for these tests? |
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.
LGTM, thank you @stephentoub
Now let's make sure that .NET is the best for all of the test cases ;)
src/harness/BenchmarkDotNet.Extensions/TooManyTestCasesValidator.cs
Outdated
Show resolved
Hide resolved
src/benchmarks/micro/libraries/System.Text.RegularExpressions/Perf.Regex.Industry.cs
Outdated
Show resolved
Hide resolved
src/benchmarks/micro/libraries/System.Text.RegularExpressions/Perf.Regex.Industry.cs
Show resolved
Hide resolved
One of the CI legs failed with mysterious python error:
I'll re-run it. |
src/benchmarks/micro/libraries/System.Text.RegularExpressions/Perf.Regex.Industry.cs
Show resolved
Hide resolved
I was unable to repro the CI failure locally: git clone https://github.com/stephentoub/performance.git && cd performance && git checkout regextests
py .\scripts\benchmarks_ci.py -f net461 --filter *Regex* --bdn-arguments="--iterationCount 1 --warmupCount 0 --invocationCount 1 --unrollFactor 1 --strategy ColdStart --stopOnFirstError true" |
Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
failure --
@DrewScoggins have we seen this before? |
BTW surely if you just run this test on .NET Framework locally and it passes that's good enough to check in. |
Yes we were seeing stuff like this, and it was fixed with #2108.
Hard to say, because our lab machines are not identical to the CI machines. We have never had a situation where something works on local machines, but fails on the CI VMs. |
cc: @adamsitnik, @danmoseley, @olsaarik