[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

[ghost]

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.

[danmoseley]

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.

[danmoseley]

@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 #39702 😊- we've just been pretty occupied on bugs, so it may miss the 5.0 cutoff.

[danmoseley]

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.

[danmoseley]

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.