Implementation for Dictionary.TryAdd

[jamesqo]

Progress towards https://github.com/dotnet/corefx/issues/1942

/cc @stephentoub, @jkotas, @danmosemsft, @ianhays

[jkotas]

This branch has conflicts that must be resolved

[jamesqo]

@jkotas Yeah, that's because of the codeformatter changes that recently went through (somewhat glad to see those conflicts, actually). Just fixed them with GitHub editor.

[jamesqo]

I fixed the conflicts for real after verifying that it compiled on my machine.

[safern]

Thanks @jamesqo, are you adding tests for this new API in corefx side right?

[jamesqo]

@safern Yes.

[danmoseley]

When you have the corefx PR up we can go ahead and merge this one. That rule helps us not forget ..

[jamesqo]

@danmosemsft The issues are not closed until the API is fully exposed in corefx, but sure, I'll submit the corefx tests (which I'm currently working on) alongside this PR.

[danmoseley]

@jamesqo I shouldn't have said forget -- the problem is if the corefx change never goes in, someone has to go do the work to rollback the coreclr change. You are completely reliable of course :), but that has happened recently with another contributor. So better to know the corefx change is ready to go.

[danmoseley]

@jkotas do you have any concerns about any subtle bad effect this may accidentally have on codegen? given this is rather critical code.

[jkotas]

When in doubts about perf ... measure!

using System;
using System.Collections.Generic;

class Test
{

    static void Main()
    {
        int start = Environment.TickCount;
        for (int i = 0; i < 1000000; i++)
        {
            var dict = new Dictionary<int, string>(1000);
            for (int j = 0; j < 500; j++)
                dict.Add(j, "Hello");
        }
        int end = Environment.TickCount;
        Console.WriteLine((end - start).ToString());
    }
}

Baseline (average of multiple runs): 9438ms
With changes (average of multiple runs): 9875ms

So there is about 5% regression for Dictionary<int, string>.Add method. This regression is unnecessary. It can be completely eliminated by structuring the change differently. Leaving the details as an exercise for the reader ;)

[danmoseley]

Should also check public TValue this[TKey key] perf isn't affected.

[jamesqo]

Baseline (average of multiple runs): 9438ms
With changes (average of multiple runs): 9875ms

@jkotas, 37 / 9438 is 0.3%, not 5%.

[stephentoub]

37 / 9438

Read the numbers Jan shared again? 😄

[jamesqo]

@stephentoub Wow, that should have taken me less than 5 minutes of staring at the comment to figure that out. Will fix the implementation and run tests to make sure perf doesn't regress again.

[jamesqo]

@jkotas I removed the extra branch I had accidentally introduced in Add, and I modified TryInsert to take an enum. After making the changes, I ran your microbenchmark 10 times against the old/new implementations and averaged the times.

Add, before: 12545.4
Add, after: 11928

I also modified it to use the indexer per @danmosemsft's concerns.

Indexer, before: 12484.4
Indexer, after: 11679.6

[jkotas]

Enum.IsDefined is too slow, even for a debug check in this method.

[jamesqo]

@jkotas OK. I can remove the assert then.

[mikedn]

switch? I wouldn't do that. If the C# and JIT compilers decide to generate a jump table based switch (and it looks like they do that unfortunately) both perf and code size will suffer.

Besides, the code ends up being rather convoluted. The None case breaks to return false while the OverwriteExisting case returns true directly. And ThrowOnExisting breaks but of course it doesn't really do that in reality.

Just use if

if (behavior == InsertionBehavior.ThrowOnExisting)
    ThrowHelper.ThrowAddingDuplicateWithKeyArgumentException(key); 
if (behavior == InsertionBehavior.None)
    return false;
Debug.Assert(behavior == InsertBehavior.OverwriteExisting); // if you want
entries[i].value = value; 
version++; 
return true; 

[jamesqo]

@mikedn Sure, thanks for the advice. I modified your example to check for OverwriteExisting first because that will be the most common branch.

[jamesqo]

@jkotas Does this PR need anything else to go forward?

[jkotas]

Could you please resolve conflicts in dotnet/corefx#16642 ?

[jamesqo]

@jkotas Should get around to it within next day or so. Am busy today.