Skip to content

Optimize replace_first, replace_last, and truncate#523

Merged
sebastienros merged 4 commits intosebastienros:mainfrom
danielmpetrov:refactor/optimize-string-filters
Oct 24, 2022
Merged

Optimize replace_first, replace_last, and truncate#523
sebastienros merged 4 commits intosebastienros:mainfrom
danielmpetrov:refactor/optimize-string-filters

Conversation

@danielmpetrov
Copy link
Copy Markdown
Contributor

Leverage the string.Concat with ReadOnlySpan<char> - see https://learn.microsoft.com/en-us/dotnet/api/system.string.concat?view=netcore-3.1#system-string-concat(system-readonlyspan((system-char))-system-readonlyspan((system-char))), to remove extra allocations. Since the overload was added in .NET Core 3.0, the optimization can only be applied thereafter (unless we shim the method).

The following scenarios were benchmarked for replace_first and replace_last

{{ "a a a a" | replace_first: "a", "b" }}
{{ "c c c c" | replace_first: "a", "b" }}
{{ "a a a a" | replace_last: "a", "b" }}
{{ "c c c c" | replace_last: "a", "b" }}
BenchmarkDotNet=v0.13.2, OS=Windows 10 (10.0.19044.2130/21H2/November2021Update)
AMD Ryzen 5 2600X, 1 CPU, 12 logical and 6 physical cores
.NET SDK=6.0.402
  [Host]     : .NET 6.0.10 (6.0.1022.47605), X64 RyuJIT AVX2
  DefaultJob : .NET 6.0.10 (6.0.1022.47605), X64 RyuJIT AVX2

Method Input Mean Error StdDev Ratio RatioSD Gen0 Allocated Alloc Ratio
ReplaceFirst_Span a a a a 45.70 ns 0.727 ns 0.645 ns 1.00 0.00 0.0172 72 B 1.00
ReplaceFirst_String a a a a 86.56 ns 1.418 ns 1.184 ns 1.89 0.04 0.0267 112 B 1.56
ReplaceFirst_Span c c c c 16.55 ns 0.301 ns 0.282 ns 1.00 0.00 - - NA
ReplaceFirst_String c c c c 55.64 ns 0.437 ns 0.387 ns 3.36 0.06 - - NA
ReplaceLast_Span a a a a 43.55 ns 0.542 ns 0.507 ns 1.00 0.00 0.0172 72 B 1.00
ReplaceLast_String a a a a 84.02 ns 1.271 ns 1.127 ns 1.93 0.02 0.0267 112 B 1.56
ReplaceLast_Span c c c c 15.98 ns 0.153 ns 0.143 ns 1.00 0.00 - - NA
ReplaceLast_String c c c c 62.40 ns 0.708 ns 0.662 ns 3.91 0.06 - - NA

The following scenarios were benchmarked for truncate

{{ "The cat came back the very next day" | truncate: 13 }}
BenchmarkDotNet=v0.13.2, OS=Windows 10 (10.0.19044.2130/21H2/November2021Update)
AMD Ryzen 5 2600X, 1 CPU, 12 logical and 6 physical cores
.NET SDK=6.0.402
  [Host]     : .NET 6.0.10 (6.0.1022.47605), X64 RyuJIT AVX2
  DefaultJob : .NET 6.0.10 (6.0.1022.47605), X64 RyuJIT AVX2

Method Input Mean Error StdDev Ratio RatioSD Gen0 Allocated Alloc Ratio
Truncate_New The c(...)t day [35] 44.13 ns 0.890 ns 0.832 ns 1.00 0.00 0.0191 80 B 1.00
Truncate_Old The c(...)t day [35] 52.49 ns 0.471 ns 0.418 ns 1.19 0.03 0.0306 128 B 1.60

@hishamco
Copy link
Copy Markdown
Collaborator

Could you please add the benchmark to the PR?

@danielmpetrov
Copy link
Copy Markdown
Contributor Author

That's a bit of a problem, since the benchmark project is not multi targeted, I copy-pasted the implementation twice just to evaluate it. The code is not good to be merged.

@danielmpetrov
Copy link
Copy Markdown
Contributor Author

I cleaned up the benchmarks and pushed them. I think they're good to verify the benefits, but should probably not merge them into the main branch.

@sebastienros
Copy link
Copy Markdown
Owner

I don't see the point of keeping these benchmarks. And I don't like the fact that the old implementation is in them. My preference is to not have them here. You used them to prove the new implementation is better, good job, that's sufficient. I don't expect someone else to reuse them. We could have a set of benchmarks for all current implementations the day we start tracking it.

@hishamco
Copy link
Copy Markdown
Collaborator

Of course he add them to prove it's better ;), if that's sufficient @danielmpetrov please remove them, then we could merge

Thanks a lot for your contribution

This reverts commit d0451cb.
@sebastienros sebastienros merged commit 3664c71 into sebastienros:main Oct 24, 2022
@danielmpetrov danielmpetrov deleted the refactor/optimize-string-filters branch October 24, 2022 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants