Use pooled objects in more locations #10598

ryzngard · 2024-07-09T20:27:53Z

Cleaning up some places that are newing up Lists intead of using the object pool. I also moved a few things to ImmutableArray instead of List since they weren't mutated after initial collection

ryzngard · 2024-07-09T20:29:04Z

...c/Microsoft.AspNetCore.Razor.LanguageServer/CodeActions/AddUsingsCodeActionProviderHelper.cs

@@ -38,7 +39,7 @@ public static async Task<TextEdit[]> GetUsingStatementEditsAsync(RazorCodeDocume
        var oldUsings = await FindUsingDirectiveStringsAsync(originalCSharpText, cancellationToken).ConfigureAwait(false);
        var newUsings = await FindUsingDirectiveStringsAsync(changedCSharpText, cancellationToken).ConfigureAwait(false);

-        var edits = new List<TextEdit>();
+        using var _ = ListPool<TextEdit>.GetPooledObject(out var edits);


I'm unsure which would be better here, ListPool or ArrayBuilderPool

ListPool<T> is fine, but if you just want an array in the end, consider using PooledArrayBuilder<T> like so:

using var edits = new PooledArrayBuilder<TextEdit>();

For small arrays (4 or fewer elements), it uses stack space. For larger arrays, it grabs a builder from ArrayBuilderPool<T>.

To be read in the least authoratative voice you can imagine, but I've always been of the opinion that if something is never realized as an immutable collection, then using an immutable collection builder is a bit silly, and List is fine. I think they work the same way (resizing internal arrays), but my hope is that List, being the more common type, might get some tiny jit advantage.

EDIT: Ignore me, Dustin knows best!

To be fair, you're completley right. ListPool<T> is fine when you're just going to call ToArray(). However, if it's likely to be a small array, PooledArrayBuilder<T> we'll be better because it won't even hit the pool.

I'm much happier when there is one specific guidance we can give. So "just use pooled array builder, and call builder.toimmutable" is an easy rule to remember.

What I've arrived at for this change

Default to PooledArrayBuilder<T> for places that are just building them and then ToArray/ToImmutable

Use ArrayBuilderPool<T> when there's lots of functions passing around the list but one cental start/end of the chain to manage lifetime and are synchronous (use AsRef)

Use ListPool<T> for the async version of above

Use ArrayBuilderPool when there's lots of functions passing around the list but one cental start/end of the chain to manage lifetime and are synchronous (use AsRef)

Use ListPool for the async version of above

FWIW, ArrayBuilderPool<T> can be used for both of those situations. ImmutableArray<T>.Builder is a reference type and does not need to be passed by ref. Of course, PooledArrayBuilder<T> can't be passed to an async method.

In the last and most nit-picky way, I just find it more annoying to write ImmutableArray<T>.Builder as the type. If there's a perf benefit I'm more than happy to amend my thinking, otherwise I'm lazy and like typing less

Or better put, I should have merged the first two dots

PooledArrayBuilder<T> for all cases, and if passed sync pass by ref.

ListPool<T> for passing to async methods

I just find it more annoying to write ImmutableArray<T>.Builder as the type

For sure. I totally agree with you on that. 😄

DustinCampbell

Sorry to "request changes", but there are a few spots that'll cause problems if this goes in. In general, I have a few pieces of feedback that apply throughout:

Prefer PooledArrayBuilder<T> when the goal is to create an ImmutableArray<T> or a T[]. This data structure is generally more efficient because it stores the first four elements on the stack and only acquires an ImmutableArray<T>.Builder from ArrayBuilderPool<T> if there are more than four elements. For a completely temporary array (i.e. collecting a few items and just iterating them), PooledArrayBuilder<T> is especially useful. Also, PooledArrayBuider<T> is better ergonomically, since it doesn't require an out var.
Call ImmutableArray<T>.Builder.ToImmutable() rather than ToImmutableArray(). ToImmutable() is the instance method provided on all immutable collection builder types to produce the final collection. ToImmutableArray() is an extension method provided for converting other data structures to an ImmutableArray<T>. Ultimately, ToImmutableArray() will work, but it ends up making an unnecessary extra null check.
Don't spread builders into collection expressions. The compiler does not generate optimal code for this scenario. Instead, call ToImmutable() or ToArray().
Be sure to set the capacity when known. I spotted one place where the capacity was no longer set. For many structures, we have SetCapacityIflarger(...) extension methods. PooledArrayBuilder<T> can take a capacity in its constructor.
Do not allow pooled objects to escape from methods! This is very dangerous, and I marked a couple of spots where this was happening. When this happens to a pooled object retrieved in a using, the object will still be used outside the method after it has been returned to the pool.

DustinCampbell · 2024-07-09T21:03:55Z

...c/Microsoft.AspNetCore.Razor.LanguageServer/CodeActions/AddUsingsCodeActionProviderHelper.cs

@@ -38,7 +39,7 @@ public static async Task<TextEdit[]> GetUsingStatementEditsAsync(RazorCodeDocume
        var oldUsings = await FindUsingDirectiveStringsAsync(originalCSharpText, cancellationToken).ConfigureAwait(false);
        var newUsings = await FindUsingDirectiveStringsAsync(changedCSharpText, cancellationToken).ConfigureAwait(false);

-        var edits = new List<TextEdit>();
+        using var _ = ListPool<TextEdit>.GetPooledObject(out var edits);


ListPool<T> is fine, but if you just want an array in the end, consider using PooledArrayBuilder<T> like so:

using var edits = new PooledArrayBuilder<TextEdit>();

For small arrays (4 or fewer elements), it uses stack space. For larger arrays, it grabs a builder from ArrayBuilderPool<T>.

...rosoft.AspNetCore.Razor.LanguageServer/CodeActions/CSharp/DefaultCSharpCodeActionProvider.cs

...ft.AspNetCore.Razor.LanguageServer/CodeActions/CSharp/TypeAccessibilityCodeActionProvider.cs

...re.Razor.LanguageServer/DocumentPresentation/AbstractTextDocumentPresentationEndpointBase.cs

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormatter.cs

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/MapCode/MapCodeEndpoint.cs

src/Razor/src/Microsoft.VisualStudio.LegacyEditor.Razor/Parsing/VisualStudioRazorParser.cs

davidwengier

Thoughts:

Legacy editor change is scary
~~I never know whether to call .ToImmutableArray() or .DrainToImmutable()~~ I think Dustin answered this above
~~Our use of ListPool vs ArrayBuilderPool is inconsistent but I don't know if thats an issue or not~~ I think Dustin answered this above

Overall a very good change and cleanup though, thank you

.../src/Microsoft.AspNetCore.Razor.LanguageServer/Completion/AggregateCompletionItemResolver.cs

src/Razor/src/Microsoft.VisualStudio.LegacyEditor.Razor/Parsing/VisualStudioRazorParser.cs

...Razor/src/Microsoft.VisualStudio.RazorExtension/SyntaxVisualizer/IntraTextAdornmentTagger.cs

ryzngard · 2024-07-10T01:28:36Z

Converting to draft until I've responded/fixed feedback. Going to do it incrementally

DustinCampbell

This is looking much better! Happy to approve this change. However, be mindful of the loop reversal in VisualStudioRazorParser. Please don't merge without addressing that.

...rosoft.AspNetCore.Razor.LanguageServer/CodeActions/CSharp/DefaultCSharpCodeActionProvider.cs

...ft.AspNetCore.Razor.LanguageServer/CodeActions/CSharp/TypeAccessibilityCodeActionProvider.cs

...Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/CodeActions/CodeActionResolveEndpoint.cs

...spNetCore.Razor.LanguageServer/CodeActions/Razor/ComponentAccessibilityCodeActionProvider.cs

...re.Razor.LanguageServer/DocumentPresentation/AbstractTextDocumentPresentationEndpointBase.cs

DustinCampbell · 2024-07-10T16:32:56Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/MapCode/MapCodeEndpoint.cs

@@ -77,7 +77,7 @@ public void ApplyCapabilities(VSInternalServerCapabilities serverCapabilities, V

    private async Task<WorkspaceEdit?> HandleMappingsAsync(VSInternalMapCodeMapping[] mappings, Guid mapCodeCorrelationId, CancellationToken cancellationToken)
    {
-        using var _ = ArrayBuilderPool<TextDocumentEdit>.GetPooledObject(out var changes);
+        using var _ = ListPool<TextDocumentEdit>.GetPooledObject(out var changes);


List<> is good here. You could also use a PooledArrayBuilder<T> and pass it by ref as I described above.

This is used in TryMapCodeAsync so I think it has to be a non-ref argument

Yes -- good point.

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/MapCode/MapCodeEndpoint.cs

DustinCampbell · 2024-07-10T16:44:55Z

...c/Microsoft.AspNetCore.Razor.LanguageServer/CodeActions/AddUsingsCodeActionProviderHelper.cs

@@ -38,7 +39,7 @@ public static async Task<TextEdit[]> GetUsingStatementEditsAsync(RazorCodeDocume
        var oldUsings = await FindUsingDirectiveStringsAsync(originalCSharpText, cancellationToken).ConfigureAwait(false);
        var newUsings = await FindUsingDirectiveStringsAsync(changedCSharpText, cancellationToken).ConfigureAwait(false);

-        var edits = new List<TextEdit>();
+        using var _ = ListPool<TextEdit>.GetPooledObject(out var edits);


Use ArrayBuilderPool when there's lots of functions passing around the list but one cental start/end of the chain to manage lifetime and are synchronous (use AsRef)

Use ListPool for the async version of above

FWIW, ArrayBuilderPool<T> can be used for both of those situations. ImmutableArray<T>.Builder is a reference type and does not need to be passed by ref. Of course, PooledArrayBuilder<T> can't be passed to an async method.

ryzngard · 2024-07-10T19:52:52Z

...ft.AspNetCore.Razor.LanguageServer/CodeActions/CSharp/TypeAccessibilityCodeActionProvider.cs

    {
-        var typeAccessibilityCodeActions = new List<RazorVSInternalCodeAction>(1);


@DustinCampbell this had a list set to 1 and I'm not really sure why... I think I wouldn't need to do that here because that would just keep it on the stack?

Super weird! I would just ignore the capacity in that case.

ryzngard · 2024-07-11T00:44:59Z

Ran an integration test just to be sure: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=9862176&view=results

ryzngard added 2 commits July 8, 2024 18:46

Convert a lot of usages to pooled objects

89db3c8

A few more locations

b34f0ec

ryzngard requested review from a team as code owners July 9, 2024 20:27

ryzngard commented Jul 9, 2024

View reviewed changes

Merge branch 'main' into pooled_arrays

dce8467

DustinCampbell requested changes Jul 9, 2024

View reviewed changes

davidwengier approved these changes Jul 9, 2024

View reviewed changes

ryzngard marked this pull request as draft July 10, 2024 01:28

PR feedback and cleanup

def15fb

DustinCampbell approved these changes Jul 10, 2024

View reviewed changes

PR feedback part 2

6f2626f

ryzngard marked this pull request as ready for review July 10, 2024 19:43

ryzngard commented Jul 10, 2024

View reviewed changes

ryzngard added 2 commits July 10, 2024 13:04

Missed a few places

55a8a69

Fix one case where immutable should have been mutable

0a24790

ryzngard merged commit 7b511df into dotnet:main Jul 11, 2024
17 checks passed

dotnet-policy-service bot added this to the Next milestone Jul 11, 2024

ryzngard deleted the pooled_arrays branch July 11, 2024 00:45

DustinCampbell mentioned this pull request Jul 11, 2024

Add ImmutableArray extension methods for ordering #10608

Merged

This was referenced Jul 16, 2024

[Automated] PRs inserted in VS build main-35115.158 #10624

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35118.200 #10649

Closed

DustinCampbell mentioned this pull request Jul 19, 2024

Added basic extract to component functionality on cursor over html tag #10578

Merged

RikkiGibson modified the milestones: Next, 17.12 Preview 1 Jul 29, 2024

dotnet-bot mentioned this pull request Aug 2, 2024

[Automated] PRs inserted in VS build feature.debugger.shadowDebug-35201.55 #10703

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use pooled objects in more locations #10598

Use pooled objects in more locations #10598

ryzngard commented Jul 9, 2024

ryzngard Jul 9, 2024

DustinCampbell Jul 9, 2024

davidwengier Jul 9, 2024 •

edited

Loading

DustinCampbell Jul 9, 2024

davidwengier Jul 9, 2024

ryzngard Jul 10, 2024

DustinCampbell Jul 10, 2024

ryzngard Jul 10, 2024

ryzngard Jul 10, 2024

DustinCampbell Jul 10, 2024

DustinCampbell left a comment

DustinCampbell Jul 9, 2024

davidwengier left a comment •

edited

Loading

ryzngard commented Jul 10, 2024

DustinCampbell left a comment

DustinCampbell Jul 10, 2024

ryzngard Jul 10, 2024

DustinCampbell Jul 10, 2024

DustinCampbell Jul 10, 2024

ryzngard Jul 10, 2024

DustinCampbell Jul 10, 2024

ryzngard commented Jul 11, 2024

		{
		var typeAccessibilityCodeActions = new List<RazorVSInternalCodeAction>(1);

Use pooled objects in more locations #10598

Use pooled objects in more locations #10598

Conversation

ryzngard commented Jul 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidwengier Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidwengier left a comment • edited Loading

Choose a reason for hiding this comment

ryzngard commented Jul 10, 2024

DustinCampbell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzngard commented Jul 11, 2024

davidwengier Jul 9, 2024 •

edited

Loading

davidwengier left a comment •

edited

Loading