Skip to content
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

[Perf -14%] System.Tests.Perf_UInt32.TryParseHex #39119

Closed
DrewScoggins opened this issue Jul 10, 2020 · 9 comments
Closed

[Perf -14%] System.Tests.Perf_UInt32.TryParseHex #39119

DrewScoggins opened this issue Jul 10, 2020 · 9 comments
Labels
arch-x64 area-System.Numerics os-linux Linux OS (any supported distro) tenet-performance Performance related issue tenet-performance-benchmarks Issue from performance benchmark
Milestone

Comments

@DrewScoggins
Copy link
Member

DrewScoggins commented Jul 10, 2020

Run Information

Architecture x64
OS ubuntu 18.04
Changes diff

Regressions in System.Tests.Perf_UInt32

Benchmark Baseline Test Test/Base Modality Baseline Outlier
TryParseHex 15.14 ns 17.49 ns 1.16 Bimodal False

graph
Historical Data in Reporting System

Repro

git clone https://github.com/dotnet/performance.git
py .\performance\scripts\benchmarks_ci.py -f netcoreapp5.0 --filter 'System.Tests.Perf_UInt32*';

Histogram

System.Tests.Perf_UInt32.TryParseHex(value: "FFFFFFFF")

[14.487 ; 14.918) | @@@
[14.918 ; 15.494) | @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
[15.494 ; 15.920) | @@@
[15.920 ; 16.496) | 
[16.496 ; 16.855) | 
[16.855 ; 17.250) | @@@@@@@@
[17.250 ; 17.826) | @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
[17.826 ; 18.411) | @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
[18.411 ; 18.967) | @@@@@@
[18.967 ; 19.543) | 
[19.543 ; 20.119) | 
[20.119 ; 20.695) | 
[20.695 ; 21.323) | 
[21.323 ; 21.899) | @

Docs

Profiling workflow for dotnet/runtime repository
Benchmarking workflow for dotnet/runtime repository

@DrewScoggins DrewScoggins added os-linux Linux OS (any supported distro) tenet-performance Performance related issue tenet-performance-benchmarks Issue from performance benchmark arch-x64 labels Jul 10, 2020
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jul 10, 2020
@danmoseley
Copy link
Member

@DrewScoggins please include a diff link showing changes closely bracketing regression. This one does not work. I believe this is important for all regression issues.

@DrewScoggins
Copy link
Member Author

DrewScoggins commented Jul 10, 2020

#39122 (comment)

This should help you track down the diff.

@stephentoub
Copy link
Member

I can't repro this locally on Ubuntu 18.04. All inputs to this test show improvement for me:

dotnet run -c Release -f netcoreapp3.1 --runtimes netcoreapp3.1 netcoreapp5.0 --join --filter *System.Tests.Perf_UInt32.TryParseHex*
Method Job Runtime Toolchain value Mean Error StdDev Median Min Max Ratio
TryParseHex Job-WVEAVH .NET Core 3.1 netcoreapp3.1 0 10.219 ns 0.0542 ns 0.0480 ns 10.204 ns 10.125 ns 10.317 ns 1.00
TryParseHex Job-ZMIWWJ .NET Core 5.0 netcoreapp5.0 0 8.613 ns 0.0618 ns 0.0578 ns 8.583 ns 8.554 ns 8.732 ns 0.84
TryParseHex Job-WVEAVH .NET Core 3.1 netcoreapp3.1 3039 14.377 ns 0.1322 ns 0.1104 ns 14.356 ns 14.227 ns 14.556 ns 1.00
TryParseHex Job-ZMIWWJ .NET Core 5.0 netcoreapp5.0 3039 13.265 ns 0.0622 ns 0.0552 ns 13.271 ns 13.148 ns 13.355 ns 0.92
TryParseHex Job-WVEAVH .NET Core 3.1 netcoreapp3.1 FFFFFFFF 19.740 ns 0.1613 ns 0.1347 ns 19.765 ns 19.464 ns 19.950 ns 1.00
TryParseHex Job-ZMIWWJ .NET Core 5.0 netcoreapp5.0 FFFFFFFF 18.601 ns 0.1318 ns 0.1233 ns 18.604 ns 18.430 ns 18.842 ns 0.94

@ghost
Copy link

ghost commented Jul 12, 2020

Tagging subscribers to this area: @tannergooding, @pgovind
Notify danmosemsft if you want to be subscribed.

@danmoseley danmoseley added this to the Future milestone Aug 26, 2020
@jeffschwMSFT jeffschwMSFT removed the untriaged New issue has not been triaged by the area owner label Aug 31, 2020
@danmoseley
Copy link
Member

I looked at the diffs for each transition back and forth in Dec and Jan and nothing stands out as relevant. Since there's not a local repro, I don't think this issue is useful.

@DrewScoggins
Copy link
Member Author

This commit,f6dc87f, is in the tight bound, 49186d7...b570558. You can see on this report that we are seeing a very noticeable jump. Given that the code change happened over that 12 hour period, it feels like it is worth some further digging. We can give access to a lab machine for repro as well if we are having trouble tracking it down on other machines.

@DrewScoggins DrewScoggins reopened this Sep 3, 2020
@danmoseley
Copy link
Member

Thank you @DrewScoggins - that's from July 17, whereas the issue was for Nov and Mar regressions, and @stephentoub verified July 10th. Still we can reuse this issue.

We discussed a very similar regression due to this case here #39720 (comment)

The commit you correctly flagged was one that created a public Hex parsing API and replaced all other internal Hex parsing with this. As in that case, the API is slower for very short inputs. We decided to ignore that case because the absolute time is so tiny. In this case the absolute time is still tiny (regression is 2 nanoseconds) but it's an API that's more likely to be called often.

@tannergooding owns this area and can make the best call on whether the code unification is worth the 2ns regression -- and in particular whether @tkp1n 's #39702 would potentially regain this regression without losing that nice common implementation.

@danmoseley danmoseley modified the milestones: Future, 5.0.0 Sep 3, 2020
@tannergooding
Copy link
Member

I don't think the 2ns difference is a concern here. We can always revisit in the future if someone reports an issue on larger workloads.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-x64 area-System.Numerics os-linux Linux OS (any supported distro) tenet-performance Performance related issue tenet-performance-benchmarks Issue from performance benchmark
Projects
None yet
Development

No branches or pull requests

6 participants