DictionarySlim

[AnthonyLloyd]

No description provided.

[dnfclas]

All CLA requirements met.

[TylerBrinkley]

Where is the speclet issue related to this PR?

[AnthonyLloyd]

BenchmarkDotNet=v0.11.1, OS=Windows 10.0.17134.285 (1803/April2018Update/Redstone4)
Intel Core i7-7500U CPU 2.70GHz (Kaby Lake), 1 CPU, 4 logical and 2 physical cores
Frequency=2835941 Hz, Resolution=352.6166 ns, Timer=TSC
.NET Core SDK=2.1.402
[Host] : .NET Core 2.1.4 (CoreCLR 4.6.26814.03, CoreFX 4.6.26814.02), 64bit RyuJIT
DefaultJob : .NET Core 2.1.4 (CoreCLR 4.6.26814.03, CoreFX 4.6.26814.02), 64bit RyuJIT

RefDictionaryGeneral:

Method	Size	Mean	Error	StdDev	Median	Scaled	ScaledSD	Allocated
LoadRefDictionary	10000000	145.5 ms	7.376 ms	21.63 ms	132.9 ms	0.47	0.08	0 B
LoadDictionary	10000000	312.9 ms	10.645 ms	31.22 ms	298.1 ms	1.00	0.00	0 B
LoadRefDictionary	20000000	291.0 ms	7.944 ms	23.42 ms	278.5 ms	0.40	0.07	0 B
LoadDictionary	20000000	751.8 ms	42.727 ms	125.98 ms	721.3 ms	1.00	0.00	0 B
LoadRefDictionary	30000000	596.0 ms	32.589 ms	96.09 ms	642.7 ms	0.41	0.10	0 B
LoadDictionary	30000000	1,481.3 ms	78.445 ms	231.30 ms	1,560.1 ms	1.00	0.00	0 B

RefDictionaryKNucleotide:

Method	Size	Mean	Error	StdDev	Median	Scaled	ScaledSD
Hack	250000	127.05 ms	2.508 ms	3.829 ms	126.48 ms	0.81	0.03
Dictionary	250000	157.68 ms	3.045 ms	3.127 ms	157.50 ms	1.00	0.00
RefDictionary	250000	98.92 ms	1.919 ms	2.931 ms	99.51 ms	0.63	0.02
Hack	2500000	823.63 ms	16.111 ms	18.553 ms	820.17 ms	0.71	0.05
Dictionary	2500000	1,164.66 ms	26.708 ms	78.749 ms	1,139.85 ms	1.00	0.00
RefDictionary	2500000	596.48 ms	8.264 ms	6.901 ms	597.15 ms	0.51	0.03

Note: 25_000_000 shows even better perf but takes ages

RefDictionaryGroupBy:

Method	Size	Mean	Error	StdDev	Median	Scaled	ScaledSD
GroupBy	250000	10.228 ms	0.1995 ms	0.3164 ms	10.236 ms	1.00	0.00
RefDictionary	250000	8.991 ms	0.1772 ms	0.1896 ms	8.978 ms	0.88	0.03
GroupBy	2500000	161.679 ms	3.3140 ms	5.3514 ms	161.136 ms	1.00	0.00
RefDictionary	2500000	156.901 ms	3.1248 ms	6.3831 ms	158.265 ms	0.97	0.05
GroupBy	25000000	2,533.243 ms	59.5682 ms	175.6382 ms	2,539.343 ms	1.00	0.00
RefDictionary	25000000	2,403.900 ms	73.6960 ms	217.2943 ms	2,461.395 ms	0.95	0.11

@ahsonkhan @TylerBrinkley @safern @danmosemsft We should discuss next steps. I'm just going to look at improving the other methods like Keys/Values and see if I can use Span anywhere until then.

Note: updated after resize improvements.

[danmoseley]

Whoa!!

[danmoseley]

Can't wait to get numbers on the Q6600

[dadhi]

@AnthonyLloyd, interesting!

How about adding comparer as type parameter?

