Fix CollectionHelper.GetHashCode ignoring values of dictionaries

[fredericDelaporte]

This is causing FilterKey and QueryKey for the same queries but having different parameter values to always have the same hashcode, which is inefficient for keys lookup.

This PR is build over #1612, which includes the first four commits we can currently see here.

[hazzik]

I would like this to go to master first. Need to have a separate unit test for the method.

[hazzik]

Actually, do not want this change. The GetHashCode method should return the same value for the same object, but the uniqueness of these values are not required.

[fredericDelaporte]

The GetHashCode method should return the same value for the same object, but the uniqueness of these values are not required.

Yes it is not required by GetHashCode, I am well aware of it. But this is still a bad thing for performances, since this means the cache lookup will be unable to benefit of the hashtable for distinguishing two queries differing only by their parameters values.

So when a query is cached for many different parameter values, its lookup in cache is an O(n) operation due to the current implementation (with n being the number of different parameter values).

[fredericDelaporte]

Need to have a separate unit test for the method.

Done. This PR is now independent of #1612.

Still, if #1612 is merged before, it would then be better to also adjust query key and filter key hashcode tests (which cannot be adjusted without re-doing the fixes #1612 does).

[hazzik]

Maybe better to make an overload that accepts IEqualityComparer<T> and KeyValuePairComparer<TK, TV> instead?

[fredericDelaporte]

The overload added here is a specialization of the existing public static int GetHashCode<T>(IEnumerable<T> coll), allowing to handle the dictionary case specifically without source changes on the GetHashCode callers side: with a recompilation, when they supply a dictionary, that is this new overload which gets called, allowing to fix the issue.

Adding comparer overloads would require to change the calling code too (8 cases, in cache, engine and mapping namespaces). I would rather see this as a separate refactoring.

[hazzik]

But this fix is too specialised. The problem exists for all struct types which can contain references.

[fredericDelaporte]

This fix matches what NHibernate currently uses. CollectionHelper shoudn't be public in the first place but internal, and shouldn't aim at being generic but tailored to what NHibernate needs.

[hazzik]

Can we make this method internal then?

[fredericDelaporte]

Done. But now the test heavily relies on reflection, and it would be better to merge #1612 first then add tests here for checking QueryKey and FilterKey benefit of the internal overload. (Their tests fixtures are currently broken, causing them to be unusable. #1612 fixes them.)

[hazzik]

CollectionHelper in the current form shall die. I propose to replace it with a set of content equality comparers (I will probably prepare a PR).

[fredericDelaporte]

CollectionHelper in the current form shall die.

I agree. Nonetheless it should not prevent fixing its issues while it is still alive. So unless the switch to equality comparer can be done soon, better still fix its current implementation.

[hazzik]

Maybe it's better to add a long overdue [InternalsVisibleToAttribute]?

[fredericDelaporte]

This would require strong signing the test assembly too, because for assemblies to be friend, either both are unsigned or both are signed. An unsigned one cannot be friend of a signed one.

[hazzik]

I've re-checked the code. CollectionHelper.GetHashCode has 2 invocations that accept a dictionary, both of them use the same type Dictionary<string, TypedValue>. I've submitted a PR with the GetHashCode method which accepts an IEqualityComparer<T> and also a specific comparer for this type of a dictionary. In total CollectionHelper.GetHashCode has 8 invocation, which changed in the PR as below:

1. 2 invocations mentioned above;
2. 1 invocation with an array - changed to ArrayHelper.ArrayGetHashCode;
3. 3 invocations with an HashSet<>/ISet<> - changed to use set's comparer;
4. 2 invocations with List<Column> - unchanged.

[hazzik]

Replaced by #1694