string.Split to Span.Split #46098
string.Split to Span.Split #46098
Conversation
| @@ -62,16 +63,35 @@ public static async Task RetrieveClaimsAsync(LdapSettings settings, ClaimsIdenti | |||
| { | |||
| // Example distinguished name: CN=TestGroup,DC=KERB,DC=local | |||
| var groupDN = $"{Encoding.UTF8.GetString((byte[])group)}"; | |||
| var groupCN = groupDN.Split(',')[0].Substring("CN=".Length); | |||
|
|
|||
| if (!TryGetGroupCN(groupDN, out var groupCN)) | |||
There was a problem hiding this comment.
There aren't any unit tests for this area. So this change could be reverted if we aren't confident.
There was a problem hiding this comment.
This will conflict with https://github.com/dotnet/aspnetcore/pull/45649/files. Revert changes in this file.
| @@ -3,9 +3,9 @@ | |||
|
|
|||
| namespace Microsoft.AspNetCore.Rewrite.ApacheModRewrite; | |||
|
|
|||
| internal sealed class FlagParser | |||
| internal static class FlagParser | |||
There was a problem hiding this comment.
This was just silly, both the class and the _ruleFlagLookup dictionary were allocated every time this type was used.
| // non-allocating version of mergedCodes.Split(';').Length | ||
| var count = 1; | ||
| var index = 0; | ||
| while (index < mergedCodes.Length) | ||
| { | ||
| var semiColonIndex = mergedCodes.IndexOf(';', index); | ||
| if (semiColonIndex < 0) | ||
| { | ||
| break; | ||
| } | ||
| count++; | ||
| index = semiColonIndex + 1; | ||
| } | ||
| return count; |
There was a problem hiding this comment.
This one makes me a little sad. Feels like something missing API?
cc @stephentoub
There was a problem hiding this comment.
finally got around to do it. PR here: dotnet/runtime#80662
BrennanConroy
requested review from
Tratcher,
halter73,
JamesNK,
mgravell and
javiercn
as code owners
| @@ -154,25 +154,27 @@ private static EdgeKey CreateEdgeKey(string host) | |||
| return EdgeKey.WildcardEdgeKey; | |||
| } | |||
|
|
|||
| var hostParts = host.Split(':'); | |||
| if (hostParts.Length == 1) | |||
| Span<Range> hostParts = stackalloc Range[3]; | |||
There was a problem hiding this comment.
Why Range 3 when we only support lengths 1 and 2?
There was a problem hiding this comment.
We need to know if there were more than 2 for the error case
| @@ -62,16 +63,35 @@ public static async Task RetrieveClaimsAsync(LdapSettings settings, ClaimsIdenti | |||
| { | |||
| // Example distinguished name: CN=TestGroup,DC=KERB,DC=local | |||
| var groupDN = $"{Encoding.UTF8.GetString((byte[])group)}"; | |||
| var groupCN = groupDN.Split(',')[0].Substring("CN=".Length); | |||
|
|
|||
| if (!TryGetGroupCN(groupDN, out var groupCN)) | |||
There was a problem hiding this comment.
This will conflict with https://github.com/dotnet/aspnetcore/pull/45649/files. Revert changes in this file.
src/Middleware/Localization/src/CookieRequestCultureProvider.cs
Outdated
Show resolved
Hide resolved
ghost
added this to the 
|
Very nice! Are you planning to tackle routing @BrennanConroy ? |
|
Maybe in the future. I don't see it being worth the effort right now. Routing already does its own custom span-like string splitting via PathSegment and it flows everywhere (20ish files including benchmarks and tests). |
New
ReadOnlySpan<char>.Splitmethods introduced in dotnet/runtime#76186 can avoid the string+array allocations fromstring.Splitin some cases. Went through our code base and found where we can use the new methods.