Skip to content

Commit d7d1d30

Browse files
andyleejordanandyleejordan
committed
Reduce unnecessary allocations and exceptions in handlers
By consistently using a static empty container, and breaking instead of throwing on cancellation.
1 parent f628106 commit d7d1d30

File tree

8 files changed

+118
-98
lines changed

8 files changed

+118
-98
lines changed

‎src/PowerShellEditorServices/Services/TextDocument/Handlers/CodeActionHandler.cs

+14-6
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ namespace Microsoft.PowerShell.EditorServices.Handlers
1919
{
2020
internal class PsesCodeActionHandler : CodeActionHandlerBase
2121
{
22+
private static readonly CommandOrCodeActionContainer s_emptyCommandOrCodeActionContainer = new();
2223
private readonly ILogger _logger;
2324
private readonly AnalysisService _analysisService;
2425

@@ -42,23 +43,29 @@ public override async Task<CommandOrCodeActionContainer> Handle(CodeActionParams
4243
if (cancellationToken.IsCancellationRequested)
4344
{
4445
_logger.LogDebug($"CodeAction request canceled at range: {request.Range}");
45-
return Array.Empty<CommandOrCodeAction>();
46+
return s_emptyCommandOrCodeActionContainer;
4647
}
4748

4849
IReadOnlyDictionary<string, IEnumerable<MarkerCorrection>> corrections = await _analysisService.GetMostRecentCodeActionsForFileAsync(
4950
request.TextDocument.Uri)
5051
.ConfigureAwait(false);
5152

52-
if (corrections == null)
53+
// GetMostRecentCodeActionsForFileAsync actually returns null if there's no corrections.
54+
if (corrections is null)
5355
{
54-
return Array.Empty<CommandOrCodeAction>();
56+
return s_emptyCommandOrCodeActionContainer;
5557
}
5658

5759
List<CommandOrCodeAction> codeActions = new();
5860

5961
// If there are any code fixes, send these commands first so they appear at top of "Code Fix" menu in the client UI.
6062
foreach (Diagnostic diagnostic in request.Context.Diagnostics)
6163
{
64+
if (cancellationToken.IsCancellationRequested)
65+
{
66+
break;
67+
}
68+
6269
if (string.IsNullOrEmpty(diagnostic.Code?.String))
6370
{
6471
_logger.LogWarning(
@@ -100,8 +107,7 @@ public override async Task<CommandOrCodeActionContainer> Handle(CodeActionParams
100107
HashSet<string> ruleNamesProcessed = new();
101108
foreach (Diagnostic diagnostic in request.Context.Diagnostics)
102109
{
103-
if (
104-
!diagnostic.Code.HasValue ||
110+
if (!diagnostic.Code.HasValue ||
105111
!diagnostic.Code.Value.IsString ||
106112
string.IsNullOrEmpty(diagnostic.Code?.String))
107113
{
@@ -134,7 +140,9 @@ public override async Task<CommandOrCodeActionContainer> Handle(CodeActionParams
134140
}
135141
}
136142

137-
return codeActions;
143+
return codeActions.Count == 0
144+
? s_emptyCommandOrCodeActionContainer
145+
: codeActions;
138146
}
139147
}
140148
}

‎src/PowerShellEditorServices/Services/TextDocument/Handlers/DefinitionHandler.cs

+12-10
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ namespace Microsoft.PowerShell.EditorServices.Handlers
1717
{
1818
internal class PsesDefinitionHandler : DefinitionHandlerBase
1919
{
20+
private static readonly LocationOrLocationLinks s_emptyLocationOrLocationLinks = new();
2021
private readonly SymbolsService _symbolsService;
2122
private readonly WorkspaceService _workspaceService;
2223

@@ -45,20 +46,19 @@ public override async Task<LocationOrLocationLinks> Handle(DefinitionParams requ
4546

4647
if (foundSymbol is null)
4748
{
48-
return new LocationOrLocationLinks();
49+
return s_emptyLocationOrLocationLinks;
4950
}
5051

5152
// Short-circuit if we're already on the definition.
5253
if (foundSymbol.IsDeclaration)
5354
{
54-
return new LocationOrLocationLinks(
55-
new LocationOrLocationLink[] {
56-
new LocationOrLocationLink(
57-
new Location
58-
{
59-
Uri = DocumentUri.From(foundSymbol.FilePath),
60-
Range = foundSymbol.NameRegion.ToRange()
61-
})});
55+
return new LocationOrLocationLink[] {
56+
new LocationOrLocationLink(
57+
new Location
58+
{
59+
Uri = DocumentUri.From(foundSymbol.FilePath),
60+
Range = foundSymbol.NameRegion.ToRange()
61+
})};
6262
}
6363

6464
List<LocationOrLocationLink> definitionLocations = new();
@@ -74,7 +74,9 @@ public override async Task<LocationOrLocationLinks> Handle(DefinitionParams requ
7474
}));
7575
}
7676

77-
return new LocationOrLocationLinks(definitionLocations);
77+
return definitionLocations.Count == 0
78+
? s_emptyLocationOrLocationLinks
79+
: definitionLocations;
7880
}
7981
}
8082
}

‎src/PowerShellEditorServices/Services/TextDocument/Handlers/DocumentHighlightHandler.cs

+5-2
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// Licensed under the MIT License.
33

44
using System.Collections.Generic;
5+
using System.Linq;
56
using System.Threading;
67
using System.Threading.Tasks;
78
using Microsoft.Extensions.Logging;
@@ -45,7 +46,7 @@ public override Task<DocumentHighlightContainer> Handle(
4546
request.Position.Line + 1,
4647
request.Position.Character + 1);
4748

48-
if (occurrences is null)
49+
if (!occurrences.Any())
4950
{
5051
return Task.FromResult(s_emptyHighlightContainer);
5152
}
@@ -62,7 +63,9 @@ public override Task<DocumentHighlightContainer> Handle(
6263

6364
_logger.LogDebug("Highlights: " + highlights);
6465

65-
return Task.FromResult(new DocumentHighlightContainer(highlights));
66+
return highlights.Count == 0
67+
? Task.FromResult(s_emptyHighlightContainer)
68+
: Task.FromResult(new DocumentHighlightContainer(highlights));
6669
}
6770
}
6871
}

‎src/PowerShellEditorServices/Services/TextDocument/Handlers/DocumentSymbolHandler.cs

+17-53
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,11 @@
1-
// Copyright (c) Microsoft Corporation.
1+
// Copyright (c) Microsoft Corporation.
22
// Licensed under the MIT License.
33

4-
using System;
54
using System.Collections.Generic;
6-
using System.Diagnostics;
75
using System.IO;
8-
using System.Linq;
96
using System.Threading;
107
using System.Threading.Tasks;
118
using Microsoft.Extensions.Logging;
12-
using Microsoft.PowerShell.EditorServices.Logging;
139
using Microsoft.PowerShell.EditorServices.Services;
1410
using Microsoft.PowerShell.EditorServices.Services.Symbols;
1511
using Microsoft.PowerShell.EditorServices.Services.TextDocument;
@@ -23,6 +19,7 @@ namespace Microsoft.PowerShell.EditorServices.Handlers
2319
{
2420
internal class PsesDocumentSymbolHandler : DocumentSymbolHandlerBase
2521
{
22+
private static readonly SymbolInformationOrDocumentSymbolContainer s_emptySymbolInformationOrDocumentSymbolContainer = new();
2623
private readonly ILogger _logger;
2724
private readonly WorkspaceService _workspaceService;
2825
private readonly IDocumentSymbolProvider[] _providers;
@@ -48,23 +45,21 @@ public PsesDocumentSymbolHandler(ILoggerFactory factory, WorkspaceService worksp
4845
// AKA the outline feature
4946
public override async Task<SymbolInformationOrDocumentSymbolContainer> Handle(DocumentSymbolParams request, CancellationToken cancellationToken)
5047
{
51-
ScriptFile scriptFile = _workspaceService.GetFile(request.TextDocument.Uri);
52-
53-
IEnumerable<SymbolReference> foundSymbols = ProvideDocumentSymbols(scriptFile);
54-
if (foundSymbols is null)
55-
{
56-
return null;
57-
}
48+
_logger.LogDebug($"Handling document symbols for {request.TextDocument.Uri}");
5849

50+
ScriptFile scriptFile = _workspaceService.GetFile(request.TextDocument.Uri);
5951
string containerName = Path.GetFileNameWithoutExtension(scriptFile.FilePath);
60-
6152
List<SymbolInformationOrDocumentSymbol> symbols = new();
62-
foreach (SymbolReference r in foundSymbols)
53+
54+
foreach (SymbolReference r in ProvideDocumentSymbols(scriptFile))
6355
{
6456
// This async method is pretty dense with synchronous code
6557
// so it's helpful to add some yields.
6658
await Task.Yield();
67-
cancellationToken.ThrowIfCancellationRequested();
59+
if (cancellationToken.IsCancellationRequested)
60+
{
61+
break;
62+
}
6863

6964
// Outline view should only include declarations.
7065
//
@@ -93,58 +88,27 @@ public override async Task<SymbolInformationOrDocumentSymbolContainer> Handle(Do
9388
Location = new Location
9489
{
9590
Uri = DocumentUri.From(r.FilePath),
96-
Range = new Range(r.NameRegion.ToRange().Start, r.ScriptRegion.ToRange().End) // Jump to name start, but keep whole range to support symbol tree in outline
91+
// Jump to name start, but keep whole range to support symbol tree in outline
92+
Range = new Range(r.NameRegion.ToRange().Start, r.ScriptRegion.ToRange().End)
9793
},
9894
Name = r.Name
9995
}));
10096
}
10197

102-
return new SymbolInformationOrDocumentSymbolContainer(symbols);
98+
return symbols.Count == 0
99+
? s_emptySymbolInformationOrDocumentSymbolContainer
100+
: new SymbolInformationOrDocumentSymbolContainer(symbols);
103101
}
104102

105103
private IEnumerable<SymbolReference> ProvideDocumentSymbols(ScriptFile scriptFile)
106104
{
107-
return InvokeProviders(p => p.ProvideDocumentSymbols(scriptFile))
108-
.SelectMany(r => r);
109-
}
110-
111-
/// <summary>
112-
/// Invokes the given function synchronously against all
113-
/// registered providers.
114-
/// </summary>
115-
/// <param name="invokeFunc">The function to be invoked.</param>
116-
/// <returns>
117-
/// An IEnumerable containing the results of all providers
118-
/// that were invoked successfully.
119-
/// </returns>
120-
protected IEnumerable<TResult> InvokeProviders<TResult>(
121-
Func<IDocumentSymbolProvider, TResult> invokeFunc)
122-
{
123-
Stopwatch invokeTimer = new();
124-
List<TResult> providerResults = new();
125-
126105
foreach (IDocumentSymbolProvider provider in _providers)
127106
{
128-
try
107+
foreach (SymbolReference symbol in provider.ProvideDocumentSymbols(scriptFile))
129108
{
130-
invokeTimer.Restart();
131-
132-
providerResults.Add(invokeFunc(provider));
133-
134-
invokeTimer.Stop();
135-
136-
_logger.LogTrace(
137-
$"Invocation of provider '{provider.GetType().Name}' completed in {invokeTimer.ElapsedMilliseconds}ms.");
138-
}
139-
catch (Exception e)
140-
{
141-
_logger.LogException(
142-
$"Exception caught while invoking provider {provider.GetType().Name}:",
143-
e);
109+
yield return symbol;
144110
}
145111
}
146-
147-
return providerResults;
148112
}
149113
}
150114
}

