Adding some basic perf tests for Double.TryParse and Single.TryParse #32392
Conversation
|
Do we need numbers separately for each of these? Do I need to report separately on negative pi and negative e? If we had single test for Double.TryParse (mixing up all the test values you have) with a single result, that is probably sufficient regression protection? Another possibility is two or three tests, for the different major codepaths (or duration buckets) |
|
I just wrote this using the existing format the other tests follow. I've sent a separate e-mail asking about what we can do for non micro-benchmarking. As I believe it would be good to get numbers for broader ranges of inputs here. |
|
Numbers are: |
|
Is it surprising that "1" is 10x slower than "0" ? |
|
No, "0" is currently fast-path'd, while "1" is not. |
|
Also, I don't think it's 10x slower. The inner iteration count between the two is different. If you scaled it, it would be 180 iterations at 57.350 seconds average vs 158 iterations at 63.624 seconds average. |
|
OK |
| @@ -49,6 +50,40 @@ public void DefaultToString(double number, int innerIterations) | |||
| } | |||
| } | |||
|
|
|||
| [Benchmark] | |||
| [InlineData("-∞", 10_000_000)] // Negative Infinity | |||
| [InlineData("-1.7976931348623157E+308", 100_000)] // Min Negative Normal | |||
There was a problem hiding this comment.
I'm not sure how much it matters, but do we want numbers that are larger than 10?
There was a problem hiding this comment.
That shouldn't matter as much. The current algorithm is mostly dependent on the length of the input string rather than the actual magnitude of its value.
| { | ||
| for (int i = 0; i < innerIterations; i++) | ||
| { | ||
| _bool = double.TryParse(input, out var result); |
There was a problem hiding this comment.
Do we want (or would it be valuable) to test TryParse that takes a Span<char>?
There was a problem hiding this comment.
double.TryParse(string, out double) is already calling the Span overload internally: https://source.dot.net/#System.Private.CoreLib/shared/System/Double.cs,315
| [Benchmark] | ||
| [InlineData("-∞", 10_000_000)] // Negative Infinity | ||
| [InlineData("-1.7976931348623157E+308", 100_000)] // Min Negative Normal | ||
| [InlineData("-3.1415926535897931", 1_000_000)] // Negative pi |
There was a problem hiding this comment.
How about numbers with commas (thousands separator)?
There was a problem hiding this comment.
Also - numbers with white space at the beginning/end. from https://docs.microsoft.com/en-us/dotnet/api/system.double.tryparse
or a string of the form:
[ws][sign][integral-digits,]integral-digits[.[fractional-digits]][e[sign]exponential-digits][ws]
In reply to: 219635010 [](ancestors = 219635010)
There was a problem hiding this comment.
These few tests are already adding on 6min 20sec onto the total benchmark time (38 new tests at 10s each). I think there are a number of other scenarios that would be good to test, but the current Benchmark.NET setup does not make that very feasible, so I tried to cover a number of useful/interesting inputs for the most common string format.
There was a problem hiding this comment.
so I tried to cover a number of useful/interesting inputs for the most common string format.
From a cursory look, I think some of your existing ones can be eliminated. For example:
System.Tests.Perf_Single.DefaultTryParse(input: "2.71828175", innerIterations: 1000000) | Duration | msec | 116 | 86.756 | 3.454 | 82.339 | 104.550
System.Tests.Perf_Single.DefaultTryParse(input: "-2.71828175", innerIterations: 1000000) | Duration | msec | 106 | 94.820 | 8.288 | 85.799 | 131.644
System.Tests.Perf_Single.DefaultTryParse(input: "3.14159274", innerIterations: 1000000) | Duration | msec | 108 | 92.847 | 7.307 | 83.224 | 120.448
System.Tests.Perf_Single.DefaultTryParse(input: "-3.14159274", innerIterations: 1000000) | Duration | msec | 115 | 87.533 | 3.175 | 84.733 | 106.748
Yes, these are important numbers: e and pi, but is parsing these numbers really that much different of a scenario?
Similar comment for:
System.Tests.Perf_Double.DefaultTryParse(input: "1.7976931348623157E+308", innerIterations: 100000) | Duration | msec | 863 | 11.594 | 0.479 | 11.033 | 15.891
System.Tests.Perf_Double.DefaultTryParse(input: "-1.7976931348623157E+308", innerIterations: 100000) | Duration | msec | 776 | 12.890 | 1.045 | 11.703 | 22.712
System.Tests.Perf_Double.DefaultTryParse(input: "2.2250738585072009E-308", innerIterations: 100000) | Duration | msec | 805 | 12.421 | 0.531 | 11.805 | 18.015
System.Tests.Perf_Double.DefaultTryParse(input: "-2.2250738585072009E-308", innerIterations: 100000) | Duration | msec | 777 | 12.867 | 0.841 | 12.156 | 24.272
System.Tests.Perf_Double.DefaultTryParse(input: "2.2250738585072014E-308", innerIterations: 100000) | Duration | msec | 845 | 11.839 | 0.536 | 11.271 | 18.168
System.Tests.Perf_Double.DefaultTryParse(input: "-2.2250738585072014E-308", innerIterations: 100000) | Duration | msec | 827 | 12.096 | 0.389 | 11.574 | 15.560
I think we could probably drop 2-3 from each of those blocks, and add at least 1 scenario with a comma in it.
There was a problem hiding this comment.
Talked with @eerhardt briefly offline.
I definitely agree that we should add more tests, but I am waiting to hear back on if we have or plan to have a better story for "end to end" benchmarks (so that we can actually validate the average perf for a broad range of inputs, or for benchmarks that take longer than 1 second per outer iteration to run).
The current tests where chosen because they have the broadest impact on the current algorithm (overall) and will be the most useful in validating some related changes going in.
|
This broke the perf CI leg: https://ci2.dot.net/job/dotnet_corefx/job/perf/job/master/job/perf_windows_nt_release/6797/consoleText cc @jorive |
|
Probably a xunit-performance bug as the remaining digits are truncated. code: report: |
|
Logged xunit/xunit#1822 |
CC. @adamsitnik, @eerhardt, @danmosemsft, @jkotas