Optimize Visitors (#105)

[SergeiPavlov]

• Optimize Visitors

[alex-kulakov]

I have a better idea - avoid using .GetGenericDefenition() for equality checks. Instead, use .IsOfGenericType(). It will not pollute memory with multiple values of generic definition for multiple keys (for instance, type Func<int> and type Func<long> each will have entry with value to the same Func<> definition). What I see is that you pollute cache with useless entries which will not ever be true for the following equality checks. So, what's the point of this cache?

MemberComlilerProvider.GetCompilerKey() - caching here is useless. Now, if we speak of built-in member compilers, the only member type which can cause call of GetGenericDefinition is Nullable<> which has 4 compilers for its members. I would make following changes in the method for the case of generic target type

var targetType = canonicalMember.ReflectedType;
if (targetType.IsGenericType) {
  targetType = targetType.IsGenericTypeDefinition
    ? targetType
    : targetType.GetGenericTypeDefinition();
  if (canonicalMember is FieldInfo)
    canonicalMember = targetType.GetField(canonicalMember.Name);
  else if (canonicalMember is MethodInfo methodInfo) {
    canonicalMember = GetCanonicalMethod(methodInfo, targetType.GetMethods());
  }
  else if (canonicalMember is ConstructorInfo constructorInfo)
    canonicalMember = GetCanonicalMethod(constructorInfo, targetType.GetConstructors());
  else
    canonicalMember = null;
}

For case of Nullable<> it is enough because typeof (Nullable<>) is generic type definition, plus it has future-proof check for other cases. How many member compilers with generic target type do you have? and how many of them have declared exact generic type? if they all have target type of generic type definition then GetGenericTypeDefinition() will never be called in this case.

UpgradeHintProcessor.BuildRenameFieldHintsForGenericTypes() and .GetGenericTypes() - the same, useless in my opinion. In most cases it is done once while Domain is upgrading, and you want to keep the entries in cache for the rest of Domain's life. Even for storage nodes it still doesn't really matter that much. Since you upgrade the nodes, it is already not very fast process with a lot of time spent on data manipulation on server side. And once again, you spend some memory for things which won't be in use ever again during Domain lifetime. Unlike you, other users can actually use upgraded domain when it is built instead of building it in Skip mode one more time (and these methods will not be called in SKIP), so caching these generic definitions is waste of memory.

TypeHelper.Activate() - probably the only case when caching could make sense. BUT there is no such test which reached this code region, among all the tests in Xtensive.Orm.Tests project, at least for MS SQL Server provider.

All the mentioned above make me think that this cache is not very needed.

[SergeiPavlov]

One of motivations to cache was that .NET8 caches it: https://github.com/dotnet/runtime/blob/main/src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs#L1643

I agree, it is minor improvement. I've reverted it. Anyway we will get it for free with .NET8

Your code snippet is applied with some refactoring

[SergeiPavlov]

BTW what was intention of this branching?: https://github.com/DataObjects-NET/dataobjects-net/pull/320/files#diff-d048690964f7d1617a41f1895bf202d0ef07811ccfac434f34a6a6a706665f28L334

Both branches do the same thing with the same arguments

I'm removed this redundancy

[alex-kulakov]

The branching was probably accident, I agree it should be removed.