Followup PR to https://github.com/dotnet/roslyn/pull/69677 to reduce allocations further. #69746
Conversation
This prevents a string.Split call (which is called from quite a few places) and replaces it with ReadOnlyMemory operations instead.
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| { | ||
| var result = ArrayBuilder<ReadOnlyMemory<char>>.GetInstance(); | ||
|
|
||
| var index = s.Span.IndexOf(separator); |
There was a problem hiding this comment.
Is this the same semantics as before? What if the file only has \n newlines? Before I believe it would still split ok. But with this new logic, I'm not sure.
There was a problem hiding this comment.
Didn't the string.Split call take in a 1-element array, indicating "\r\n" is the delimiter? Was it intending to send in an array with '\r' and '\n' as delimiters instead?
There was a problem hiding this comment.
I'm not sure (and I'm on my phone). If the are the same semantics, great. It's just something I was nervous about. Thanks!
There was a problem hiding this comment.
It sure looks like the same semantics to me, but maybe I'm missing something as it does seem a little odd. (the Split call didn't change in your PR either, so if it's the same and it's wrong, it's been as such for a while)
| } | ||
|
|
||
| formattedLines.Add(sb.ToString()); | ||
| } | ||
|
|
||
| public static ImmutableArray<ReadOnlyMemory<char>> Split( |
There was a problem hiding this comment.
Consider making an extension. Consider returning an IEnumerable
There was a problem hiding this comment.
Note: on net-8 something, this method exists right? So maybe use that, and polyfill it in if on net standard?
There was a problem hiding this comment.
I intentionally made it return a type with a struct enumerator. Let me know if that's generally not preferred in this scenario.
As for net-8, there are Split extensions, but I don't think I'm fully understanding those. It looks like they require a Span argument in which to store the results, but I'm not sure how large to make that.
There was a problem hiding this comment.
As for making this an extension method, I thought about that too, but it looks like the other extension methods on ReadOnly(Memory/Span) are down in the compiler, which the features assembly doesn't have IVT to.
There was a problem hiding this comment.
We don't need ivt. A common thing we do is just link the same files into the ide layer.
There was a problem hiding this comment.
Let me know if that's generally not preferred in this scenario.
It's basically a tradeoff. One form allocatesa linear sequence up front, but has free enumeration.
The other form allocates for enumeration, but doesn't need the contiguous initial allocation.
I'm personally fine with either.
There was a problem hiding this comment.
As for net-8, there are Split extensions, but I don't think I'm fully understanding those. It looks like they require a Span argument in which to store the results, but I'm not sure how large to make that.
Ah. My mistake then. Ignore that idea then :-)
I thought there was some sort of SplitLines helper that streamed results.
| } | ||
|
|
||
| formattedLines.Add(sb.ToString()); | ||
| } | ||
|
|
||
| public static ImmutableArray<ReadOnlyMemory<char>> Split( | ||
| ReadOnlyMemory<char> s, |
There was a problem hiding this comment.
Not: use single character names for lambdas. Give a real name for full method.
|
Only nits. Feel free to ignore. |
ghost
added this to the 
This prevents a string.Split call (which is called from quite a few places) and replaces it with ReadOnlyMemory operations instead.