Slow performance of ILocalizationProvider.Translate<T>()

[stefanolsen]

During a code profiling session, I found that 36% of the running time is spent on this line:

LocalizationProvider/common/src/DbLocalizationProvider/LocalizationProvider.cs

Line 237 in 4c0d66e

return JsonConvert.DeserializeObject<T>(jsonToken.ToString(),

The test scenario is a start page with various blocks. Two of these blocks are instances of the same block type, which uses the ILocalizationProvider.Translate<T>() method.

Is there a way to get rid of both serialization and deserialization in that method?

[hangy]

The JsonSerializer being recreated for each method call seems have be a way bigger impact than the (de-)serialization if the method is called repeatedly

[stefanolsen]

@hangy Looks like a good optimization.

But is there a way to fill in the properties that avoids converting to and from JSON?

[valdisiljuconoks]

noice! :) @stefanolsen there might be some options for that. json route was the lazy old man path

[stefanolsen]

I guess it could be filled in with Reflection. Sort of like how Newtonsoft.Json fills in properties on an object. Like:

1. Create an object from type parameter.
2. Iterate all string properties and fill in from the provided translations. This might require some slicing of strings, to disregard the namespace parts of the translation keys.
3. If a property is an object, repeat in a nested level.

Although, Newtonsoft.Json runs through all the JSON tokens and then fills properties. While this is the opposite way. But it is the gist of my idea.

[stefanolsen]

@valdisiljuconoks I think I cracked the case on replacing JSON conversion with reflection. It is still rather crude code, and it is not yet benchmarked. But it passes all the unit tests.

[valdisiljuconoks]

never trust unit tests ;)

but you are awesome guys! 🤙 I never had time to look closely on perf. there are many unoptimized anomalies..

[valdisiljuconoks]

you also motivated me to work on v9 specifically focusing on bottlenecks

[stefanolsen]

You are right about the unit tests. They are not a full proof. Can we do a prerelease NuGet (marked so solutions doesn't auto-update)? Then I could test with a solution where we use regular, nested and static resource classes.

[stefanolsen]

If you do a version 9, then I would suggest to:

• Change TypeFactory to regular dependency injection. It seems to allocate too much memory.
• Optimize ResourceKeyBuilder further.
• Consider replacing the queries with regular service class methods. They are not particularly slow. But the creation of them seems slow.
• Store the result of GetAllResources as a dictionary, so it is cached like that, as it is much faster to pull specific resources by name this way.

[stefanolsen]

@valdisiljuconoks Do you want me to merge this PR into a v9 branch instead of master?
It could also be nice to look into the data layer, like test the performance of loading from SQL Server, and the caching of data. But probably in a v9 release.

[valdisiljuconoks]

I changed base already for the PR. will go through later tonight and merge all pending stuff. we can push pre-release nuget if you wanna test out.

[valdisiljuconoks]

#345

[valdisiljuconoks]

@stefanolsen are you looking for testing this is opti? I'm targeting v9 to net9...

[stefanolsen]

@valdisiljuconoks Yes, I will test on Optimizely CMS 12.

[valdisiljuconoks]

that one will be still running on net8?

[stefanolsen]

@valdisiljuconoks Yes, .NET 8. I also have a project using .NET 9. So cross-compiling for both will be great in v9.

[stefanolsen]

@valdisiljuconoks Just did a code profiling session on the same site and page, with the v9 branch build. And the results were very nice.

Now the bottleneck is moved to CachedGetAllResourcesHandler. Apparently, it reads a lot of individual keys from cache, which incurs waiting time on a lock. It also reads ConcurrentDictionary.Count every time. Both of these take about the same time.

I wonder if there is a way to avoid the KnownKeys, and maybe cache the data in a different way. So that the loading on each request gets less intensive.

[valdisiljuconoks]

@stefanolsen yeah, I never liked KnownKeys. try another round - GetAllResources is now using dictionary

[stefanolsen]

@valdisiljuconoks Nice! Is it materialized to dictionary before or after adding to cache?

Regarding caching. Would you prefer to cache every single resource separately (one cache key for each) and a list of all resources? Sort of like how Optimizely CMS caches many things internally.

Or would you like to cache the result of conversion (like how Optimizely CMS caches read-only content instances)? This would require some changes to the data layer, so that ReflectionConverter can load a scoped set of resources from the data store. Then it would only cache the resulting objects, so they don't need to be done on every request.
I get that raw ASP.NET should be supported as well. So in this case those helpers would probably have to cache the "resulting" items or lists as well.

[valdisiljuconoks]

GetAllResources already returns a dictionary. But I haven't decided on cache heuristics.. it's next