Conversation
Add a fast path that calls StringBuilder.Append/Insert/Replace(char) when Rune.IsBmp holds. Move the existing span path into a private method so that the stackalloc stays out of the fast path. This follows the pattern used in dotnet#127939.
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/area-system-runtime |
There was a problem hiding this comment.
Pull request overview
This PR updates StringBuilder’s Rune overloads to add a BMP fast path that routes to the existing char-based APIs, with the non-BMP path moved into private helpers.
Changes:
- Add
value.IsBmp/oldRune.IsBmp && newRune.IsBmpfast paths that callAppend(char),Insert(char), andReplace(char, char, ...). - Move the existing span +
stackallocconversion logic into private*Runehelper methods. - Add
Debug.Assert(!value.IsBmp)in the new slow-path helpers forAppend/Insert.
Suppressed comments (2)
src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs:1397
- Same concern as AppendRune: this helper contains a tiny fixed-size stackalloc that the JIT may optimize and still inline, reintroducing stackalloc / GS-cookie overhead into the public fast path. Marking the helper NoInlining keeps the BMP path reliably lean.
private unsafe StringBuilder InsertRune(int index, Rune value)
src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs:2357
- This helper uses small fixed-size stackallocs; if it gets inlined, the public entrypoint could inherit stackalloc-related prolog costs. To keep the fast-path benefit reliable, consider marking this helper NoInlining.
private unsafe StringBuilder ReplaceRune(Rune oldRune, Rune newRune, int startIndex, int count)
| return AppendRune(value); | ||
| } | ||
|
|
||
| private unsafe StringBuilder AppendRune(Rune value) |
There was a problem hiding this comment.
I'm not sure whether to apply [MethodImpl(MethodImplOptions.NoInlining)].
Should I add it to make it explicit that these private methods should not be inlined?
There was a problem hiding this comment.
🔵 Needs a closer look
Mark the three extracted span helpers NoInlining to keep stackalloc work out of the BMP fast paths.
Review details
Suppressed comments (3)
src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs:1070
- This split does not by itself guarantee that the stackalloc stays out of the BMP fast path: the JIT can inline a small helper after promoting a fixed-size localloc, which would put the span stack setup back into
Append(Rune). The existing CoreLib span helpers useNoInliningfor this exact reason (for example,Encoding.Internal.cs:225-230); please mark this helper asNoInlining, and apply the same treatment to the other two Rune helpers below.
private unsafe StringBuilder AppendRune(Rune value)
src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs:1397
- The same inlining risk applies here: without
NoInlining, the JIT may inline this fixed-size-stackalloc helper intoInsert(int, Rune), defeating the stated goal of keeping stack setup out of the fast path. Please add the attribute here as well, matching the existing span-helper pattern inEncoding.Internal.cs.
private unsafe StringBuilder InsertRune(int index, Rune value)
src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs:2357
- Without
NoInlining, this small helper can be inlined and its two fixed-size stackallocs can become part of the caller's prolog, so extracting it does not reliably keep the slow path out ofReplace(Rune, Rune, ...). Please mark this helperNoInliningtoo, consistent with the existing CoreLib span-helper pattern.
private unsafe StringBuilder ReplaceRune(Rune oldRune, Rune newRune, int startIndex, int count)
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
Add a fast path that calls
StringBuilder.Append/Insert/Replace(char)whenRune.IsBmpholds.Move the existing span path into a private method so that the stackalloc stays out of the fast path.
This follows the pattern used in #127939.
Benchmark
Benchmark source