Add ITranslationProvider.LoadTranslationsAsync() #15886
Conversation
WalkthroughThe updates across OrchardCore's localization modules primarily focus on transitioning from synchronous to asynchronous methods. This shift enhances the system's efficiency by supporting non-blocking operations, especially beneficial when loading translations from various data stores. Key interfaces and classes within the localization framework have been refactored to include async methods, with corresponding deprecations of their synchronous counterparts. Changes
Assessment against linked issues
The changes effectively address the primary objectives outlined in the linked issue #11356 by incorporating asynchronous methods across the localization interfaces and their implementations. This refactoring ensures that the system can handle translation loading operations asynchronously, thereby avoiding blocking calls and enhancing performance when interacting with various data storage solutions. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| ? GetAllStringsFromCultureHierarchyAsync(culture) | ||
| : GetAllStringsAsync(culture).ToEnumerable(); |
There was a problem hiding this comment.
Consider maintaining asynchronous flow in GetAllStrings.
- : GetAllStringsAsync(culture).ToEnumerable();
+ : GetAllStringsAsync(culture);This change avoids potential performance bottlenecks by not blocking the thread while converting IAsyncEnumerable to IEnumerable.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| ? GetAllStringsFromCultureHierarchyAsync(culture) | |
| : GetAllStringsAsync(culture).ToEnumerable(); | |
| ? GetAllStringsFromCultureHierarchyAsync(culture) | |
| : GetAllStringsAsync(culture); |
src/OrchardCore/OrchardCore.Localization.Core/PortableObject/PortableObjectStringLocalizer.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.Localization.Core/PortableObject/PortableObjectStringLocalizer.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.Localization.Core/PortableObject/PortableObjectStringLocalizer.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.Localization.Core/PortableObject/PortableObjectStringLocalizer.cs
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.Localization.Core/PortableObject/PortableObjectStringLocalizer.cs
Show resolved
Hide resolved
| return includeParentCultures | ||
| ? GetAllStringsFromCultureHierarchyAsync(culture).GetAwaiter().GetResult() | ||
| : GetAllStringsAsync(culture).ToEnumerable(); |
There was a problem hiding this comment.
Consider maintaining asynchronous flow in GetAllStrings.
- return includeParentCultures
- ? GetAllStringsFromCultureHierarchyAsync(culture).GetAwaiter().GetResult()
- : GetAllStringsAsync(culture).ToEnumerable();
+ return includeParentCultures
+ ? await GetAllStringsFromCultureHierarchyAsync(culture).ToListAsync()
+ : await GetAllStringsAsync(culture).ToListAsync();This change avoids potential performance bottlenecks by not blocking the thread while converting IAsyncEnumerable to IEnumerable.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| return includeParentCultures | |
| ? GetAllStringsFromCultureHierarchyAsync(culture).GetAwaiter().GetResult() | |
| : GetAllStringsAsync(culture).ToEnumerable(); | |
| return includeParentCultures | |
| ? await GetAllStringsFromCultureHierarchyAsync(culture).ToListAsync() | |
| : await GetAllStringsAsync(culture).ToListAsync(); |
src/OrchardCore/OrchardCore.Localization.Core/PortableObject/PortableObjectStringLocalizer.cs
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.Localization.Core/PortableObject/PortableObjectHtmlLocalizer.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Since the ultimate consumers of these services, PortableObjectHtmlLocalizer and PortableObjectStringLocalizer retrieve the localization values synchronously, I doubt we get a performance benefit but there is a chance of deadlocks.
Please provide end-to-end benchmarks for these services so we can see if we gain something.
src/OrchardCore/OrchardCore.Localization.Abstractions/ITranslationProvider.cs
Outdated
Show resolved
Hide resolved
| /// <inheritdoc /> | ||
| public async Task<CultureDictionary> GetDictionaryAsync(CultureInfo culture) | ||
| { | ||
| var cachedDictionary = _cache.GetOrCreate(CacheKeyPrefix + culture.Name, k => new Lazy<Task<CultureDictionary>>(async () => |
There was a problem hiding this comment.
This now caches not the dictionary but a Task that produces the dictionary. While this looks working at a first glance, this is not what should really happen. Rather, the dictionary itself needs to be cached. I don't think the Lazy makes sense here anyway.
| <ItemGroup> | ||
| <PackageReference Include="System.Linq.Async" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
It's necessary to convert from IAsyncEnumerable to Enumberable
There was a problem hiding this comment.
It still builds if I remove it.
There was a problem hiding this comment.
Really!! I got 4 errors, how the IAsyncEnumerable extension methods be there without referencing a proper package?
src/OrchardCore/OrchardCore.Localization.Core/PortableObject/PoParser.cs
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.Localization.Core/PortableObject/PoParser.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Zoltán Lehóczky <zoltan.lehoczky@lombiq.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (1)
src/OrchardCore/OrchardCore.Localization.Core/LocalizationManager.cs (1)
39-62: Consider caching the dictionary directly instead of the task to avoid potential issues with multiple concurrent requests.
This comment was marked as off-topic.
This comment was marked as off-topic.
This reverts commit 93a802e.
There was a problem hiding this comment.
If the other changes in the PR prove to be suitable, then ILocalizationFileLocationProvider can be made async too.
There was a problem hiding this comment.
Why, it just retrieve a set of locations
| /// <inheritdoc /> | ||
| public CultureDictionary GetDictionary(CultureInfo culture) | ||
| { | ||
| var cachedDictionary = _cache.GetOrCreate(CacheKeyPrefix + culture.Name, k => new Lazy<CultureDictionary>(() => |
There was a problem hiding this comment.
The Lazyness here is important as it prevents r-entrancy. Without it multiple threads (concurrent requests?) can get into the body and run. The MemoryCache is not blocking on the factory body. With the Lazy, all concurrent requests can potentially create a Lazy instance, but only one will be added to the cache and executed.
| /// <inheritdoc /> | ||
| public async Task<CultureDictionary> GetDictionaryAsync(CultureInfo culture) | ||
| { | ||
| var cachedDictionary = await _cache.GetOrCreateAsync(CacheKeyPrefix + culture.Name, async (e) => |
There was a problem hiding this comment.
Sicne we don't want entries to be revoked here (we don't, right? even on memory pressure), we can use a keyed Dictionary and remove the prefix on the key (because there can't be conflicts anymore). I did it recently, I am sure you will recall.
There was a problem hiding this comment.
Can we use ConccurentDictionary?
| { | ||
| if (sb == null) | ||
| sb = new StringBuilder(str.Length); |
There was a problem hiding this comment.
It's related to PoParser PERF https://github.com/OrchardCMS/OrchardCore/pull/15886/files#r1587065200
| { | ||
| private static readonly Dictionary<char, char> _escapeTranslations = new() |
There was a problem hiding this comment.
Perfect candidate for FrozenDictionary.
Or just remove this dictionary and use a switch the compiler is clever enough to use a dictionary of ifs based on the number of items for best performance, so we don't have to think about it too much.
There was a problem hiding this comment.
I will create another PR for that because it might need extra testing and benchmarking
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
Fixes #11356
No breaking changes