From 27b5325714dbd5154a45d6a93663be76520db06f Mon Sep 17 00:00:00 2001 From: Andy Jordan <2226434+andyleejordan@users.noreply.github.com> Date: Wed, 27 Sep 2023 15:14:45 -0700 Subject: [PATCH 1/3] Add `SortDocumentSymbols` to make the outline hierarchical (again) This is probably what OmniSharp was doing for us. --- .../Handlers/DocumentSymbolHandler.cs | 91 +++++++++++++++++-- 1 file changed, 85 insertions(+), 6 deletions(-) diff --git a/src/PowerShellEditorServices/Services/TextDocument/Handlers/DocumentSymbolHandler.cs b/src/PowerShellEditorServices/Services/TextDocument/Handlers/DocumentSymbolHandler.cs index f416d7615..01c1454fd 100644 --- a/src/PowerShellEditorServices/Services/TextDocument/Handlers/DocumentSymbolHandler.cs +++ b/src/PowerShellEditorServices/Services/TextDocument/Handlers/DocumentSymbolHandler.cs @@ -40,13 +40,79 @@ public PsesDocumentSymbolHandler(ILoggerFactory factory, WorkspaceService worksp DocumentSelector = LspUtils.PowerShellDocumentSelector }; + // This turns a flat list of symbols into a hierarchical list. It's ugly because we're + // dealing with records and so sadly must slowly copy and replace things whenever need to do + // a modification, but it seems to work. + private static async Task> SortDocumentSymbols(List symbols, CancellationToken cancellationToken) + { + // Sort by the start of the symbol definition. + symbols.Sort((x1, x2) => x1.Range.Start.CompareTo(x2.Range.Start)); + + List parents = new(); + + foreach (DocumentSymbol symbol in symbols) + { + // This async method is pretty dense with synchronous code + // so it's helpful to add some yields. + await Task.Yield(); + if (cancellationToken.IsCancellationRequested) + { + return symbols; + } + + // Base case. + if (parents.Count == 0) + { + parents.Add(symbol); + } + // Symbol starts after end of last symbol parsed. + else if (symbol.Range.Start > parents[parents.Count - 1].Range.End) + { + parents.Add(symbol); + } + // Find where it fits. + else + { + for (int i = 0; i < parents.Count; i++) + { + DocumentSymbol parent = parents[i]; + if (parent.Range.Start <= symbol.Range.Start && symbol.Range.End <= parent.Range.End) + { + List children = new(); + if (parent.Children is not null) + { + children.AddRange(parent.Children); + } + children.Add(symbol); + parents[i] = parent with { Children = children }; + break; + } + } + } + } + + // Recursively sort the children. + for (int i = 0; i < parents.Count; i++) + { + DocumentSymbol parent = parents[i]; + if (parent.Children is not null) + { + List children = new(parent.Children); + children = await SortDocumentSymbols(children, cancellationToken).ConfigureAwait(false); + parents[i] = parent with { Children = children }; + } + } + + return parents; + } + // AKA the outline feature public override async Task Handle(DocumentSymbolParams request, CancellationToken cancellationToken) { _logger.LogDebug($"Handling document symbols for {request.TextDocument.Uri}"); ScriptFile scriptFile = _workspaceService.GetFile(request.TextDocument.Uri); - List symbols = new(); + List symbols = new(); foreach (SymbolReference r in ProvideDocumentSymbols(scriptFile)) { @@ -71,18 +137,31 @@ public override async Task Handle(Do // symbols, and we don't have the information nor algorithm to do that currently. // OmniSharp was previously doing this for us based on the range, perhaps we can // find that logic and reuse it. - symbols.Add(new SymbolInformationOrDocumentSymbol(new DocumentSymbol + symbols.Add(new DocumentSymbol { Kind = SymbolTypeUtils.GetSymbolKind(r.Type), Range = r.ScriptRegion.ToRange(), SelectionRange = r.NameRegion.ToRange(), Name = r.Name - })); + }); + } + + // Short-circuit if we have no symbols. + if (symbols.Count == 0) + { + return s_emptySymbolInformationOrDocumentSymbolContainer; } - return symbols.Count == 0 - ? s_emptySymbolInformationOrDocumentSymbolContainer - : new SymbolInformationOrDocumentSymbolContainer(symbols); + // Otherwise slowly sort them into a hierarchy. + symbols = await SortDocumentSymbols(symbols, cancellationToken).ConfigureAwait(false); + + // And finally convert them to the silly SymbolInformationOrDocumentSymbol wrapper. + List container = new(); + foreach (DocumentSymbol symbol in symbols) + { + container.Add(new SymbolInformationOrDocumentSymbol(symbol)); + } + return container; } private IEnumerable ProvideDocumentSymbols(ScriptFile scriptFile) From 8de86ff327b0590411580a0324dfe020a328f616 Mon Sep 17 00:00:00 2001 From: Andy Jordan <2226434+andyleejordan@users.noreply.github.com> Date: Thu, 12 Oct 2023 17:15:35 -0700 Subject: [PATCH 2/3] Improve algorithm to avoid excess allocations --- .../Handlers/DocumentSymbolHandler.cs | 116 +++++++++++------- 1 file changed, 69 insertions(+), 47 deletions(-) diff --git a/src/PowerShellEditorServices/Services/TextDocument/Handlers/DocumentSymbolHandler.cs b/src/PowerShellEditorServices/Services/TextDocument/Handlers/DocumentSymbolHandler.cs index 01c1454fd..75f854a50 100644 --- a/src/PowerShellEditorServices/Services/TextDocument/Handlers/DocumentSymbolHandler.cs +++ b/src/PowerShellEditorServices/Services/TextDocument/Handlers/DocumentSymbolHandler.cs @@ -40,81 +40,106 @@ public PsesDocumentSymbolHandler(ILoggerFactory factory, WorkspaceService worksp DocumentSelector = LspUtils.PowerShellDocumentSelector }; - // This turns a flat list of symbols into a hierarchical list. It's ugly because we're - // dealing with records and so sadly must slowly copy and replace things whenever need to do - // a modification, but it seems to work. - private static async Task> SortDocumentSymbols(List symbols, CancellationToken cancellationToken) + // This turns a flat list of symbols into a hierarchical list. + private static async Task> SortHierarchicalSymbols(List symbols, CancellationToken cancellationToken) { - // Sort by the start of the symbol definition. + // Sort by the start of the symbol definition (they're probably sorted but we need to be + // certain otherwise this algorithm won't work). symbols.Sort((x1, x2) => x1.Range.Start.CompareTo(x2.Range.Start)); - List parents = new(); + List parents = new(); - foreach (DocumentSymbol symbol in symbols) + foreach (HierarchicalSymbol symbol in symbols) { // This async method is pretty dense with synchronous code // so it's helpful to add some yields. await Task.Yield(); if (cancellationToken.IsCancellationRequested) { - return symbols; + return parents; } - - // Base case. + // Base case where we haven't found any parents yet. if (parents.Count == 0) { parents.Add(symbol); } - // Symbol starts after end of last symbol parsed. + // If the symbol starts after end of last symbol parsed then it's a new parent. else if (symbol.Range.Start > parents[parents.Count - 1].Range.End) { parents.Add(symbol); } - // Find where it fits. + // Otherwise it's a child, we just need to figure out whose child it is. else { - for (int i = 0; i < parents.Count; i++) + foreach (HierarchicalSymbol parent in parents) { - DocumentSymbol parent = parents[i]; - if (parent.Range.Start <= symbol.Range.Start && symbol.Range.End <= parent.Range.End) + // If the symbol starts after the parent starts and ends before the parent + // ends then its a child. + if (symbol.Range.Start > parent.Range.Start && symbol.Range.End < parent.Range.End) { - List children = new(); - if (parent.Children is not null) - { - children.AddRange(parent.Children); - } - children.Add(symbol); - parents[i] = parent with { Children = children }; + parent.Children.Add(symbol); break; } } + // TODO: If we somehow exist the list of parents and didn't find a place for the + // child...we have a problem. } } - // Recursively sort the children. - for (int i = 0; i < parents.Count; i++) + // Now recursively sort the children into nested buckets of children too. + foreach (HierarchicalSymbol parent in parents) { - DocumentSymbol parent = parents[i]; - if (parent.Children is not null) - { - List children = new(parent.Children); - children = await SortDocumentSymbols(children, cancellationToken).ConfigureAwait(false); - parents[i] = parent with { Children = children }; - } + List sortedChildren = await SortHierarchicalSymbols(parent.Children, cancellationToken).ConfigureAwait(false); + // Since this is a foreach we can't just assign to parent.Children and have to do + // this instead. + parent.Children.Clear(); + parent.Children.AddRange(sortedChildren); } return parents; } - // AKA the outline feature + // This struct and the mapping function below exist to allow us to skip a *bunch* of + // unnecessary allocations when sorting the symbols since DocumentSymbol (which this is + // pretty much a mirror of) is an immutable record...but we need to constantly modify the + // list of children when sorting. + private struct HierarchicalSymbol + { + public SymbolKind Kind; + public Range Range; + public Range SelectionRange; + public string Name; + public List Children; + } + + // Recursively turn our HierarchicalSymbol struct into OmniSharp's DocumentSymbol record. + private static List GetDocumentSymbolsFromHierarchicalSymbols(IEnumerable hierarchicalSymbols) + { + List documentSymbols = new(); + foreach (HierarchicalSymbol symbol in hierarchicalSymbols) + { + documentSymbols.Add(new DocumentSymbol + { + Kind = symbol.Kind, + Range = symbol.Range, + SelectionRange = symbol.SelectionRange, + Name = symbol.Name, + Children = GetDocumentSymbolsFromHierarchicalSymbols(symbol.Children) + }); + } + return documentSymbols; + } + + // AKA the outline feature! public override async Task Handle(DocumentSymbolParams request, CancellationToken cancellationToken) { _logger.LogDebug($"Handling document symbols for {request.TextDocument.Uri}"); ScriptFile scriptFile = _workspaceService.GetFile(request.TextDocument.Uri); - List symbols = new(); - foreach (SymbolReference r in ProvideDocumentSymbols(scriptFile)) + List hierarchicalSymbols = new(); + + foreach (SymbolReference symbolReference in ProvideDocumentSymbols(scriptFile)) { // This async method is pretty dense with synchronous code // so it's helpful to add some yields. @@ -128,36 +153,33 @@ public override async Task Handle(Do // // TODO: We should also include function invocations that are part of DSLs (like // Invoke-Build etc.). - if (!r.IsDeclaration || r.Type is SymbolType.Parameter) + if (!symbolReference.IsDeclaration || symbolReference.Type is SymbolType.Parameter) { continue; } - // TODO: This now needs the Children property filled out to support hierarchical - // symbols, and we don't have the information nor algorithm to do that currently. - // OmniSharp was previously doing this for us based on the range, perhaps we can - // find that logic and reuse it. - symbols.Add(new DocumentSymbol + hierarchicalSymbols.Add(new HierarchicalSymbol { - Kind = SymbolTypeUtils.GetSymbolKind(r.Type), - Range = r.ScriptRegion.ToRange(), - SelectionRange = r.NameRegion.ToRange(), - Name = r.Name + Kind = SymbolTypeUtils.GetSymbolKind(symbolReference.Type), + Range = symbolReference.ScriptRegion.ToRange(), + SelectionRange = symbolReference.NameRegion.ToRange(), + Name = symbolReference.Name, + Children = new List() }); } // Short-circuit if we have no symbols. - if (symbols.Count == 0) + if (hierarchicalSymbols.Count == 0) { return s_emptySymbolInformationOrDocumentSymbolContainer; } // Otherwise slowly sort them into a hierarchy. - symbols = await SortDocumentSymbols(symbols, cancellationToken).ConfigureAwait(false); + hierarchicalSymbols = await SortHierarchicalSymbols(hierarchicalSymbols, cancellationToken).ConfigureAwait(false); // And finally convert them to the silly SymbolInformationOrDocumentSymbol wrapper. List container = new(); - foreach (DocumentSymbol symbol in symbols) + foreach (DocumentSymbol symbol in GetDocumentSymbolsFromHierarchicalSymbols(hierarchicalSymbols)) { container.Add(new SymbolInformationOrDocumentSymbol(symbol)); } From d68be7ddc256951a8b6ac34709071c2e9f7e2d5d Mon Sep 17 00:00:00 2001 From: Andy Jordan <2226434+andyleejordan@users.noreply.github.com> Date: Wed, 18 Oct 2023 18:16:18 -0700 Subject: [PATCH 3/3] Optimize algorithm by sorting in-place This finishes the optimizations by relying on the fact that we're passing a reference to the list which means we can modify it in-place. We sort the whole list of symbols on the first pass only, and then as sort children into parents' buckets we *move* them by adding them to `parent.Children` and removing them from the current list, and then recurse and modify those lists until we're done. I don't normally like relying on side-effects such as modifying in the called function, but it makes the most sense in this case for optimizations and is duly noted. --- .../Handlers/DocumentSymbolHandler.cs | 65 +++++++++++-------- 1 file changed, 39 insertions(+), 26 deletions(-) diff --git a/src/PowerShellEditorServices/Services/TextDocument/Handlers/DocumentSymbolHandler.cs b/src/PowerShellEditorServices/Services/TextDocument/Handlers/DocumentSymbolHandler.cs index 75f854a50..868d3af76 100644 --- a/src/PowerShellEditorServices/Services/TextDocument/Handlers/DocumentSymbolHandler.cs +++ b/src/PowerShellEditorServices/Services/TextDocument/Handlers/DocumentSymbolHandler.cs @@ -2,6 +2,7 @@ // Licensed under the MIT License. using System.Collections.Generic; +using System.Diagnostics; using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.Logging; @@ -40,63 +41,75 @@ public PsesDocumentSymbolHandler(ILoggerFactory factory, WorkspaceService worksp DocumentSelector = LspUtils.PowerShellDocumentSelector }; - // This turns a flat list of symbols into a hierarchical list. - private static async Task> SortHierarchicalSymbols(List symbols, CancellationToken cancellationToken) + // Modifies a flat list of symbols into a hierarchical list. + private static Task SortHierarchicalSymbols(List symbols, CancellationToken cancellationToken) { // Sort by the start of the symbol definition (they're probably sorted but we need to be - // certain otherwise this algorithm won't work). + // certain otherwise this algorithm won't work). We only need to sort the list once, and + // since the implementation is recursive, it's easiest to use the stack to track that + // this is the first call. symbols.Sort((x1, x2) => x1.Range.Start.CompareTo(x2.Range.Start)); + return SortHierarchicalSymbolsImpl(symbols, cancellationToken); + } - List parents = new(); - - foreach (HierarchicalSymbol symbol in symbols) + private static async Task SortHierarchicalSymbolsImpl(List symbols, CancellationToken cancellationToken) + { + for (int i = 0; i < symbols.Count; i++) { // This async method is pretty dense with synchronous code // so it's helpful to add some yields. await Task.Yield(); if (cancellationToken.IsCancellationRequested) { - return parents; + return; } - // Base case where we haven't found any parents yet. - if (parents.Count == 0) + + HierarchicalSymbol symbol = symbols[i]; + + // Base case where we haven't found any parents yet (the first symbol must be a + // parent by definition). + if (i == 0) { - parents.Add(symbol); + continue; } // If the symbol starts after end of last symbol parsed then it's a new parent. - else if (symbol.Range.Start > parents[parents.Count - 1].Range.End) + else if (symbol.Range.Start > symbols[i - 1].Range.End) { - parents.Add(symbol); + continue; } - // Otherwise it's a child, we just need to figure out whose child it is. + // Otherwise it's a child, we just need to figure out whose child it is and move it there (which also means removing it from the current list). else { - foreach (HierarchicalSymbol parent in parents) + for (int j = 0; j <= i; j++) { + // While we should only check up to j < i, we iterate up to j <= i so that + // we can check this assertion that we didn't exhaust the parents. + Debug.Assert(j != i, "We didn't find the child's parent!"); + + HierarchicalSymbol parent = symbols[j]; // If the symbol starts after the parent starts and ends before the parent // ends then its a child. if (symbol.Range.Start > parent.Range.Start && symbol.Range.End < parent.Range.End) { + // Add it to the parent's list. parent.Children.Add(symbol); + // Remove it from this "parents" list (because it's a child) and adjust + // our loop counter because it's been removed. + symbols.RemoveAt(i); + i--; break; } } - // TODO: If we somehow exist the list of parents and didn't find a place for the - // child...we have a problem. } } // Now recursively sort the children into nested buckets of children too. - foreach (HierarchicalSymbol parent in parents) + foreach (HierarchicalSymbol parent in symbols) { - List sortedChildren = await SortHierarchicalSymbols(parent.Children, cancellationToken).ConfigureAwait(false); - // Since this is a foreach we can't just assign to parent.Children and have to do - // this instead. - parent.Children.Clear(); - parent.Children.AddRange(sortedChildren); + // Since this modifies in place we just recurse, no re-assignment or clearing from + // parent.Children necessary. + await SortHierarchicalSymbols(parent.Children, cancellationToken).ConfigureAwait(false); } - - return parents; } // This struct and the mapping function below exist to allow us to skip a *bunch* of @@ -174,8 +187,8 @@ public override async Task Handle(Do return s_emptySymbolInformationOrDocumentSymbolContainer; } - // Otherwise slowly sort them into a hierarchy. - hierarchicalSymbols = await SortHierarchicalSymbols(hierarchicalSymbols, cancellationToken).ConfigureAwait(false); + // Otherwise slowly sort them into a hierarchy (this modifies the list). + await SortHierarchicalSymbols(hierarchicalSymbols, cancellationToken).ConfigureAwait(false); // And finally convert them to the silly SymbolInformationOrDocumentSymbol wrapper. List container = new();