From 1dd10eabd4d8745e6edb3eb5dec2f75630254f26 Mon Sep 17 00:00:00 2001 From: Markus Johansson Date: Tue, 1 Apr 2025 11:39:22 +0200 Subject: [PATCH] Use StringComparison.Ordinal in hot paths (#18893) --- src/Umbraco.Core/Extensions/UriExtensions.cs | 6 +- src/Umbraco.Core/Routing/DomainUtilities.cs | 2 +- .../Routing/NewDefaultUrlProvider.cs | 4 +- src/Umbraco.Core/Routing/PublishedRouter.cs | 2 +- src/Umbraco.Core/Routing/UriUtility.cs | 4 +- src/Umbraco.Core/UriUtilityCore.cs | 2 +- .../StringIndexOfBenchmarks.cs | 94 +++++++++++++++++++ .../StringStartsWithBenchmarks.cs | 52 ++++++++++ 8 files changed, 156 insertions(+), 10 deletions(-) create mode 100644 tests/Umbraco.Tests.Benchmarks/StringIndexOfBenchmarks.cs create mode 100644 tests/Umbraco.Tests.Benchmarks/StringStartsWithBenchmarks.cs diff --git a/src/Umbraco.Core/Extensions/UriExtensions.cs b/src/Umbraco.Core/Extensions/UriExtensions.cs index 3868586e44..6d3b47141f 100644 --- a/src/Umbraco.Core/Extensions/UriExtensions.cs +++ b/src/Umbraco.Core/Extensions/UriExtensions.cs @@ -22,7 +22,7 @@ public static class UriExtensions /// Everything else remains unchanged, except for the fragment which is removed. public static Uri Rewrite(this Uri uri, string path) { - if (path.StartsWith("/") == false) + if (path.StartsWith("/", StringComparison.Ordinal) == false) { throw new ArgumentException("Path must start with a slash.", "path"); } @@ -42,12 +42,12 @@ public static class UriExtensions /// Everything else remains unchanged, except for the fragment which is removed. public static Uri Rewrite(this Uri uri, string path, string query) { - if (path.StartsWith("/") == false) + if (path.StartsWith("/", StringComparison.Ordinal) == false) { throw new ArgumentException("Path must start with a slash.", "path"); } - if (query.Length > 0 && query.StartsWith("?") == false) + if (query.Length > 0 && query.StartsWith("?", StringComparison.Ordinal) == false) { throw new ArgumentException("Query must start with a question mark.", "query"); } diff --git a/src/Umbraco.Core/Routing/DomainUtilities.cs b/src/Umbraco.Core/Routing/DomainUtilities.cs index 560285b5cf..2d9be885a3 100644 --- a/src/Umbraco.Core/Routing/DomainUtilities.cs +++ b/src/Umbraco.Core/Routing/DomainUtilities.cs @@ -388,7 +388,7 @@ namespace Umbraco.Cms.Core.Routing public static Uri ParseUriFromDomainName(string domainName, Uri currentUri) { // turn "/en" into "http://whatever.com/en" so it becomes a parseable uri - var name = domainName.StartsWith("/") && currentUri != null + var name = domainName.StartsWith("/", StringComparison.Ordinal) && currentUri != null ? currentUri.GetLeftPart(UriPartial.Authority) + domainName : domainName; var scheme = currentUri?.Scheme ?? Uri.UriSchemeHttp; diff --git a/src/Umbraco.Core/Routing/NewDefaultUrlProvider.cs b/src/Umbraco.Core/Routing/NewDefaultUrlProvider.cs index 3b140021a8..314f2c5598 100644 --- a/src/Umbraco.Core/Routing/NewDefaultUrlProvider.cs +++ b/src/Umbraco.Core/Routing/NewDefaultUrlProvider.cs @@ -155,7 +155,7 @@ public class NewDefaultUrlProvider : IUrlProvider } // need to strip off the leading ID for the route if it exists (occurs if the route is for a node with a domain assigned) - var pos = route.IndexOf('/'); + var pos = route.IndexOf('/', StringComparison.Ordinal); var path = pos == 0 ? route : route.Substring(pos); var uri = new Uri(CombinePaths(d.Uri.GetLeftPart(UriPartial.Path), path)); @@ -237,7 +237,7 @@ public class NewDefaultUrlProvider : IUrlProvider // extract domainUri and path // route is / or / - var pos = route.IndexOf('/'); + var pos = route.IndexOf('/', StringComparison.Ordinal); var path = pos == 0 ? route : route[pos..]; DomainAndUri? domainUri = pos == 0 ? null diff --git a/src/Umbraco.Core/Routing/PublishedRouter.cs b/src/Umbraco.Core/Routing/PublishedRouter.cs index b184434405..472fe47122 100644 --- a/src/Umbraco.Core/Routing/PublishedRouter.cs +++ b/src/Umbraco.Core/Routing/PublishedRouter.cs @@ -440,7 +440,7 @@ public class PublishedRouter : IPublishedRouter return false; } - var pos = alias.IndexOf('/'); + var pos = alias.IndexOf('/', StringComparison.Ordinal); if (pos > 0) { // recurse diff --git a/src/Umbraco.Core/Routing/UriUtility.cs b/src/Umbraco.Core/Routing/UriUtility.cs index 1869641fb5..c6920d7733 100644 --- a/src/Umbraco.Core/Routing/UriUtility.cs +++ b/src/Umbraco.Core/Routing/UriUtility.cs @@ -144,7 +144,7 @@ public sealed class UriUtility var idxOfScheme = relativeUrl.IndexOf(@"://", StringComparison.Ordinal); if (idxOfScheme != -1) { - var idxOfQM = relativeUrl.IndexOf('?'); + var idxOfQM = relativeUrl.IndexOf('?', StringComparison.Ordinal); if (idxOfQM == -1 || idxOfQM > idxOfScheme) { return relativeUrl; @@ -226,7 +226,7 @@ public sealed class UriUtility throw new ArgumentNullException(nameof(absolutePath)); } - if (!absolutePath.StartsWith("/")) + if (!absolutePath.StartsWith("/", StringComparison.Ordinal)) { throw new FormatException("The absolutePath specified does not start with a '/'"); } diff --git a/src/Umbraco.Core/UriUtilityCore.cs b/src/Umbraco.Core/UriUtilityCore.cs index 67e978b8d5..cefeed5e6f 100644 --- a/src/Umbraco.Core/UriUtilityCore.cs +++ b/src/Umbraco.Core/UriUtilityCore.cs @@ -4,7 +4,7 @@ namespace Umbraco.Cms.Core; public static class UriUtilityCore { - public static bool HasScheme(string uri) => uri.IndexOf("://", StringComparison.InvariantCulture) > 0; + public static bool HasScheme(string uri) => uri.IndexOf("://", StringComparison.Ordinal) > 0; public static string StartWithScheme(string uri) => StartWithScheme(uri, null); diff --git a/tests/Umbraco.Tests.Benchmarks/StringIndexOfBenchmarks.cs b/tests/Umbraco.Tests.Benchmarks/StringIndexOfBenchmarks.cs new file mode 100644 index 0000000000..d3ce096e7c --- /dev/null +++ b/tests/Umbraco.Tests.Benchmarks/StringIndexOfBenchmarks.cs @@ -0,0 +1,94 @@ +using BenchmarkDotNet.Attributes; +using Umbraco.Tests.Benchmarks.Config; + +namespace Umbraco.Tests.Benchmarks; + +[QuickRunWithMemoryDiagnoserConfig] +public class StringIndexOfBenchmarks +{ + private string _domainName = "https://www.lorem-ipsum.com"; + + [Benchmark()] + public bool IndexOf_Original() + { + return _domainName.IndexOf("://") > 0; + } + + [Benchmark()] + public bool IndexOf_Ordinal() + { + return _domainName.IndexOf("://", StringComparison.Ordinal) > -1; + } + + [Benchmark()] + public bool IndexOf_Invariant() + { + return _domainName.IndexOf("://", StringComparison.InvariantCulture) > -1; + } + + [Benchmark()] + public bool IndexOf_Span() + { + return _domainName.AsSpan().IndexOf("://", StringComparison.InvariantCulture) > -1; + } + + [Benchmark()] + public bool Contains() + { + return _domainName.Contains("://"); + } + + [Benchmark()] + public bool Contains_Ordinal() + { + return _domainName.Contains("://", StringComparison.Ordinal); + } + + [Benchmark()] + public bool Contains_Invariant() + { + return _domainName.Contains("://", StringComparison.InvariantCulture); + } + + [Benchmark()] + public bool Contains_Span_Ordinal() + { + return _domainName.AsSpan().Contains("://", StringComparison.Ordinal); + } + + [Benchmark()] + public bool Contains_Span_Invariant() + { + return _domainName.AsSpan().Contains("://", StringComparison.InvariantCulture); + } + + [Benchmark()] + public bool Span_Index_Of() + { + var uri = "https://www.lorem-ipsum.com".AsSpan(); + return uri.IndexOf("#") > -1; + } + + [Benchmark()] + public bool Span_Index_Of_Ordinal() + { + var uri = "https://www.lorem-ipsum.com".AsSpan(); + return uri.IndexOf("#".AsSpan(), StringComparison.Ordinal) > -1; + } + + /* + | Method | Mean | Error | StdDev | Allocated | + |------------------------ |-----------:|-----------:|----------:|----------:| + | IndexOf_Original | 916.918 ns | 73.7556 ns | 4.0428 ns | - | + | IndexOf_Ordinal | 4.083 ns | 1.5083 ns | 0.0827 ns | - | + | IndexOf_Invariant | 12.941 ns | 3.7574 ns | 0.2060 ns | - | + | IndexOf_Span | 13.076 ns | 3.0666 ns | 0.1681 ns | - | + | Contains | 2.828 ns | 0.3648 ns | 0.0200 ns | - | + | Contains_Ordinal | 4.368 ns | 0.9882 ns | 0.0542 ns | - | + | Contains_Invariant | 12.986 ns | 2.3526 ns | 0.1290 ns | - | + | Contains_Span_Ordinal | 2.924 ns | 0.1593 ns | 0.0087 ns | - | + | Contains_Span_Invariant | 12.502 ns | 1.4153 ns | 0.0776 ns | - | + | Span_Index_Of | 1.741 ns | 0.9093 ns | 0.0498 ns | - | + | Span_Index_Of_Ordinal | 1.809 ns | 0.3703 ns | 0.0203 ns | - | + */ +} diff --git a/tests/Umbraco.Tests.Benchmarks/StringStartsWithBenchmarks.cs b/tests/Umbraco.Tests.Benchmarks/StringStartsWithBenchmarks.cs new file mode 100644 index 0000000000..a3b957a445 --- /dev/null +++ b/tests/Umbraco.Tests.Benchmarks/StringStartsWithBenchmarks.cs @@ -0,0 +1,52 @@ +using BenchmarkDotNet.Attributes; +using Umbraco.Tests.Benchmarks.Config; + +namespace Umbraco.Tests.Benchmarks; + +[QuickRunWithMemoryDiagnoserConfig] +public class StringStartsWithBenchmarks +{ + + private string _domainName = "domain1.com"; + + [Benchmark(Baseline = true)] + public bool Original() + { + return _domainName.StartsWith("/"); + } + + [Benchmark()] + public bool Ordinal() + { + return _domainName.StartsWith("/",StringComparison.Ordinal); + } + + [Benchmark()] + public bool Invariant() + { + return _domainName.StartsWith("/", StringComparison.InvariantCulture); + } + + [Benchmark()] + public bool FirstChar() + { + return _domainName.Length > 0 && _domainName[0] == '/'; + } + + [Benchmark()] + public bool Span() + { + return _domainName.AsSpan().StartsWith("/".AsSpan(),StringComparison.Ordinal); + } + + /* + | Method | Mean | Error | StdDev | Allocated | + |---------- |------------:|-----------:|----------:|----------:| + | Original | 255.2239 ns | 10.9432 ns | 0.5998 ns | - | + | Ordinal | 0.1784 ns | 0.3070 ns | 0.0168 ns | - | + | Invariant | 4.1270 ns | 0.4990 ns | 0.0274 ns | - | + | FirstChar | 0.0127 ns | 0.0098 ns | 0.0005 ns | - | + | Span | 0.8000 ns | 0.4526 ns | 0.0248 ns | - | + */ + +}