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

Add some additional micro benchmark coverage for immutable collections. #2268

Merged

Conversation

madelson
Copy link
Contributor

See dotnet/runtime#14477 (comment) for context.

Adding these additional benchmarks will obviously have runtime cost, so I wanted to start with just adding immutable collections benchmarks to a few files to get some feedback / a sense of whether or not this is the intended direction.

@danmoseley
Copy link
Member

@adamsitnik maybe can give feedback.

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@madelson big thanks for your contribution! Please take a look at my comments.

@@ -3,6 +3,7 @@
// See the LICENSE file in the project root for more information.

using System.Collections.Generic;
using System.Collections.Immutable;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the existing benchmarks that you can find in this file has been added in #938 to validate dotnet/coreclr#27299 which required a very specific benchmark coverage of the Dictionary<TKey, Value> APIs. We don't need such detailed coverage for other dictionary types as they already have good overall coverage. Please revert all the changes in this file.

@@ -133,5 +134,71 @@ public ObservableCollection<T> ObservableCollection()
}
return collection;
}

[Benchmark]
public ImmutableArray<T> ImmutableArray()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These benchmarks overall look good, but we should move them out of this class. This class is called AddGivenSize because it tests adding new items to collections that support specifying initial capacity. Like new List<T>(Size). We had benchmarks for AddDefaultSize (added in #92), but we have later decided to redesign them to CreateAddAndClear (#663) so they can also cover Clear. Please move the new benchmarks from this type to CreateAddAndClear and add a Clear method invocation to them.

madelson and others added 2 commits March 7, 2022 18:10
Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
@madelson
Copy link
Contributor Author

madelson commented Mar 8, 2022

@adamsitnik quickly reviewing the remaining benchmarks, I see the following places where we could add additional immutable collections coverage. Are any of these of interest?

TryAddDefaultSize

  • SortedDictionary
  • SortedList
  • ImmutableDictionary
  • ImmutableSortedDictionary

AddRemoveFromDifferentThreads/SameThread

  • ImmutableQueue + ImmutableInterlocked
  • ImmutableStack + ImmutableInterlocked

Perf_Dictionary

  • SortedDictionary
  • SortedList
  • ImmutableDictionary
  • ImmutableSortedDictionary

IndexerSet

  • ImmutableArray
  • ImmutableList
  • ImmutableDictionary
  • ImmutableSortedDictionary

Perf_SortedSet

  • ImmutableSortedSet

CopyTo

  • ImmutableList

CreateAddAndRemove

  • All immutable collections

Sort

  • ImmutableArray
  • ImmutableList

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @madelson !

@adamsitnik
Copy link
Member

TryAddDefaultSize

It's a good idea to add these (I was not aware of the existence of the TryAdd extension methods before you have asked this question).

AddRemoveFromDifferentThreads/SameThread

The original benchmarks are quite noisy (source):

image

and I am not sure whether we should promote immutable collections for multi-threaded mutable use cases (cc @stephentoub). I would rather say no.

Perf_Dictionary

The Perf_Dictionary type contains two benchmarks: cloning and ContainsValue. You could two dedicated types for that and add benchmarks for each collection that supports it and even remove Perf_Dictionary.

IndexerSet

the immutable collections are throwing for mutable methods:

https://github.com/dotnet/runtime/blob/0a684281f661f5520ad7f358a5056a2eed5b50ba/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray_1.cs#L30

and in general we are not benchmarking edge case scenarios like this (https://github.com/dotnet/performance/blob/main/docs/microbenchmark-design-guidelines.md#benchmarks-are-not-unit-tests)

Perf_SortedSet

It's a good idea to create similar type for ImmutableSortedSet

CopyTo
CreateAddAndRemove
Sort

👍

@madelson I am going to merge this PR, please send more benchmarks in new PRs. Thank you for your contribution!

@adamsitnik adamsitnik merged commit d2e6ba7 into dotnet:main Mar 9, 2022
@madelson
Copy link
Contributor Author

madelson commented Mar 9, 2022

Thanks @adamsitnik !

the immutable collections are throwing for mutable methods:

Sorry I should have clarified. The useful equivalent of indexer set for immutable collections is the SetItem method (e. g. list = list.SetItem(2, "a"). I think it would be reasonable to make that part of the IndexerSet benchmark but it could also be its own thing. Thoughts?

I am not sure whether we should promote immutable collections for multi-threaded mutable use cases

As mentioend there is the ImmutableInterlocked class which directly supports this use-case if we did want to support that. Happy to skip this one if you don't think it is important, though.

madelson added a commit to madelson/performance that referenced this pull request Mar 11, 2022
madelson added a commit to madelson/performance that referenced this pull request Mar 11, 2022
madelson added a commit to madelson/performance that referenced this pull request Mar 11, 2022
madelson added a commit to madelson/performance that referenced this pull request Mar 11, 2022
madelson added a commit to madelson/performance that referenced this pull request Mar 11, 2022
madelson added a commit to madelson/performance that referenced this pull request Mar 11, 2022
madelson added a commit to madelson/performance that referenced this pull request Mar 11, 2022
madelson added a commit to madelson/performance that referenced this pull request Mar 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants