Clean up CollectionExpression and Select combined usage#12919
Conversation
There were several locations that had an immutable array, called select on it (getting back an IEnumerable) and then did a collection expression on that to get back an array. Instead, add a SelectAsPlainArray extension method on ImmutableArray that does this more efficiently. Old way allocates iterator from IEnumerable, intermediate arrays while calculating select, and then allocated final array New way: only allocates final array I noticed this from a code review I was doing, and not from perf traces, so I don't have evidence that this will make a tangible difference, but every bit helps.
| } | ||
|
|
||
| mappedChanges[razorDocumentUri.AbsoluteUri] = mappedEdits; | ||
| mappedChanges[razorDocumentUri.AbsoluteUri] = ImmutableCollectionsMarshal.AsArray(mappedEdits)!; |
There was a problem hiding this comment.
I trust that you know what you're doing, but my initial reaction to this is worry. Isn't this dangerous because the internal array came from a pool, and therefore could in theory be reused for something else before whatever is reading mappedChanges see it?
There was a problem hiding this comment.
This would only be dangerous if mappedEdits (or it's underlying array) were released back to a pool before we used it. Or maybe I'm misunderstanding the concern?
There was a problem hiding this comment.
More likely I'm misunderstanding, I just worry seeing "unsafe" things in product code, not in vetted helper methods. You never know who is going to copy them in potentially bad ways.
| var edits = new SumType<TextEdit, AnnotatedTextEdit>[textEdits.Length]; | ||
| for (var i = 0; i < textEdits.Length; i++) | ||
| { | ||
| edits[i] = textEdits[i]; | ||
| } |
There was a problem hiding this comment.
Second time I've seen this pattern in the PR. Consider a CastAsPlainArray<T> extension?
There was a problem hiding this comment.
I gave it a quick go and it didn't seem to flow. I'm going to pass on this one unless you feel strongly, or have something in mind and coded up.
There was a problem hiding this comment.
Not at all, merge at will.
|
You know what would be a great addition to this? An analyzer in Razor.Diagnostics.Analyzers so I don't make these mistakes again. |
|
That would be my first analyzer, which would be kinda cool. I'll put this on my backlog, as it does sound interesting, but there are probably bigger fish to fry right now. In reply to: 4085507970 |
|
I've never written an analyzer from scratch either, but I have told copilot to do it, and it does a pretty good job :) |
3 participants
There were several locations that had an immutable array, called select on it (getting back an IEnumerable) and then did a collection expression on that to get back an array. Instead, add a SelectAsPlainArray extension method on ImmutableArray that does this more efficiently.
Old way allocates iterator from IEnumerable, intermediate arrays while calculating select, and then allocated final array New way: only allocates final array
I noticed this from a code review I was doing, and not from perf traces, so I don't have evidence that this will make a tangible difference, but every bit helps.