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 -22%] System.Collections.AddGivenSize<Int32>.SortedList #39112

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

[Perf -22%] System.Collections.AddGivenSize<Int32>.SortedList #39112

DrewScoggins opened this issue Jul 10, 2020 · 5 comments
Assignees
Labels
arch-x64 area-System.Collections 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.Collections.AddGivenSize

Benchmark Baseline Test Test/Base Modality Baseline Outlier
SortedList 36.19 μs 43.51 μs 1.20 False

Related Issue on x86 Windows

[Perf -12%] System.Collections.AddGivenSize.SortedList

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.Collections.AddGivenSize<Int32>*';

Histogram

System.Collections.AddGivenSize.SortedList(Size: 512)

[33468.100 ; 34241.363) | @
[34241.363 ; 35597.511) | @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
[35597.511 ; 36860.074) | @@@@@@@@@@
[36860.074 ; 38381.947) | 
[38381.947 ; 40025.831) | @@@@@@@@@@@@@
[40025.831 ; 41613.940) | @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
[41613.940 ; 42970.089) | @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
[42970.089 ; 44360.270) | @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
[44360.270 ; 46060.324) | @

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 Dotnet-GitSync-Bot added area-System.Collections untriaged New issue has not been triaged by the area owner labels Jul 10, 2020
@ghost
Copy link

ghost commented Jul 10, 2020

Tagging subscribers to this area: @eiriktsarpalis
Notify danmosemsft if you want to be subscribed.

@eiriktsarpalis eiriktsarpalis added this to the 5.0.0 milestone Jul 13, 2020
@eiriktsarpalis eiriktsarpalis self-assigned this Jul 13, 2020
@danmoseley
Copy link
Member

This test primarily stresses Array.Copy.

Array.Copy(keys, index, keys, index + 1, _size - index);
Array.Copy(values, index, values, index + 1, _size - index);

The regression apparently occurred between Oct 29 and Nov 4th . On Nov 2nd, we rewrote Array.Copy in C# (dotnet/coreclr#27634)

There was actually a followup PR to replace some overloads of Array.Copy with others that touched SortedList on Nov 4th (dotnet/corefx#42343) but it does not affect the tested codepath.

cc @jkotas

@danmoseley
Copy link
Member

Duped a bunch of tests that all stress Array.Copy and regressed at the same time.

Per comments from @stephentoub in some of these, it may be bimodal, or configuration dependent.

@jkotas
Copy link
Member

jkotas commented Aug 1, 2020

Memory copying has very complex perf characteristics. Any change will make some cases faster in some situations and some other cases slower in other situations.

I do not think there is much we can do here.

@danmoseley
Copy link
Member

danmoseley commented Aug 2, 2020

OK, I am going to close this. Realistically if you are doing this in a hot code path, you would want to consider using a different datastructure . Instead of SortedList/SortedList<K,V>, which is array backed, SortedDictionary<K,V>, which would give you faster insert/remove at the cost of some memory. (details). Similarly for List<T>/Array you might use SortedSet<T> depending on what you want.

cc area owners @layomia @eiriktsarpalis

@layomia layomia removed the untriaged New issue has not been triaged by the area owner label Aug 28, 2020
@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.Collections 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