public sealed class RefDictionary<TKey, TValue,
TEq> : IReadOnlyCollection<KeyValuePair<TKey, TValue>> // almost IReadOnlyDictionary<TKey, TValue>
    where TEq : struct, IEqualityComparer<TKey>
    {
        private static readonly TEq _comparer = default;
// etc.

[AnthonyLloyd]

@dadhi Thanks, I tried it but not quite as good as IEquatable<T>

Method	Size	Mean	Error	StdDev	Scaled	ScaledSD
Hack	250000	131.7 ms	1.299 ms	1.215 ms	0.78	0.02
Dictionary	250000	169.2 ms	3.294 ms	4.045 ms	1.00	0.00
RefDictionary	250000	122.7 ms	2.431 ms	3.162 ms	0.73	0.02
Hack	2500000	884.3 ms	17.352 ms	17.820 ms	0.70	0.03
Dictionary	2500000	1,266.5 ms	25.324 ms	58.187 ms	1.00	0.00
RefDictionary	2500000	754.3 ms	15.086 ms	23.038 ms	0.60	0.03

[danmoseley]

For GetOrAddValueRef, I did not include the value as a parameter. I think that's potentially confusing/adds overhead.

Also I didn't add the "normal" indexer as we don't have a scenario currently. I think all feedback is now addressed?

[danmoseley]

@AnthonyLloyd @TylerBrinkley do changes look good? I would like to merge ..

[TylerBrinkley]

Looks good to me.

[danmoseley]

Q6600 with e536592 still fine.

Method	Size	Mean	Error	StdDev	Median	Ratio	RatioSD
Hack9	250000	128.0 ms	2.543 ms	5.582 ms	127.8 ms	1.00	0.00
Dictionary	250000	223.0 ms	4.870 ms	8.136 ms	221.8 ms	1.73	0.09
DictionarySlim	250000	130.4 ms	2.588 ms	4.030 ms	130.3 ms	1.02	0.05
Hack9	2500000	652.9 ms	22.393 ms	66.027 ms	656.9 ms	1.00	0.00
Dictionary	2500000	1,434.1 ms	34.787 ms	102.024 ms	1,484.9 ms	2.22	0.29
DictionarySlim	2500000	627.9 ms	17.783 ms	52.434 ms	613.3 ms	0.97	0.12
Hack9	25000000	6,261.5 ms	161.945 ms	472.401 ms	6,434.9 ms	1.00	0.00
Dictionary	25000000	14,430.4 ms	283.994 ms	540.329 ms	14,508.9 ms	2.31	0.21
DictionarySlim	25000000	5,753.9 ms	122.339 ms	360.720 ms	5,712.9 ms	0.92	0.09

[danmoseley]

Squashed, backup of history at https://github.com/dotnet/corefxlab/compare/master...danmosemsft:slimdictionary.history.backup?expand=1

[danmoseley]

Thanks @AnthonyLloyd @mikedn and @TylerBrinkley

We got a nice result, looking forward to getting it on nuget for feedback, and in the benchmark game.

[TylerBrinkley]

Thanks @danmosemsft. I really appreciated the opportunity of providing input on this!

[safern]

@AnthonyLloyd we've published the latest NuGet package into nuget.org so that it can be used for the benchmarks 😄

https://www.nuget.org/packages/Microsoft.Experimental.Collections/1.0.6-e190117-3

[AnthonyLloyd]

@safern great thanks, I've now submitted k-nucleotide to BenchmarksGame with it.

[danmoseley]

@AnthonyLloyd interesting, I see they posted it, and it's a bit slower than the Hack9 one. My measurements above, on the exact same hardware as them, got 9% faster. Any idea about that?

I was going to suggest we put an F# one up too, but it would be good to figure out what's wrong here first. One thing I notice is that the CPU utilization of the new one is a bit lower than the old one. That may be spurious though and I remember I did try various reorderings of the tasks and none were better than the existing one (I put those notes in one of the issues/PR's, can't find it right now)

[AnthonyLloyd]

I'm not sure. There are start up costs that we don't model. In the Benchmarks Game they run it 5 times and take the best result. It may be a bit random. I've seen this before where I knew something was faster but it didn't show up.

[benaadams]

. There are start up costs that we don't model. In the Benchmarks Game they run it 5 times and take the best result.

Yeah, I'm interested on what the effects of Tiering will be (e.g. 2.1 v 3.0) as they should get rid of some of the startup costs; but may introduce unoptimization + recompilation costs; so will be interesting how they play out in these benchmarks

[danmoseley]

An extra DLL load..

[AnthonyLloyd]

Just got the results for F#. Interesting its > 0.5s slower still.

(F# still ** all over Java though, so all good)

[danmoseley]

@AnthonyLloyd is it worth asking for suggestions in the F# repo? I doubt they will be satisfied with being slower than C#

[Krakean]

@danmosemsft @AnthonyLloyd @benaadams
Curious, anyone tested DictionarySlim.ContainsKey on empty dictionary?
In my case I getting "Arithmetic operation resulted in an overflow." here - https://i.imgur.com/T6zUIvi.png

I initiate DictionarySlim this way, in case if it matters:
private static readonly DictionarySlim<string, ConsoleCommand> _commandsMap = new DictionarySlim<string, ConsoleCommand>();

Then I just before execute console command, check whether it exists/registered in _commandMap and get the crash above, because at the moment of such check _commandMap is empty yet.

Also, the same error I getting when trying to add command first: https://i.imgur.com/Y8BkGrz.png
I do it this way:

                var cc = new ConsoleCommand
                {
                    name      = command,
                    arguments = arguments,
                    desc      = commandDesc,
                    flags     = cmdFlags,
                    function  = consoleCmdDelegate
                };

                _commandsMap.GetOrAddValueRef(command) = cc;

I do it wrong way?

P.S. Using .NET Core 2.2 & VS2019.

[danmoseley]

@Krakean could you please open a new issue?

It looks like you are consuming DictionarySlim as source, not via the nuget package. And your binary Nuage.Engine.dll is compiled with checked arithmetic, which is not the default. Without checked, the comparison would fail and the method simply return false.

We could explicitly put unchecked{ } around the cases like (uint)i < (uint)entries.Length to help this case. perhaps you'd like to open an issue and offer a PR?

BTW in general it is best to not append to closed issues/PR's as the comment may not be seen.

[Krakean]

@danmosemsft

It looks like you are consuming DictionarySlim as source, not via the nuget package.

Yep. I prefer to not bring packages when I need just one file :)

And your binary Nuage.Engine.dll is compiled with checked arithmetic, which is not the default.

Yes, it is. Because, well, arithmetic overflows are no good, no? :)

We could explicitly put unchecked{ } around the cases like (uint)i < (uint)entries.Length to help this case

Well, if it can fix it, then why not.

perhaps you'd like to open an issue and offer a PR?

PR - unlikely, but open an issue - yes :)

[danmoseley]

arithmetic overflows are no good, no? :)

A community member began an effort a while back to change all of CoreFX (the .NET Core libraries) to build with checked arithmetic, marking unchecked as necessary (for cases like this). We abandoned the effort as it was making the code uglier without finding significant actual bugs. I think it is fine to mark this case if it helps you though.

[jkotas]

Note that checked arithmetic has significant performance overhead. If you are building your projects with checked arithmetic, you may be better off using the built-in Dictionary. DictionarySlim compiled with checked arithmetic is likely slower than the built-in Dictionary.