-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move to collection exprs #73009
Move to collection exprs #73009
Conversation
@ToddGrun ptal |
@@ -17,7 +17,7 @@ internal static void UpdateText(SourceText newText, ITextBuffer buffer, EditOpti | |||
var oldSnapshot = buffer.CurrentSnapshot; | |||
var oldText = oldSnapshot.AsText(); | |||
var changes = newText.GetTextChanges(oldText); | |||
UpdateText(changes.ToImmutableArray(), buffer, oldSnapshot, oldText, options); | |||
UpdateText([.. changes], buffer, oldSnapshot, oldText, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it rewrites it to:
int num = 0;
TextChange[] array = new TextChange[readOnlyList.Count];
foreach (TextChange item in readOnlyList)
{
array[num] = item;
num++;
}
UpdateText(ImmutableCollectionsMarshal.AsImmutableArray(array), buffer, oldSnapshot, oldText, options);
which is basically ideal here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't the old code not allocate since changes is actually an ImmutableArray?
public static ImmutableArray<TSource> ToImmutableArray<TSource>(this IEnumerable<TSource> items)
{
if (items is ImmutableArray<TSource>)
{
return (ImmutableArray<TSource>)items;
}
return CreateRange(items);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see. This benefitted from a runtime check. That's a tougher call. Will have to think about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talked with Cyrus about this, and while there is a new allocation here, he's ran perf tests and no regressions appear at that level. Additionally, he's indicated that this particular regression is something that is being investigated to see if it can be addressed at the collection expression level. Not going to block anymore based on this.
src/EditorFeatures/Core/IntelliSense/AsyncCompletion/CompletionSource.cs
Show resolved
Hide resolved
@@ -45,7 +45,7 @@ public override ITypeSymbol VisitNamedType(INamedTypeSymbol symbol) | |||
return symbol; | |||
} | |||
|
|||
return symbol.ConstructedFrom.Construct(arguments.ToArray()); | |||
return symbol.ConstructedFrom.Construct([.. arguments]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -47,7 +47,7 @@ public override ITypeSymbol VisitNamedType(INamedTypeSymbol symbol) | |||
if (arguments.SequenceEqual(symbol.TypeArguments)) | |||
return symbol; | |||
|
|||
return symbol.ConstructedFrom.Construct(arguments.ToArray()); | |||
return symbol.ConstructedFrom.Construct([.. arguments]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -49,7 +49,7 @@ public override ITypeSymbol VisitNamedType(INamedTypeSymbol symbol) | |||
return symbol; | |||
} | |||
|
|||
return symbol.ConstructedFrom.Construct(arguments.ToArray()); | |||
return symbol.ConstructedFrom.Construct([.. arguments]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -151,7 +151,7 @@ public async Task WaitAllAsync(Workspace? workspace, string[]? featureNames = nu | |||
do | |||
{ | |||
// wait for all current tasks to be done for the time given | |||
if (Task.WaitAll(tasks.ToArray(), smallTimeout)) | |||
if (Task.WaitAll([.. tasks], smallTimeout)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do in follow ups! |
PR1: https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_build/results?buildId=9445152&view=results
PR2: https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_build/results?buildId=9445153&view=results