PoParser PERF #15943
PoParser PERF #15943
Conversation
WalkthroughWalkthroughThe recent update in the OrchardCore.Localization.Core project involves enhancing performance and optimizing memory usage. The inclusion of the Changes
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 Configuration File (
|
|
Benchmarks Before:
After:
The allocation is quite similar but there's some improvement after this PR |
|
Something strange here, 3 unit tests broken, I will check again |
| @@ -55,31 +61,32 @@ public IEnumerable<CultureDictionaryRecord> Parse(TextReader reader) | |||
|
|
|||
| private static string Unescape(string str) | |||
| { | |||
| StringBuilder sb = null; | |||
| var builder = default(Utf16ValueStringBuilder); | |||
There was a problem hiding this comment.
I would just check for \ in the input string. (IndexOf is fast, it's vectorized). If not return the string. Otherwise create the ZString, check the other usages in Orchard, it needs to be disposed.
There was a problem hiding this comment.
I'm not sure why we need to check? BTW the disposing in my mind after the tests pass
There was a problem hiding this comment.
The idea is that you check that the builder is initialized in lots of places, to prevent initializing it if there is nothing to unescape.
My suggestion is to first check if there is anything to encode, by looking for \ in the whole string first. If there is not you return, if there is then you create the builder and don't check anymore.
Looking for \ is very fast as I explained.
There was a problem hiding this comment.
Example:
if (!str.Contains('\\'))
{
return str;
}
using var builder = ZString.CreateStringBuilder();
... // no more checking, just use builderThere was a problem hiding this comment.
I already did that two weeks ago but the few tests were broken, I found the issue and fix it
|
Code is good. Only negative point is that it introduces a new dependency (the only one) for the users who use it outside or OC. And for a marginal perf gain (only for escaped strings, only on startup, for few strings allocations -> this doesn't even show up in the benchmark, maybe the string in the benchmark are not un-escapable). |
I don't think it's a big issue
Can I measure this too? or do you have some suggestions for improvements? |
Well you created the PR to have some improvements, the changes are supposed to decrease allocations, but the benchmark doesn't show any change on allocations. Try to create a benchmark that shows the PR off, make us want to want this PR. |
|
Maybe the allocation is same but it's faster :) |
|
I am not sure. I'd need to see the benchmark. Note that the new code has an early termination that the first one didn't have. I bet this is the only improvement you are seeing, not the ZString usage. |
|
Let the benchmarks decide the life of this PR :) |
Seems you are right after the last benchmark, so let's decide to make it or break it :) |
|
@sebastienros is there a reason why this isn't merged yet? @hishamco is it ready? |
|
Maybe we forgot to merge it, I will do it after I update the branch |
Related to Seb comments in ##15886
Summary by CodeRabbit