Skip to content

Signaturehelp cancellation and some caching #1251

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Apr 8, 2020
Merged

Signaturehelp cancellation and some caching #1251

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bf46f94
SignatureHelp cancellation & some caching in CommandHelpers
TylerLeonhardt Apr 7, 2020
19fca4c
add comment
TylerLeonhardt Apr 7, 2020
c430554
better language
TylerLeonhardt Apr 7, 2020
45d9e08
fixed wording
TylerLeonhardt Apr 7, 2020
bdcfcdc
address feedback from Keith and Patrick
TylerLeonhardt Apr 8, 2020
ff09fe0
move to hashset, remove static ctr
TylerLeonhardt Apr 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 48 additions & 13 deletions src/PowerShellEditorServices/Services/PowerShellContext/Utilities/CommandHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,26 @@ namespace Microsoft.PowerShell.EditorServices.Services.PowerShellContext
/// </summary>
internal static class CommandHelpers
{
private static readonly ConcurrentDictionary<string, bool> NounExclusionList =
private static readonly ConcurrentDictionary<string, bool> s_nounExclusionList =
new ConcurrentDictionary<string, bool>();

private static readonly ConcurrentDictionary<string, CommandInfo> s_commandInfoCache =
new ConcurrentDictionary<string, CommandInfo>();

private static readonly ConcurrentDictionary<string, string> s_synopsisCache =
new ConcurrentDictionary<string, string>();

static CommandHelpers()
{
NounExclusionList.TryAdd("Module", true);
NounExclusionList.TryAdd("Script", true);
NounExclusionList.TryAdd("Package", true);
NounExclusionList.TryAdd("PackageProvider", true);
NounExclusionList.TryAdd("PackageSource", true);
NounExclusionList.TryAdd("InstalledModule", true);
NounExclusionList.TryAdd("InstalledScript", true);
NounExclusionList.TryAdd("ScriptFileInfo", true);
NounExclusionList.TryAdd("PSRepository", true);
s_nounExclusionList.TryAdd("Module", true);
s_nounExclusionList.TryAdd("Script", true);
s_nounExclusionList.TryAdd("Package", true);
s_nounExclusionList.TryAdd("PackageProvider", true);
s_nounExclusionList.TryAdd("PackageSource", true);
s_nounExclusionList.TryAdd("InstalledModule", true);
s_nounExclusionList.TryAdd("InstalledScript", true);
s_nounExclusionList.TryAdd("ScriptFileInfo", true);
s_nounExclusionList.TryAdd("PSRepository", true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the comment on line 62, shouldn't PowerShellGet be in this list?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah nevermind, this is the noun list, duh. Not the package name list.

Copy link
Contributor

@rkeithhill rkeithhill Apr 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, I see more nouns from PowerShellGet 2.2.3 than appear in the list above. gcm -m PowerShellGet | % Noun | % Sort -uniq Is that a problem? For instance, there is a Find-Command function in PowerShellGet.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I just renamed the variable. We should have a better check for verb-noun when PSGet uses the same noun as an inbox module Module as an example.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a s_cmdletExclusionList now which will handle that ^

}

/// <summary>
Expand All @@ -45,12 +51,18 @@ public static async Task<CommandInfo> GetCommandInfoAsync(
Validate.IsNotNull(nameof(commandName), commandName);
Validate.IsNotNull(nameof(powerShellContext), powerShellContext);

// Make sure the command's noun isn't blacklisted. This is
// If we have a CommandInfo cached, return that.
if (s_commandInfoCache.TryGetValue(commandName, out CommandInfo cmdInfo))
{
return cmdInfo;
}

// Make sure the command's noun isn't in the exclusion list. This is
// currently necessary to make sure that Get-Command doesn't
// load PackageManagement or PowerShellGet because they cause
// a major slowdown in IntelliSense.
var commandParts = commandName.Split('-');
if (commandParts.Length == 2 && NounExclusionList.ContainsKey(commandParts[1]))
if (commandParts.Length == 2 && s_nounExclusionList.ContainsKey(commandParts[1]))
{
return null;
}
Expand All @@ -60,10 +72,18 @@ public static async Task<CommandInfo> GetCommandInfoAsync(
command.AddArgument(commandName);
command.AddParameter("ErrorAction", "Ignore");

return (await powerShellContext.ExecuteCommandAsync<PSObject>(command, sendOutputToHost: false, sendErrorToHost: false).ConfigureAwait(false))
CommandInfo commandInfo = (await powerShellContext.ExecuteCommandAsync<PSObject>(command, sendOutputToHost: false, sendErrorToHost: false).ConfigureAwait(false))
.Select(o => o.BaseObject)
.OfType<CommandInfo>()
.FirstOrDefault();

// Only cache CmdletInfos since they're exposed in binaries they are likely to not change throughout the session.
if (commandInfo.CommandType == CommandTypes.Cmdlet)
{
s_commandInfoCache.TryAdd(commandName, commandInfo);
}

return commandInfo;
}

/// <summary>
Expand All @@ -87,6 +107,15 @@ public static async Task<string> GetCommandSynopsisAsync(
return string.Empty;
}

// If we have a synopsis cached, return that.
// NOTE: If the user runs Update-Help, it's possible that this synopsis will be out of date.
// Given the perf increase of doing this, and the simple workaround of restarting the extension,
// this seems worth it.
if (s_synopsisCache.TryGetValue(commandInfo.Name, out string synopsis))
{
return synopsis;
}

PSCommand command = new PSCommand()
.AddCommand(@"Microsoft.PowerShell.Core\Get-Help")
// We use .Name here instead of just passing in commandInfo because
Expand All @@ -102,6 +131,12 @@ public static async Task<string> GetCommandSynopsisAsync(
(string)helpObject?.Properties["synopsis"].Value ??
string.Empty;

// Only cache cmdlet infos because since they're exposed in binaries, the can never change throughout the session.
if (commandInfo.CommandType == CommandTypes.Cmdlet)
{
s_synopsisCache.TryAdd(commandInfo.Name, synopsisString);
}

// Ignore the placeholder value for this field
if (string.Equals(synopsisString, "SHORT DESCRIPTION", System.StringComparison.CurrentCultureIgnoreCase))
{
Expand Down
5 changes: 1 addition & 4 deletions src/PowerShellEditorServices/Services/TextDocument/Handlers/HoverHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,20 +22,17 @@ internal class HoverHandler : IHoverHandler
private readonly ILogger _logger;
private readonly SymbolsService _symbolsService;
private readonly WorkspaceService _workspaceService;
private readonly PowerShellContextService _powerShellContextService;

private HoverCapability _capability;

public HoverHandler(
ILoggerFactory factory,
SymbolsService symbolsService,
WorkspaceService workspaceService,
PowerShellContextService powerShellContextService)
WorkspaceService workspaceService)
{
_logger = factory.CreateLogger<HoverHandler>();
_symbolsService = symbolsService;
_workspaceService = workspaceService;
_powerShellContextService = powerShellContextService;
}

public HoverRegistrationOptions GetRegistrationOptions()
Expand Down
43 changes: 24 additions & 19 deletions src/PowerShellEditorServices/Services/TextDocument/Handlers/SignatureHelpHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ namespace Microsoft.PowerShell.EditorServices.Handlers
{
internal class SignatureHelpHandler : ISignatureHelpHandler
{
private static readonly SignatureInformation[] s_emptySignatureResult = Array.Empty<SignatureInformation>();
private readonly ILogger _logger;
private readonly SymbolsService _symbolsService;
private readonly WorkspaceService _workspaceService;
Expand Down Expand Up @@ -52,6 +51,12 @@ public SignatureHelpRegistrationOptions GetRegistrationOptions()

public async Task<SignatureHelp> Handle(SignatureHelpParams request, CancellationToken cancellationToken)
{
if (cancellationToken.IsCancellationRequested)
{
_logger.LogDebug("SignatureHelp request canceled for file: {0}", request.TextDocument.Uri);
return new SignatureHelp();
}

ScriptFile scriptFile = _workspaceService.GetFile(request.TextDocument.Uri);

ParameterSetSignatures parameterSets =
Expand All @@ -61,28 +66,28 @@ await _symbolsService.FindParameterSetsInFileAsync(
(int) request.Position.Character + 1,
_powerShellContextService).ConfigureAwait(false);

SignatureInformation[] signatures = s_emptySignatureResult;
if (parameterSets == null)
{
return new SignatureHelp();
}

if (parameterSets != null)
SignatureInformation[] signatures = new SignatureInformation[parameterSets.Signatures.Length];
for (int i = 0; i < signatures.Length; i++)
{
signatures = new SignatureInformation[parameterSets.Signatures.Length];
for (int i = 0; i < signatures.Length; i++)
var parameters = new ParameterInformation[parameterSets.Signatures[i].Parameters.Count()];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know why Parameters is typed as IEnumerable<>? I know this is existing code, but while you're here it might be worth changing that to an array and use Length here instead of Count().

If it's IEnumerable<> because it's a yield enumerator or LINQ then Count() should be skipped and a List<> with a best estimate capacity should be used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really sure what the best estimate could be here... so I'll just leave it empty.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added!

int j = 0;
foreach (ParameterInfo param in parameterSets.Signatures[i].Parameters)
{
var parameters = new ParameterInformation[parameterSets.Signatures[i].Parameters.Count()];
int j = 0;
foreach (ParameterInfo param in parameterSets.Signatures[i].Parameters)
{
parameters[j] = CreateParameterInfo(param);
j++;
}

signatures[i] = new SignatureInformation
{
Label = parameterSets.CommandName + " " + parameterSets.Signatures[i].SignatureText,
Documentation = null,
Parameters = parameters,
};
parameters[j] = CreateParameterInfo(param);
j++;
}

signatures[i] = new SignatureInformation
{
Label = parameterSets.CommandName + " " + parameterSets.Signatures[i].SignatureText,
Documentation = null,
Parameters = parameters,
};
}

return new SignatureHelp
Expand Down