Hi @JeffreyZhao

thank you for excellent benchmarks and your improvements in HashSet<T>!

Could you please share results of a benchmark where the performance of int and string are much different than CustomValue and CustomObject? Adding these two generic type arguments doubles the number of System.Collections benchmarks (and the time it takes to run them). I want to make sure that it's worth it.

I have a question about the CopyAndRemove benchmarks that are being added? Is that a common scenario that we see with user code? If it is than I think the test makes a lot of sense, but if all we are trying to do is measure the time for removing objects from various collection types it seems like we should try and move the copying of the data outside of the timing loop.

@DrewScoggins Is that a common scenario that we see with user code? It seems like we should try and move the copying of the data outside of the timing loop.

No, I don't think it's a common scenarios.

A benchmark is required to be no side-effect (so that BenchmarkDotNet could run it repeatedly to get an accurate result), so we have to make a copy before actual removal from a collection. I believe it's the same reason that currently we don't have a dedicated benchmark for removal operation, instead, we have to use CreateAddAndRemove for the same reason. I've tried to applied [IterationSetup] but unfortunately BenchmarkDotNet will only run the actual benchmark once if there's a setup for a specific benchmark method.

However, we have RemoveFalse for removing items are not included in the collection, since trying to remove something not there has no side-effect.

@adamsitnik

The benchmark results forint and CustomValue are close in all cases. The question is that we only know it when we have the benchmark. My concern is that we don't know if code works the same good or bad for int (a special type close to the runtime), and for another custom struct (even it has the same size as an int, e.g., CustomValue).

The case for string and CustomObject is more interesting since sometimes we treat string differently in .NET.

For example:

public Dictionary(int capacity, IEqualityComparer<TKey>? comparer)
{
    if (capacity < 0) ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.capacity);
    if (capacity > 0) Initialize(capacity);
    if (comparer != EqualityComparer<TKey>.Default)
    {
        _comparer = comparer;
    }

    if (typeof(TKey) == typeof(string) && _comparer == null)
    {
        // To start, move off default comparer for string which is randomised
        _comparer = (IEqualityComparer<TKey>)NonRandomizedStringEqualityComparer.Default;
    }
}

In Dictoinary we'll use NonRandomizedStringEqualityComparer (which runs faster than the default comparer) instead of EqualityComparer<String>.Default for string. It means we'll never run into the code branch prepared for a reference type with no comparer specified. So at least for Dictionary class, a benchmark for string cannot represent another reference type, and existing benchmarks haven't covered that case at all.

Also I can imagine more cases that hasn't been covered by existing benchmarks. For example, larger structs.

We know copying large structs (frequently) hurts performance, but sometimes we would make copies in code naturally (e.g., using temporary variable, which is considered a good code practice in many places), and we need to modify the code as accessing struct array by index repeatedly, or using ref/in etc. Using large struct in collection might not be a typical scenario for everyday use, but I do remember seeing code like using 24-byte struct as the key of a dictionary in many occasions, especially now ValueTuple has been included in .NET so people tend to use them more often.

So I have feelings that using fundamental types like int or string (which are too close to the runtime) only might not be enough.

And regarding the benchmark result. Here's an example of ContainsTrue for HashSet. As I mentioned earlier, String and CustomObject doesn't go to same code branch in Dictionary, so I use HashSet as the example here.

dotnet run -c Release -f netcoreapp3.0 --filter "*ContainsTrue*.HashSet" --join

BenchmarkDotNet=v0.12.0, OS=Windows 10.0.17763.805 (1809/October2018Update/Redstone5)
Intel Core i7-7700HQ CPU 2.80GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=3.0.100
  [Host]     : .NET Core 3.0.0 (CoreCLR 4.700.19.46205, CoreFX 4.700.19.46214), X64 RyuJIT
  Job-HEYZLQ : .NET Core 3.0.0 (CoreCLR 4.700.19.46205, CoreFX 4.700.19.46214), X64 RyuJIT

PowerPlanMode=00000000-0000-0000-0000-000000000000  IterationTime=250.0000 ms  MaxIterationCount=20
MinIterationCount=15  WarmupCount=1

So int and CustomValue are close but CustomObject is 45% faster than string. Since they're going to the same code branch, the difference can be considered as the cost of GetHashCode and Equals implementations for string. So as I stated in the initial post, when we using string we're not only benchmarking collection operations.

A benchmark is required to be no side-effect (so that BenchmarkDotNet could run it repeatedly to get an accurate result), so we have to make a copy before actual removal from a collection. I believe it's the same reason that currently we don't have a dedicated benchmark for removal operation, instead, we have to use CreateAddAndRemove for the same reason.

I agree, it's more a BenchmarkDotNet design limitation

The benchmark results for int and CustomValue are close in all cases.
We know copying large structs (frequently) hurts performance

I think that we should then increase the size of CustomValue to something like 3 long fields so we are going to have 4 scenarios covered:

• int - a very commonly used primitive type, small copying does not hurt
• CustomValue - big value type, copying hurts
• string - a very commonly used reference type with dedicated execution path for Equals and GetHashCode
• CustomObject - simple reference type that implements IEqutable

Increasing the size of CustomValue would add value to my Linux vs Windows performance investigation which has already shown some perf issues with structs on Linux.

@adamsitnik

May I ask why BenchmarkDotNet choose to run a benchmark only once if there's a [IterationSetup] for it?

I think that we should then increase the size of CustomValue to something like 3 long fields so we are going to have 4 scenarios covered:

I think that's a good idea. And what do you think of making the names more obvious, like, LargeValue (or even Value24Bytes) and SmallObject?