Add Microsoft.Experimental.Collections package and move MultivalueDictionary to it #2439
Conversation
| <PropertyGroup> | ||
| <Description>This package provides collections that are being considered for future versions of .NET Core.</Description> | ||
| <TargetFramework>netstandard2.0</TargetFramework> | ||
| <PackageTags>collections, system, microsoft, fastdictionary, multivaluedictionary</PackageTags> |
There was a problem hiding this comment.
As I discovered recently, nuget splits tags on spaces. You should have no commas
There was a problem hiding this comment.
I don't think anyone will search for "fastdictionary"
There was a problem hiding this comment.
Ok, will change to spaces.
| <Project Sdk="Microsoft.NET.Sdk"> | ||
| <Import Project="..\..\tools\common.props" /> | ||
| <PropertyGroup> | ||
| <Description>This package provides collections that are being considered for future versions of .NET Core.</Description> |
There was a problem hiding this comment.
What about something like This package contains some useful collections that are not currently included in .NET Core itself.
There was a problem hiding this comment.
Oh, I see you are using the existing description. It's fine let's stick with that.
There was a problem hiding this comment.
I did change the description, the base is the same, just changed .NET Framework -> .NET Core.
| [Benchmark] | ||
| public void ContainsValue() | ||
| { | ||
| for (int i = 0; i <= 20000; i++) |
There was a problem hiding this comment.
Is it necessary to have such a for loop (along with loop unrolling)? Also, with different iteration counts, how do we reason about/compare the perf results?
cc @adamsitnik
There was a problem hiding this comment.
This is how the old perf tests where, creating a for loop for every iteration, that is why I left it. I don't know if it makes sense to just do it once instead of within a loop, I wonder if getting rid of this loops will give us a more realistic result.
There was a problem hiding this comment.
The previous tests used xunit.performance which didn't have some of the capabilities that BDN has. One of the capabilities is to detect the iteration count automatically. It also attempts to be accurate for nano-benchmarks by measuring/eliminating the idle time costs. Since this is a port to BDN, we can take advantage of these to simplify the tests.
I wonder if getting rid of this loops will give us a more realistic result.
If it doesn't work well in these scenarios, it would be good feedback for @adamsitnik
There was a problem hiding this comment.
Ok, then I will get rid of the loops and let BDN handle the iterations.
| [Benchmark] | ||
| public void Ctor() | ||
| { | ||
| for (int i = 0; i <= 20000; i++) |
There was a problem hiding this comment.
Couldn't some of these tests simply be the Ctor call with nothing else (let BDN take care of iterations/etc.)?
[Benchmark]
public void Ctor()
{
var dict = new MultiValueDictionary<int, string>();
}There was a problem hiding this comment.
I guess yes, we could just let BDN take care of the iterations and just call the ctor once.
| } | ||
| } | ||
|
|
||
| public List<int> values; |
There was a problem hiding this comment.
nit: Should this be a private field? If not, use PascalCasing.
There was a problem hiding this comment.
Will change to be private.
|
|
||
| public MultiValueDictionary<int, int> dict; | ||
|
|
||
| [GlobalSetup(Targets = new[] { nameof(Add), nameof(Clear), nameof(GetItem), nameof(GetKeys), nameof(ContainsValue) })] |
There was a problem hiding this comment.
Interesting use of GlobalSetup. I didn't know you could specify benchmark specific setups like this. Good to know :)
| [Params(1000, 10000, 10000)] | ||
| public int Size { get; set; } | ||
|
|
||
| public int RandomKey { get; set; } |
There was a problem hiding this comment.
In other benchmarks, we tend to use private fields rather than public properties. Any specific reason why you opted for properties instead?
There was a problem hiding this comment.
I was going to use [Params] in this property and that is why I used public. I didn't know if when using [Params] it can be public, don't know how benchmark initializes the property with the specified parameters in the attribute.
However, I will update to make the dictionary, list and randomkey private 😄
There was a problem hiding this comment.
[Params] requires it to be either a public property with public setter or public non-readonly field
| <Description>This package provides collections that are being considered for future versions of .NET Core.</Description> | ||
| <TargetFramework>netstandard2.0</TargetFramework> | ||
| <PackageTags>collections, system, microsoft, fastdictionary, multivaluedictionary</PackageTags> | ||
| <VersionPrefix>1.0.4</VersionPrefix> |
There was a problem hiding this comment.
There was a problem hiding this comment.
This package already exists in NuGet
https://www.nuget.org/packages/Microsoft.Experimental.Collections/1.0.3-alpha
So, unfortunately we need to have a custom version for this package unaligned for the other packages in this repo.
There was a problem hiding this comment.
I see. We don't publish to NuGet though, but to a custom MyGet feed, so there shouldn't be any conflicts.
https://dotnet.myget.org/gallery/dotnet-corefxlab
There was a problem hiding this comment.
Yes, but we're planning to publish this package to NuGet, because we want to start using this collections for some public benchmarks out there and for that we should put it into NuGet.
There was a problem hiding this comment.
Sounds good. Thanks for the clarification.
|
|
||
| namespace Microsoft.Collections.Extensions.Tests | ||
| { | ||
| public class MultiValueDictionaryPerformanceTests |
There was a problem hiding this comment.
Do we care about memory allocations for this type/benchmarks? If so, it might be useful to add a [MemoryDiagnoser] attribute on the class.
There was a problem hiding this comment.
I think it would be good to have those numbers to have a reference of improvements if we invest on that later on. I will add it.
…rimental.Collections.Tests project
…ded to the solution
safern
force-pushed
the
AddCollectionsPackage
branch
from
abdb0c7 to
f9c51e5
Compare
|
I updated the perf tests and the description to contain the results. @adamsitnik I'm getting this warnings: Could you please help me on how to solve them? |
|
|
||
| private void SetValue() | ||
| { | ||
| if (Size > 104) |
There was a problem hiding this comment.
Was this supposed to be 1024?
And should these be >=?
There was a problem hiding this comment.
@safern recently I was porting all the CoreFX dictionary benchmarks to BDN and rewrote all of them. You can find them in the performance repo https://github.com/dotnet/performance/search?q=Dictionary&unscoped_q=Dictionary
The benchmarks you have written are OK, you might take a look at what I wrote and copy the ideas if you want to.
Please remember to return the result from benchmark method to avoid dead code elimination.
| Random rand = new Random(11231992); | ||
|
|
||
| while (_dict.Count < Size) | ||
| _dict.Add(rand.Next(), rand.Next()); |
There was a problem hiding this comment.
what if the dictionary contains given key? will Add throw an exception?
There was a problem hiding this comment.
No, since this is a multivalue dictionary it allows to have multiple values for the same key, so if the key already exists it just adds the value to the key's collection of values.
| [Benchmark] | ||
| public void Ctor() | ||
| { | ||
| var _ = new MultiValueDictionary<int, string>(); |
There was a problem hiding this comment.
to prevent from dead code elimination by JIT you need to return the result from a method. This applies to this and all other benchmarks
Thanks for the pointer. I will take a look and update accordingly. |
|
I updated the perf tests to actually return a value and also updated the PR description to include perf results for netcoreapp2.1 and net472. |
|
@adamsitnik isn't there a way to get BDN to show both A and B results in the same table, ie. showing the ratio, so I don't have to scroll back and forth? Perhaps that's not possible here as it's different frameworks. |
That would be handy. |
|
Interesting that .NET Core is not faster/equal in every case. Eg., |
I saw the same thing. Maybe the way I hooked up the tests? Basically I just changed targetframework to net472. |
@danmosemsft sure! First of all we need to add .NET 4.7.2 to
Then from console line arguments:
an example: More info in the docs |
This is the beginning of a new package to experiment new collections before adding them to .NET Core.
Related to: #2406
I tried to split it per commit to make it easier to review since I did code cleanup and some modifications for the source to be able to build.
cc: @ahsonkhan @danmosemsft @joshfree @terrajobst
Netcoreapp2.1
.NET Framework 4.7.2