Fix ResourceManagerStringLocalizerFactory caching

[sebastienros]

The cache key was using reflection which was visible in allocation profiles since it's called for every view that is rendered.

Before:

After

Allocations

Type	Bytes Before	Bytes After
GetCustomAttributeRecords	912,000	0
String	2,196,946	2,017,262

[JamesNK]

Can GetResourcePrefix return the same result for different Types? Basically, I'm wondering if this change will mean that additional localizers will end up in the cache because the key isn't shared between types.

Either way, add a comment to that effect explaining why AssemblyQualifiedName is preferable to baseName.

[sebastienros]

I had looked at it a while ago when I first worked on it. Will check and come back with an answer.

[sebastienros]

Can GetResourcePrefix return the same result for different Types

I don't think so, since it will have the type name in the result: return baseNamespace + "." + resourcesRelativePath + TrimPrefix(typeInfo.FullName, baseNamespace + ".");

[JamesNK]

This lambda has a closure because it needs to get the type. Is there a reason why AssemblyQualifiedName is preferable as a key to the type itself? If the type is the key then it can be accessed in the lambda without a closure and it can be made static.

[sebastienros]

The cache is used by another method that is not using a Type

[davidfowl]

This right?

aspnetcore/src/Localization/Localization/src/ResourceManagerStringLocalizerFactory.cs

Lines 190 to 197 in a318a56

	return _localizerCache.GetOrAdd($"B={baseName},L={location}", _ =>
	{
	var assemblyName = new AssemblyName(location);
	var assembly = Assembly.Load(assemblyName);
	baseName = GetResourcePrefix(baseName, location);
	return CreateResourceManagerStringLocalizer(assembly, baseName);
	});

[davidfowl]

This change looked simple at first, but now I'm not so sure. The methods before were calling a virtual GetResourcePrefix to resolve the cache key, not that's no longer being called so this is a breaking change if you changed that method expecting to cache based on that key (I don't know if that happens in practice though). One workaround is to only do this change if the exact type is ResourceManagerStringLocalizerFactory

[pranavkm]

Yes. It also avoids the keys conflating in case the derived implementation returned AssemblyQualifiedName. Alternatively, you could cache these as ConcurrentDictionary<Type, ResourceManagerStringLocalizer>. There isn't much reason to have the keys be the type name.

[sebastienros]

@pranavkm The IStringLocalizer has two Create methods, one using a Type, the other using two strings. The current implementation uses a single CD to cache the result of both methods, hence it's using a string key. I am not changing that in this PR.

@davidfowl Before this PR the key was using GetResourcePrefix(TypeInfo) directly. Now it's still used in the lambda when the item is added in the cache. This value only differs by type, so by using the type name as the key we are having a direct mapping between the cache entry and the value of this method. The only "breaking change" is that this was called for each cache lookup, and now only once (the goal of this PR since it's reflection heavy).

@JamesNK about the lambda creating a closure, that's why I make a call to the dictionary without the lambda right before. And the type can't be used as the key since the CD is used by the other interface method (that is not using a type).

[dougbu]

Conflict is due to #18873 The file needs the new MIT license