[mountgellert]

Delegate's GetHashCode just returns the hash code of the delegate type. For a scenario where delegates are used as keys in a dictionary, this leads to obvious performance problems. We should look at coming up with a better GetHashCode that properly factors in the target object and method, in all of the various forms a delegate can take.

Fixes https://github.com/dotnet/coreclr/issues/9040

[jkotas]

Does not build...

[jkotas]

_target should be included in the hash code only if _methodPtrAux is null.

_target is overloaded field. It points other helper delegate objects when _methodPtrAux is non-null. The table at

coreclr/src/vm/comdelegate.cpp

Line 3471 in 5e03cb0

// 1- Instance closed 'this' ptr target method null null 0

has the summary of the different options.

[mountgellert]

So something like

            if (_methodPtrAux.IsNull())
                return ( _target != null ? _target.GetHashCode() : 0) * 33 + GetType().GetHashCode();
            else
                return GetType().GetHashCode();

would be more suitable?

[jkotas]

Also, this needs to use RuntimeHelpers.GetHashCode so that it is not exposed to how custom types implement GetHashCode (e.g. they can implement it as throw new NotSupportedException()).

[mountgellert]

I don't really understand this comment; you mean instead of GetType().GetHashCode()?

[jkotas]

I meant RuntimeHelpers.GetHashCode .

[mountgellert]

Ah, to get the nonvirtual hashcode. Sure, updated.

[mountgellert]

Now it seems to build.

[mountgellert]

Windows_NT x64 Release Priority 1 Build and Test fails due to

                booldelegate workDelegate = new booldelegate(new TestClass(1).StartWork_Bool);
                booldelegate workDelegate1 = new booldelegate(new TestClass1(2).StartWork_Bool );

Having a different hash code with my changes. They are clearly different delegates, does it makes sense to have the same return code?

@jkotas if you agree on that, I'd modify the testcase as well so it would accept this (by basically flipping the comparism)

---

I'm rereading Equals(), and found the snippet:

                // Ignore the target as it will be the delegate instance, though it may be a different one
                /*
                if (_methodPtr != d._methodPtr)
                    return false;
                    */

and also looked at:

        public override sealed int GetHashCode()
        {
            if (IsUnmanagedFunctionPtr())
                return ValueType.GetHashCodeOfPtr(_methodPtr) ^ ValueType.GetHashCodeOfPtr(_methodPtrAux);

            Object[] invocationList = _invocationList as Object[];
            if (invocationList == null)
            {
                return base.GetHashCode();
            }
            else
            {
                int hash = 0;
                for (int i = 0; i < (int)_invocationCount; i++)
                {
                    hash = hash * 33 + invocationList[i].GetHashCode();
                }

                return hash;
            }
        }

from MultiCastDelegate.cs. That one also does go through the invocationlist and not the target.

Does this mean that there is no easy way to solve this problem, and generally delegates shouldn't be used as keys in dictionaries?

[jkotas]

Having a different hash code with my changes. They are clearly different delegates, does it makes sense to have the same return code?

Agree - the test needs updating.

That one also does go through the invocationlist and not the target.

The invocationList will contain other Delegate object for MulticastDelegates, so it will end up calling your hashing function recursively.

Does this mean that there is no easy way to solve this problem, and generally delegates shouldn't be used as keys in dictionaries?

I think your implementation works fine. It needs to be tested well though.

[danmoseley]

Are automated tests possible? (Ie., can we write a test where it's guaranteed that X and Y will have different hashcodes?). If so it would be great to have a CoreFX PR up.

[mountgellert]

@danmosemsft Actually there is a test already that fails now, DelegateGetHashCode.PosTest7

[mountgellert]

@jkotas I'm trying to figure out whether I need to update the Equals() method as well, can you advise.

[jkotas]

No change needed in Delegate.Equals

[mountgellert]

Than this is up for merge.

[jkotas]

• delegategethashcode1.cs had a Unicode encoding, so it was showing up as binary in the diff. I have fixed the encoding of the file. Could you please resolve the conflict with it?

• We should add a regression test to corefx repo that fails without this change, and that will pass with this change. E.g. this test can create two delegates pointing to the same method on different instances, and verify that they have different hashcode. It can retry say 100 times to deal with the rare case of collisions. (There is a very theoretical change that it may fail incorrectly even after 100 times ... but it is many orders of magnitude smaller than overall system unreliability, so this is ok.) Are you interested in taking care of that?

[jkotas]

https://github.com/dotnet/corefx/blob/master/src/System.Runtime/tests/System/DelegateTests.cs would be a good place to add this test.

[danmoseley]

To try your tests:
Here's how to run tests in corefx against your private corelib: https://github.com/dotnet/corefx/blob/c2588415e91ca1d44885ac0bbe9bf8268adc4b48/Documentation/project-docs/developer-guide.md#testing-with-private-coreclr-bits

[mountgellert]

@jkotas sure, will do

[mountgellert]

@jkotas I rebased and squashed the commits

Regards the other question - the test DelegateGetHashCode.PosTest7 does exactly this

[jkotas]

This has a very small probability of failing intermittently, but it is probably ok.

[jkotas]

Thanks!