(GH-824)(GH-812) Improve code folding speed #825

glennsarti · 2018-12-14T13:59:38Z

Previously there were no tests for PowerShell classes. This commit adds a simple
test for this scenario to ensure future changes do not break folding.

Previously the folding provider created many intermediate arrays and lists and
required post-processing. This commit changes the behaviour to use an
accumlator patter with an extended Dictionary class. This new class adds a
SafeAdd method to add FoldingRanges, which then has the logic to determine if
the range should indeed be added, for example, passing nulls or pre-existing
larger ranges.

By passing around this list using ByReference we can avoid creating many objects
which are just then thrown away.

Previously each token type detection was separated into discrete blocks
to make reading the code easier. However this meant there were many
enumerations of the Tokens array as well as passing around intermediate
arrays/lists. This commit takes the individual methods and overlaps them
to reduce the number of enumerations and regular expression matching to
a minimum.

Previously the code folding was not tested against DSC configuration scripts.
This commit adds tests for a sample DSC script to ensure the folding occurs at the correct
places

Fixes #824
Fixes #812

glennsarti · 2018-12-14T14:01:31Z

@TylerLeonhardt @rjmholt These are the code folding improvements from the AST work I did, but now are Token processing only.

Managed to shave nearly 50% runtime and probably reduced objects allocated significantly.

Raw data;
Given a script with 4096 foldable regions;

Before

Duration: 230.5017  <--- Ignored
Duration: 32.9991
Duration: 40.5561
Duration: 35.4358
Duration: 38.018
Duration: 36.9978
Duration: 34.0487
Duration: 31.9936
Duration: 34.9428
Duration: 41.0429
Duration: 36.0661
Duration: 34.0165
Duration: 37.0365
Duration: 34.026
Duration: 33.9919
Duration: 36.0026
Duration: 34.585
Duration: 39.5611
Duration: 34.5185
Duration: 34.5172
Avg Duration: 35.8082210526316

After

Duration: 132.0037  <--- Ignored
Duration: 15.9972
Duration: 15.963
Duration: 22.0686
Duration: 15.9767
Duration: 14.9559
Duration: 30.5756
Duration: 18.0449
Duration: 18.9995
Duration: 32.9537
Duration: 16.0499
Duration: 17.0227
Duration: 20.4783
Duration: 16.9985
Duration: 14.0394
Duration: 21.9654
Duration: 17.5853
Duration: 14.5099
Duration: 15.039
Duration: 26.9451
Avg Duration: 19.2720315789474

TylerLeonhardt

This looks really good - and those perf numbers 👌👌👌 awesome. Just had a couple comments but this is looking good.

src/PowerShellEditorServices/Language/TokenOperations.cs

glennsarti · 2018-12-15T09:20:50Z

@TylerLeonhardt Comments addressed

src/PowerShellEditorServices/Language/TokenOperations.cs

TylerLeonhardt

LGTM - the perf numbers are exciting 😃

glennsarti · 2018-12-15T10:26:34Z

@TylerLeonhardt Tag you're it. Typo fixed.

TylerLeonhardt · 2018-12-15T17:01:52Z

I'm good. Let's wait on 1 more 👍

rkeithhill · 2018-12-15T18:55:34Z

src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs

            foreach (FoldingReference fold in TokenOperations.FoldableRegions(
-                editorSession.Workspace.GetFile(documentUri).ScriptTokens,
-                this.currentSettings.CodeFolding.ShowLastLine))
+                script.ScriptTokens).ToArray())


Could you assign TokenOperations.FoldableRegions(script.ScriptTokens).ToArray() to an intermediate variable (foldableRegions?) for better debuggability and so this foreach doesn't have to wrap to a second line?

BTW is it really necessary to create/copy a new array here - which I see the ToArray method below is doing? The Dictionary.Values property will return an IEnumerable<FoldingReference> which is all you need to iterate (foreach) over it.

There are serialization errors when passing IEnumberable<FoldingReference> because the LSP needs FoldingRange[] (

PowerShellEditorServices/src/PowerShellEditorServices.Protocol/LanguageServer/Folding.cs

Line 40 in fbe8871

public class FoldingRange

)

And by errors I mean the serialization does not conform to the LSP specification. Which is why I do an object conversion. I tried to keep the number of array creations/copies to a minimum

Could you assign TokenOperations.FoldableRegions(script.ScriptTokens).ToArray() to an intermediate variable (foldableRegions?) for better debuggability and so this foreach doesn't have to wrap to a second line?

I can. I was trying to avoid creating array copies.

Using intermediates variables won't cause array copies.

There are serialization errors when passing IEnumberable

In this case (foreach case) you aren't really using the array but each element in the array. Therefore IEnumerable<FoldingReference> as the target of the foreach will yield individual FoldingReference objects that the foreach loop operates on. Now, you may use ToArray() later - when sending results back to the LSP client. In that case, sure, convert it to an array. But here, in this foreach loop, I still don't think it is necessary.

Fixed. Went with
foreach (KeyValuePair<int, FoldingReference> entry in TokenOperations.FoldableRegions(script.ScriptTokens))

When moving from inheritance to composition this has now changed to;
foreach (FoldingReference fold in TokenOperations.FoldableReferences(script.ScriptTokens).References)

src/PowerShellEditorServices/Language/FoldingReference.cs

