Skip to content

Commit

Permalink
Reduce unnecessary allocations and exceptions in handlers
Browse files Browse the repository at this point in the history 
By consistently using a static empty container,
and breaking instead of throwing on cancellation.
  • Loading branch information
@andyleejordan
andyleejordan committed Feb 24, 2023
1 parent a171871 commit 1fbd190
Show file tree
Hide file tree
Showing 8 changed files with 118 additions and 98 deletions.
20 changes: 14 additions & 6 deletions src/PowerShellEditorServices/Services/TextDocument/Handlers/CodeActionHandler.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ namespace Microsoft.PowerShell.EditorServices.Handlers
{
internal class PsesCodeActionHandler : CodeActionHandlerBase
{
private static readonly CommandOrCodeActionContainer s_emptyCommandOrCodeActionContainer = new();
private readonly ILogger _logger;
private readonly AnalysisService _analysisService;

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

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

if (corrections == null)
// GetMostRecentCodeActionsForFileAsync actually returns null if there's no corrections.
if (corrections is null)
{
return Array.Empty<CommandOrCodeAction>();
return s_emptyCommandOrCodeActionContainer;
}

List<CommandOrCodeAction> codeActions = new();

// If there are any code fixes, send these commands first so they appear at top of "Code Fix" menu in the client UI.
foreach (Diagnostic diagnostic in request.Context.Diagnostics)
{
if (cancellationToken.IsCancellationRequested)
{
break;
}

if (string.IsNullOrEmpty(diagnostic.Code?.String))
{
_logger.LogWarning(
Expand Down Expand Up @@ -100,8 +107,7 @@ public override async Task<CommandOrCodeActionContainer> Handle(CodeActionParams
HashSet<string> ruleNamesProcessed = new();
foreach (Diagnostic diagnostic in request.Context.Diagnostics)
{
if (
!diagnostic.Code.HasValue ||
if (!diagnostic.Code.HasValue ||
!diagnostic.Code.Value.IsString ||
string.IsNullOrEmpty(diagnostic.Code?.String))
{
Expand Down Expand Up @@ -134,7 +140,9 @@ public override async Task<CommandOrCodeActionContainer> Handle(CodeActionParams
}
}

return codeActions;
return codeActions.Count == 0
? s_emptyCommandOrCodeActionContainer
: codeActions;
}
}
}
22 changes: 12 additions & 10 deletions src/PowerShellEditorServices/Services/TextDocument/Handlers/DefinitionHandler.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ namespace Microsoft.PowerShell.EditorServices.Handlers
{
internal class PsesDefinitionHandler : DefinitionHandlerBase
{
private static readonly LocationOrLocationLinks s_emptyLocationOrLocationLinks = new();
private readonly SymbolsService _symbolsService;
private readonly WorkspaceService _workspaceService;

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

if (foundSymbol is null)
{
return new LocationOrLocationLinks();
return s_emptyLocationOrLocationLinks;
}

// Short-circuit if we're already on the definition.
if (foundSymbol.IsDeclaration)
{
return new LocationOrLocationLinks(
new LocationOrLocationLink[] {
new LocationOrLocationLink(
new Location
{
Uri = DocumentUri.From(foundSymbol.FilePath),
Range = foundSymbol.NameRegion.ToRange()
})});
return new LocationOrLocationLink[] {
new LocationOrLocationLink(
new Location
{
Uri = DocumentUri.From(foundSymbol.FilePath),
Range = foundSymbol.NameRegion.ToRange()
})};
}

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

return new LocationOrLocationLinks(definitionLocations);
return definitionLocations.Count == 0
? s_emptyLocationOrLocationLinks
: definitionLocations;
}
}
}
7 changes: 5 additions & 2 deletions src/PowerShellEditorServices/Services/TextDocument/Handlers/DocumentHighlightHandler.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT License.

using System.Collections.Generic;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Extensions.Logging;
Expand Down Expand Up @@ -45,7 +46,7 @@ public override Task<DocumentHighlightContainer> Handle(
request.Position.Line + 1,
request.Position.Character + 1);

if (occurrences is null)
if (!occurrences.Any())
{
return Task.FromResult(s_emptyHighlightContainer);
}
Expand All @@ -62,7 +63,9 @@ public override Task<DocumentHighlightContainer> Handle(

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

return Task.FromResult(new DocumentHighlightContainer(highlights));
return highlights.Count == 0
? Task.FromResult(s_emptyHighlightContainer)
: Task.FromResult(new DocumentHighlightContainer(highlights));
}
}
}
70 changes: 17 additions & 53 deletions src/PowerShellEditorServices/Services/TextDocument/Handlers/DocumentSymbolHandler.cs
View file
Original file line number Diff line number Diff line change
@@ -1,15 +1,11 @@
// Copyright (c) Microsoft Corporation.
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Extensions.Logging;
using Microsoft.PowerShell.EditorServices.Logging;
using Microsoft.PowerShell.EditorServices.Services;
using Microsoft.PowerShell.EditorServices.Services.Symbols;
using Microsoft.PowerShell.EditorServices.Services.TextDocument;
Expand All @@ -23,6 +19,7 @@ namespace Microsoft.PowerShell.EditorServices.Handlers
{
internal class PsesDocumentSymbolHandler : DocumentSymbolHandlerBase
{
private static readonly SymbolInformationOrDocumentSymbolContainer s_emptySymbolInformationOrDocumentSymbolContainer = new();
private readonly ILogger _logger;
private readonly WorkspaceService _workspaceService;
private readonly IDocumentSymbolProvider[] _providers;
Expand All @@ -48,23 +45,21 @@ public PsesDocumentSymbolHandler(ILoggerFactory factory, WorkspaceService worksp
// AKA the outline feature
public override async Task<SymbolInformationOrDocumentSymbolContainer> Handle(DocumentSymbolParams request, CancellationToken cancellationToken)
{
ScriptFile scriptFile = _workspaceService.GetFile(request.TextDocument.Uri);

IEnumerable<SymbolReference> foundSymbols = ProvideDocumentSymbols(scriptFile);
if (foundSymbols is null)
{
return null;
}
_logger.LogDebug($"Handling document symbols for {request.TextDocument.Uri}");

ScriptFile scriptFile = _workspaceService.GetFile(request.TextDocument.Uri);
string containerName = Path.GetFileNameWithoutExtension(scriptFile.FilePath);

List<SymbolInformationOrDocumentSymbol> symbols = new();
foreach (SymbolReference r in foundSymbols)

foreach (SymbolReference r in ProvideDocumentSymbols(scriptFile))
{
// This async method is pretty dense with synchronous code
// so it's helpful to add some yields.
await Task.Yield();
cancellationToken.ThrowIfCancellationRequested();
if (cancellationToken.IsCancellationRequested)
{
break;
}

// Outline view should only include declarations.
//
Expand Down Expand Up @@ -93,58 +88,27 @@ public override async Task<SymbolInformationOrDocumentSymbolContainer> Handle(Do
Location = new Location
{
Uri = DocumentUri.From(r.FilePath),
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
// Jump to name start, but keep whole range to support symbol tree in outline
Range = new Range(r.NameRegion.ToRange().Start, r.ScriptRegion.ToRange().End)
},
Name = r.Name
}));
}

return new SymbolInformationOrDocumentSymbolContainer(symbols);
return symbols.Count == 0
? s_emptySymbolInformationOrDocumentSymbolContainer
: new SymbolInformationOrDocumentSymbolContainer(symbols);
}

private IEnumerable<SymbolReference> ProvideDocumentSymbols(ScriptFile scriptFile)
{
return InvokeProviders(p => p.ProvideDocumentSymbols(scriptFile))
.SelectMany(r => r);
}

/// <summary>
/// Invokes the given function synchronously against all
/// registered providers.
/// </summary>
/// <param name="invokeFunc">The function to be invoked.</param>
/// <returns>
/// An IEnumerable containing the results of all providers
/// that were invoked successfully.
/// </returns>
protected IEnumerable<TResult> InvokeProviders<TResult>(
Func<IDocumentSymbolProvider, TResult> invokeFunc)
{
Stopwatch invokeTimer = new();
List<TResult> providerResults = new();

foreach (IDocumentSymbolProvider provider in _providers)
{
try
foreach (SymbolReference symbol in provider.ProvideDocumentSymbols(scriptFile))
{
invokeTimer.Restart();

providerResults.Add(invokeFunc(provider));

invokeTimer.Stop();

_logger.LogTrace(
$"Invocation of provider '{provider.GetType().Name}' completed in {invokeTimer.ElapsedMilliseconds}ms.");
}
catch (Exception e)
{
_logger.LogException(
$"Exception caught while invoking provider {provider.GetType().Name}:",
e);
yield return symbol;
}
}

return providerResults;
}
}
}
30 changes: 21 additions & 9 deletions src/PowerShellEditorServices/Services/TextDocument/Handlers/FoldingRangeHandler.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ namespace Microsoft.PowerShell.EditorServices.Handlers
{
internal class PsesFoldingRangeHandler : FoldingRangeHandlerBase
{
private static readonly Container<FoldingRange> s_emptyFoldingRangeContainer = new();
private readonly ILogger _logger;
private readonly ConfigurationService _configurationService;
private readonly WorkspaceService _workspaceService;
Expand All @@ -37,27 +38,36 @@ public override Task<Container<FoldingRange>> Handle(FoldingRangeRequestParam re
if (cancellationToken.IsCancellationRequested)
{
_logger.LogDebug("FoldingRange request canceled for file: {Uri}", request.TextDocument.Uri);
return Task.FromResult(new Container<FoldingRange>());
return Task.FromResult(s_emptyFoldingRangeContainer);
}

// TODO Should be using dynamic registrations
if (!_configurationService.CurrentSettings.CodeFolding.Enable) { return Task.FromResult(new Container<FoldingRange>()); }
// TODO: Should be using dynamic registrations
if (!_configurationService.CurrentSettings.CodeFolding.Enable)
{
return Task.FromResult(s_emptyFoldingRangeContainer);
}

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

List<FoldingRange> result = new();
if (!_workspaceService.TryGetFile(request.TextDocument.Uri, out ScriptFile scriptFile))
{
return Task.FromResult(s_emptyFoldingRangeContainer);
}

// If we're showing the last line, decrement the Endline of all regions by one.
int endLineOffset = _configurationService.CurrentSettings.CodeFolding.ShowLastLine ? -1 : 0;

List<FoldingRange> folds = new();
foreach (FoldingReference fold in TokenOperations.FoldableReferences(scriptFile.ScriptTokens).References)
{
result.Add(new FoldingRange
if (cancellationToken.IsCancellationRequested)
{
break;
}

folds.Add(new FoldingRange
{
EndCharacter = fold.EndCharacter,
EndLine = fold.EndLine + endLineOffset,
Expand All @@ -67,7 +77,9 @@ public override Task<Container<FoldingRange>> Handle(FoldingRangeRequestParam re
});
}

return Task.FromResult(new Container<FoldingRange>(result));
return folds.Count == 0
? Task.FromResult(s_emptyFoldingRangeContainer)
: Task.FromResult(new Container<FoldingRange>(folds));
}
}
}
Loading

0 comments on commit 1fbd190

Please sign in to comment.