‎src/PowerShellEditorServices/Services/TextDocument/Handlers/FoldingRangeHandler.cs

+21-9
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ namespace Microsoft.PowerShell.EditorServices.Handlers
1616
{
1717
internal class PsesFoldingRangeHandler : FoldingRangeHandlerBase
1818
{
19+
private static readonly Container<FoldingRange> s_emptyFoldingRangeContainer = new();
1920
private readonly ILogger _logger;
2021
private readonly ConfigurationService _configurationService;
2122
private readonly WorkspaceService _workspaceService;
@@ -37,27 +38,36 @@ public override Task<Container<FoldingRange>> Handle(FoldingRangeRequestParam re
3738
if (cancellationToken.IsCancellationRequested)
3839
{
3940
_logger.LogDebug("FoldingRange request canceled for file: {Uri}", request.TextDocument.Uri);
40-
return Task.FromResult(new Container<FoldingRange>());
41+
return Task.FromResult(s_emptyFoldingRangeContainer);
4142
}
4243

43-
// TODO Should be using dynamic registrations
44-
if (!_configurationService.CurrentSettings.CodeFolding.Enable) { return Task.FromResult(new Container<FoldingRange>()); }
44+
// TODO: Should be using dynamic registrations
45+
if (!_configurationService.CurrentSettings.CodeFolding.Enable)
46+
{
47+
return Task.FromResult(s_emptyFoldingRangeContainer);
48+
}
4549

4650
// Avoid crash when using untitled: scheme or any other scheme where the document doesn't
4751
// have a backing file. https://github.com/PowerShell/vscode-powershell/issues/1676
4852
// Perhaps a better option would be to parse the contents of the document as a string
4953
// as opposed to reading a file but the scenario of "no backing file" probably doesn't
5054
// warrant the extra effort.
51-
if (!_workspaceService.TryGetFile(request.TextDocument.Uri, out ScriptFile scriptFile)) { return Task.FromResult(new Container<FoldingRange>()); }
52-
53-
List<FoldingRange> result = new();
55+
if (!_workspaceService.TryGetFile(request.TextDocument.Uri, out ScriptFile scriptFile))
56+
{
57+
return Task.FromResult(s_emptyFoldingRangeContainer);
58+
}
5459

5560
// If we're showing the last line, decrement the Endline of all regions by one.
5661
int endLineOffset = _configurationService.CurrentSettings.CodeFolding.ShowLastLine ? -1 : 0;
57-
62+
List<FoldingRange> folds = new();
5863
foreach (FoldingReference fold in TokenOperations.FoldableReferences(scriptFile.ScriptTokens).References)
5964
{
60-
result.Add(new FoldingRange
65+
if (cancellationToken.IsCancellationRequested)
66+
{
67+
break;
68+
}
69+
70+
folds.Add(new FoldingRange
6171
{
6272
EndCharacter = fold.EndCharacter,
6373
EndLine = fold.EndLine + endLineOffset,
@@ -67,7 +77,9 @@ public override Task<Container<FoldingRange>> Handle(FoldingRangeRequestParam re
6777
});
6878
}
6979

70-
return Task.FromResult(new Container<FoldingRange>(result));
80+
return folds.Count == 0
81+
? Task.FromResult(s_emptyFoldingRangeContainer)
82+
: Task.FromResult(new Container<FoldingRange>(folds));
7183
}
7284
}
7385
}

0 commit comments

Comments
 (0)