src/PowerShellEditorServices/Language/TokenOperations.cs

rkeithhill · 2018-12-15T19:16:58Z

src/PowerShellEditorServices/Language/FoldingReference.cs

+        public void SafeAdd(FoldingReference item)
+        {
+            if (item == null) { return; }
+            FoldingReference currentItem;


Insert blank line here.

BTW you can see these various recommendations (rules) here SA1505 - SA1517. At some point, I'd really like to add this analyzer pkg to PSES and turn on these style rules.

Yeah I've been nagging @TylerLeonhardt and @rjmholt on slack about that. 😉 I'd love to see some linting tools, at the very least, I can run locally (and run in CI)

src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs

glennsarti · 2018-12-16T12:23:07Z

@rkeithhill Comments addressed.

rkeithhill

LGTM. Thanks for making those changes!

rjmholt · 2018-12-22T01:27:58Z

src/PowerShellEditorServices/Language/FoldingReference.cs

+    /// A class that holds a list of FoldingReferences and ensures that when adding a reference that the
+    /// folding rules are obeyed, e.g. Only one fold per start line
+    /// </summary>
+    public class FoldingReferenceList : Dictionary<int, FoldingReference>


If this doesn't require the all the dictionary methods, I would opt for composition over inheritance, so that no methods are exposed that would allow an invalid state in the list.

Fixed. Also took the opportunity to rename TokenOperations.FoldableRegions to TokenOperations.FoldableReferences as it returns a FoldingReferenceList type.

rjmholt · 2018-12-22T01:29:08Z

src/PowerShellEditorServices/Language/FoldingReference.cs

+            {
+                if (currentItem.CompareTo(item) == 1) { this[item.StartLine] = item; }
+            }
+            else


Can use return rather than else here

This function returns void so the return doesn't seem to be needed.

I think he meant:

if (references.TryGetValue(item.StartLine, out FoldingReference currentItem)) { if (currentItem.CompareTo(item) == 1) { references[item.StartLine] = item; } return; } references[item.StartLine] = item;

rjmholt · 2018-12-22T01:34:57Z

src/PowerShellEditorServices/Language/TokenOperations.cs

+                    tokenCommentRegionStack.Push(token);
+                    continue;
+                }
+                else if (s_endRegionTextRegex.IsMatch(token.Text))


No need for else given continue above

glennsarti · 2019-01-07T07:53:13Z

Rebased due to conflict

TylerLeonhardt · 2019-01-08T03:49:27Z

@glennsarti can you deal with the last conflict?

@rjmholt can you look at this when you can? I know you're on your way back to Seattle so no rush

glennsarti · 2019-01-08T03:50:03Z

Stop merging other stuff :-)

Previously there were no tests for PowerShell classes. This commit adds a simple test for this scenario to ensure future changes do not break folding.

glennsarti · 2019-01-08T05:30:13Z

Rebased, waiting on CI

TylerLeonhardt · 2019-01-11T21:22:41Z

ping @glennsarti about CI

…o it's own class Previously the folding provider created many intermediate arrays and lists and required post-processing. This commit changes the behaviour to use an accumlator patter with an extended Dictionary class. This new class adds a `SafeAdd` method to add FoldingRanges, which then has the logic to determine if the range should indeed be added, for example, passing nulls or pre-existing larger ranges. By passing around this list using ByReference we can avoid creating many objects which are just then thrown away. This commit also moves the ShowLastLine code from the FoldingProvider into the Language Server. This reduces the number of array enumerations to one.

Previously each token type detection was separated into discrete blocks to make reading the code easier. However this meant there were many enumerations of the Tokens array as well as passing around intermediate arrays/lists. This commit takes the individual methods and overlaps them to reduce the number of enumerations and regular expression matching to a minimum. Note that there are considerable code comments here due to the code now being more complex on initial review.

Previously the code folding was not tested against DSC configuration scripts. This commit adds tests for a sample DSC script to ensure the folding occurs at the correct places

Previously the folder would search for the region markers without case sensitivity. This commit modifies the regular expressions to be more strict on the what is a region marker and adds a negative test to ensure that regions that are not cased correctly are not folded .

glennsarti · 2019-01-12T08:05:44Z

@TylerLeonhardt Yeah whoops, broke after the rebase. Also fixed up some codacy violations.

glennsarti requested a review from rjmholt as a code owner December 14, 2018 13:59

glennsarti mentioned this pull request Dec 14, 2018

{WIP} (GH-813) Use AST for Syntax Folder #806

Closed

TylerLeonhardt reviewed Dec 15, 2018

View reviewed changes

src/PowerShellEditorServices/Language/TokenOperations.cs Show resolved Hide resolved

src/PowerShellEditorServices/Language/TokenOperations.cs Outdated Show resolved Hide resolved

glennsarti force-pushed the make-folding-bettere branch from cb58f69 to acea280 Compare December 15, 2018 09:18

TylerLeonhardt reviewed Dec 15, 2018

View reviewed changes

src/PowerShellEditorServices/Language/TokenOperations.cs Outdated Show resolved Hide resolved

TylerLeonhardt approved these changes Dec 15, 2018

View reviewed changes

TylerLeonhardt requested review from SeeminglyScience and rkeithhill December 15, 2018 09:47

glennsarti force-pushed the make-folding-bettere branch from acea280 to 420f9a0 Compare December 15, 2018 10:25