Runtime: [Perf -19%] System.Net.NetworkInformation.Tests.PhysicalAddressTests.PAShort

[DrewScoggins]

Run Information

Architecture | x64
-- | --
OS | Windows 10.0.18362
Changes | diff

Regressions in System.Net.NetworkInformation.Tests.PhysicalAddressTests

Benchmark | Baseline | Test | Test/Base | Modality | Baseline Outlier
-- | -- | -- | -- | -- | --
PAShort | 14.44 ns | 17.14 ns | 1.19 | | False

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.Net.NetworkInformation.Tests.PhysicalAddressTests*"

Histogram

System.Net.NetworkInformation.Tests.PhysicalAddressTests.PAShort

[14.317 ; 14.884) | @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
[14.884 ; 15.542) | @@@@@@@@@
[15.542 ; 16.023) | @@@
[16.023 ; 16.671) | @
[16.671 ; 17.238) | @@@@@@@@@@@@@@@@@@@@@@@
[17.238 ; 17.945) | @@@
[17.945 ; 18.526) | @@

Docs

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

[msftbot[bot]]

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

[karelz]

Triage: Not important for 5.0, still might be nice to see what caused it, but there haven't been any changes in the space ...
Fine to just close it as "OK" in future.

[DrewScoggins]

@tannergooding who mentioned that he thought there was some change in this space.

[wfurt]

hmm That lines up with #37546 on Jul 17. That change did not go through ncl.

[danmosemsft]

It does seem very likely #37546. It's the only plausible commit in this interval . cc @tkp1n in case he is interested.

[tkp1n]

The additional ~3ns can easily be explained by the added overhead of using the new public hex encoding API instead of an internal method without as many input checks.

The easy fix would be to revert this line from:
return Convert.ToHexString(_address.AsSpan());
back to
return HexConverter.ToString(_address.AsSpan(), HexConverter.Casing.Upper);

@danmosemsft Feel free to assign this issue to me, if you want me to open a PR and investigate whether the proposed change would actually help.

I've also opened #39702 (some time ago) to vectorize hex encoding/decoding. This would provide a more sustainable solution that'll also help other places in the framework. Until now, no one from the team seemed to care.

[danmosemsft]

@tkp1n thanks for the offer - I've assigned this to you. We would welcome a PR if it would reverse this small regression. We ARE interested in https://github.com/dotnet/runtime/pull/39702 😊- we've just been pretty occupied on bugs, so it may miss the 5.0 cutoff.

[danmosemsft]

Per discussion in the PR, closing this as acceptable for improved code reuse.

[adamsitnik]

Thre regression is between 30-50%, are we sure that this method is never on hot path?

System.Net.NetworkInformation.Tests.PhysicalAddressTests.PAShort

| Conclusion | Base | Diff | Base/Diff | Modality | Operating System | Bit | Processor Name | Base Runtime | Diff Runtime |
| ---------- | -----:| -----:| ---------:| -------- | ----------------------- | --- | ------------------------------------------- | --------------- | --------------------- |
| Slower | 19.96 | 37.67 | 0.53 | | Windows 10.0.18363.1016 | Arm | Microsoft SQ1 3.0 GHz | .NET Core 3.1.6 | 5.0.100-rc.1.20413.9 |
| Slower | 10.44 | 15.50 | 0.67 | | Windows 10.0.18363.959 | X64 | Intel Xeon CPU E5-1650 v4 3.60GHz | .NET Core 3.1.6 | 5.0.100-rc.1.20404.3 |
| Slower | 10.94 | 15.55 | 0.70 | | Windows 10.0.18363.959 | X64 | Intel Xeon CPU E5-1650 v4 3.60GHz | .NET Core 3.1.6 | 5.0.100-rc.1.20418.3 |
| Slower | 12.92 | 17.58 | 0.73 | | Windows 10.0.19041.450 | X64 | Intel Core i7-5557U CPU 3.10GHz (Broadwell) | .NET Core 3.1.6 | 5.0.100-rc.1.20413.9 |
| Slower | 9.39 | 13.49 | 0.70 | bimodal | Windows 10.0.19042 | X64 | Intel Core i7-7700 CPU 3.60GHz (Kaby Lake) | .NET Core 3.1.6 | 5.0.100-rc.1.20418.3 |
| Slower | 13.36 | 29.57 | 0.45 | bimodal | Windows 10.0.19041.450 | X86 | Intel Core i7-5557U CPU 3.10GHz (Broadwell) | .NET Core 3.1.6 | 5.0.100-rc.1.20419.5 |
| Slower | 16.15 | 23.93 | 0.67 | | ubuntu 18.04 | X64 | Intel Xeon CPU E5-1650 v4 3.60GHz | .NET Core 3.1.6 | 5.0.100-rc.1.20403.23|
| Slower | 17.98 | 26.68 | 0.67 | | macOS Mojave 10.14.5 | X64 | Intel Core i7-5557U CPU 3.10GHz (Broadwell) | .NET Core 3.1.6 | 5.0.100-rc.1.20404.2 |
| Slower | 17.38 | 27.83 | 0.62 | | ubuntu 18.04 | X64 | Intel Core i7-7700 CPU 3.60GHz (Kaby Lake) | .NET Core 3.1.6 | 5.0.100-rc.1.20418.3 |

[stephentoub]

Thre regression is between 30-50%, are we sure that this method is never on hot path?

This test is for an address with a single byte. That doesn't exist in any real scenario.

[adamsitnik]

@stephentoub thanks for confirmation, should we remove this benchmark then?

[stephentoub]

No strong opinion.

[danmosemsft]

If we wouldn't fix a 50% regression the scenario seems not worth having. Can it be modified to be useful?

[tkp1n]

I'd say a 6-byte MAC address is likely the typical usecase for this class.

[wfurt]

Can we modify the test to don't run automatically? This may be canary so we are aware of changes even this is not real scenario. If that is not possible I think we should remove it assuming we have coverage for the "normal" cases.

[DrewScoggins]

If we are not running this as part of the lab runs, is the value just for devs to run this test locally as a quick check if something has been regressed?

[geoffkizer]

I'm trying to imagine how this test would ever be useful, even if modified to use a 6-byte address instead of 1-byte, or some other modification.

It's testing the ToString method on PhysicalAddress. And it takes ~20ns to run.

This is not a a commonly used API. It is not perf critical. It could literally regress by 10000% and we'd still probably punt it.

I vote to kill the test and focus on higher value stuff.