Reduce allocations in the CodeLens providers #719

rjmholt · 2018-08-08T19:48:31Z

Went and tackled some easy stuff in the CodeLens provider (since disabling it seems to give the best performance improvement for EditorServices).

There should be no semantic changes from this.

I didn't go after caching, since that's a bit riskier and probably needs some design discussion about where the caches live (i.e. what objects are responsible for them).

rkeithhill · 2018-08-08T20:26:55Z

src/PowerShellEditorServices.Host/CodeLens/CodeLensFeature.cs

-            var scriptFile =
-                _editorSession.Workspace.GetFile(
-                    codeLensParams.TextDocument.Uri);
+            ScriptFile scriptFile = _editorSession.Workspace.GetFile(


I like. The rule for using var on my team is this - use var when the type of the RHS is obvious i.e. it's a literal, a new statement or type-cast. And also when required - anonymous types. Do not use var when the type is not obvious, which it usually isn't when invoking a method (or reading a property).

100% agreed on that rule!

Also 100% agree!

SeeminglyScience

Looks great! One concern/question

SeeminglyScience · 2018-08-08T20:48:23Z

src/PowerShellEditorServices.Host/CodeLens/CodeLensFeature.cs

+                acc.AddRange(providerResult);
+            }
+
+            return acc.ToArray();


I would think allocating a list and then an array would end up with more allocations. I don't know enough about how SelectMany is implemented to say that with any certainty, but maybe a quick benchmark test would be a good idea.

Speaking of, benchmarks is a good bullet point for #720

So I wrote a nasty little benchmark:

class Program { private static Random s_random = new Random(); static void Main(string[] args) { var tests = new Dictionary<string, Action> { { "Select Many Concatenation", ConcatWithSelectMany }, { "List Build Concatentation", ConcatWithList }, }; RunBenchmark(tests); } private static void ConcatWithSelectMany() { GenerateMultiData(d => d.ToTransformedDummyData()).SelectMany(d => d).ToArray(); } private static void ConcatWithList() { var acc = new List<TransformedDummyData>(); foreach (TransformedDummyData[] dataArr in GenerateMultiData(d => d.ToTransformedDummyData())) { acc.AddRange(dataArr); } acc.ToArray(); } private static IEnumerable<TransformedDummyData[]> GenerateMultiData(Func<DummyData, TransformedDummyData> process, int quantity = 5) { for (int i = 0; i < quantity; i++) { yield return GenerateData(s_random.Next(5, 15)).Select(process).ToArray(); } } private static IEnumerable<DummyData> GenerateData(int quantity = 10) { for (int i = 0; i < quantity; i++) { yield return new DummyData(s_random.Next(), new object(), Guid.NewGuid().ToString()); } } private static IntervalData Time(string testName, Action action, int repeats = 1000, int subInterval = 100) { var times = new List<double>(repeats); for (int i = 0; i < repeats; i++) { var watch = Stopwatch.StartNew(); for (int j = 0; j < subInterval; j++) { action(); } watch.Stop(); times.Add((double)watch.ElapsedMilliseconds / subInterval); } double maxTime = times.Max(); double minTime = times.Min(); double meanTime = times.Average(); double[] sortedTimes = times.OrderBy(t => t).ToArray(); return new IntervalData { TestName = testName, MaxTime = maxTime, MaxTimeIndex = times.IndexOf(maxTime), MinTime = minTime, MinTimeIndex = times.IndexOf(minTime), TotalTime = times.Sum(), MeanTime = meanTime, StdDev = Math.Sqrt(times.Sum(t => Math.Pow(t - meanTime, 2)) / (times.Count - 1)), MedianTime = sortedTimes.Length % 2 == 0 ? sortedTimes[sortedTimes.Length / 2] : sortedTimes[sortedTimes.Length / 2 + 1] }; } private static void RunBenchmark(IDictionary<string, Action> tests) { foreach (KeyValuePair<string, Action> test in tests) { IntervalData result = Time(test.Key, test.Value); Console.WriteLine($"Test {result.TestName}:"); Console.WriteLine($"\tTotal Time:\t\t\t{result.TotalTime}"); Console.WriteLine($"\tMean Time:\t\t\t{result.MeanTime}"); Console.WriteLine($"\tMedian Time:\t\t\t{result.MedianTime}"); Console.WriteLine($"\tStd Dev:\t\t\t{result.StdDev}"); Console.WriteLine($"\tMax Time:\t\t\t{result.MaxTime}"); Console.WriteLine($"\tMax Time Index:\t\t\t{result.MaxTimeIndex}"); Console.WriteLine($"\tMin Time:\t\t\t{result.MinTime}"); Console.WriteLine($"\tMin Time Index:\t\t\t{result.MinTimeIndex}"); Console.WriteLine(); } } } public class DummyData { public DummyData(int x, object y, string z) { X = x; Y = y; Z = z; } public int X { get; } public object Y { get; } public string Z { get; } } public static class DummyDataExtensions { public static TransformedDummyData ToTransformedDummyData(this DummyData data) { return new TransformedDummyData(data.X+1, data.Z?.ToUpper()); } } public class TransformedDummyData { public TransformedDummyData(int x, string z) { X = x; Z = z; } public int X { get; } public string Z { get; } } public class IntervalData { public string TestName; public double MaxTime; public int MaxTimeIndex; public double MinTime; public int MinTimeIndex; public double TotalTime; public double MeanTime; public double MedianTime; public double StdDev; }

With the following results:

Test Select Many Concatenation: Total Time: 60.7800000000007 Mean Time: 0.0607800000000007 Median Time: 0.06 Std Dev: 0.00494125652198061 Max Time: 0.13 Max Time Index: 0 Min Time: 0.06 Min Time Index: 3 Test List Build Concatentation: Total Time: 60.4900000000008 Mean Time: 0.0604900000000008 Median Time: 0.06 Std Dev: 0.00472040063788888 Max Time: 0.13 Max Time Index: 431 Min Time: 0.06 Min Time Index: 1

Don't know how representative my benchmark really is, but given that result, I will revert to SelectMany since it's simpler and may end up allocating less in the long run.

Awesome!

I converted your benchmark to use BenchmarkDotNet.

Here's the results I got in CoreCLR/Windows

Method Mean Error StdDev Scaled ScaledSD Gen 0 Allocated

ConcatWithSelectMany 36.28 us 0.7209 us 0.8302 us 1.00 0.00 5.4321 16.82 KB

ConcatWithList 38.01 us 0.9687 us 2.8562 us 1.05 0.08 5.6763 17.6 KB

Ah, see, that's what I should be doing -- using the grown ups tools.

I tested on Linux btw. So Framework is possibly different.

SeeminglyScience · 2018-08-08T20:49:35Z

src/PowerShellEditorServices.Host/CodeLens/PesterCodeLensProvider.cs


-            return lenses;
+            return lenses.ToArray();


echo: SelectMany vs List > Array

Going to leave this one on the basis of avoiding some LINQ closures

SeeminglyScience · 2018-08-08T20:49:45Z

src/PowerShellEditorServices.Host/CodeLens/ReferencesCodeLensProvider.cs

+                }
+            }
+
+            return acc.ToArray();


echo: SelectMany vs List > Array

Ditto on LINQ

SeeminglyScience · 2018-08-08T20:50:30Z

src/PowerShellEditorServices.Host/CodeLens/ReferencesCodeLensProvider.cs

+                        Range = foundReference.ScriptRegion.ToRange()
+                    });
+                }
+                referenceLocations = acc.ToArray();


echo: SelectMany vs List > Array

Ditto on LINQ

TylerLeonhardt

LGTM! Just a couple questions

TylerLeonhardt · 2018-08-11T19:33:20Z

src/PowerShellEditorServices.Host/CodeLens/PesterCodeLensProvider.cs

-                this.symbolProvider
-                    .ProvideDocumentSymbols(scriptFile);
+            var lenses = new List<CodeLens>();
+            foreach (SymbolReference symbol in _symbolProvider.ProvideDocumentSymbols(scriptFile))


This only calls ProvideDocumentSymbols once and iterates through the result, right?

I think so, yes. Why do you ask?

spoke to @SeeminglyScience about this. We're good

TylerLeonhardt · 2018-08-11T19:54:51Z

src/PowerShellEditorServices.Host/CodeLens/ReferencesCodeLensProvider.cs

-                                symbol.ScriptRegion))
-                    .ToArray();
+            var acc = new List<CodeLens>();
+            foreach (SymbolReference sym in _symbolProvider.ProvideDocumentSymbols(scriptFile))


Same question

TylerLeonhardt · 2018-08-11T19:56:57Z

src/PowerShellEditorServices.Host/CodeLens/ReferencesCodeLensProvider.cs

+            else
+            {
+                var acc = new List<Location>();
+                foreach (SymbolReference foundReference in referencesResult.FoundReferences)


I feel like all these loops could be a little more DRY.

As in each particular loop? Or the fact we are doing loops like this in each call?

They were decidedly pithier as LINQ expressions, but that meant inefficiencies. The problem is that the logic inside each loop is different, so we either write out the loop or use some kind of function pointer or closure.

But I guess to address the DRY need, we should:

Identify what the repetition you're seeing is, and

Work out what the best refactoring is that preserves efficiency

I thought the foreach's were all doing similar things but they're actually quite different :) we're good

TylerLeonhardt · 2018-08-16T22:52:32Z

src/PowerShellEditorServices.Host/CodeLens/CodeLensFeature.cs

-                    .SelectMany(r => r)
-                    .ToArray();
+            return InvokeProviders(provider => provider.ProvideCodeLenses(scriptFile))
+                .SelectMany(codeLens => codeLens)


More LINQ to remove?

Is this the one I put back in?

Yeah, see #719 (comment)

rjmholt · 2018-08-17T22:27:31Z

AppVeyor keeps timing out. We may need to run the tests manually

Robert Holt and others added 4 commits August 7, 2018 17:41

Stop creating new JsonSerializers

dccea93

Reduce allocating operations in CodeLens providers

d9d1d35

Fix style mistake

4d200bd

Fix style mistake

dc09c18

rjmholt requested review from TylerLeonhardt, rkeithhill and SeeminglyScience August 8, 2018 19:48

rjmholt added 2 commits August 8, 2018 12:49

Remove unused class

76d7aad

Use types instead of var

4136b56

rkeithhill reviewed Aug 8, 2018

View reviewed changes

SeeminglyScience approved these changes Aug 8, 2018

View reviewed changes

Use SelectMany in simple non-linq case

4f7230f

TylerLeonhardt approved these changes Aug 11, 2018

View reviewed changes

Fix empty array handling

66d976c

TylerLeonhardt reviewed Aug 16, 2018

View reviewed changes

rjmholt merged commit 06d3c22 into PowerShell:master Aug 24, 2018

rjmholt deleted the improve-codelens branch August 24, 2018 20:28

rjmholt mentioned this pull request Sep 18, 2018

IntelliSense slow or unresponsive forced to kill powershell to force it to work. PowerShell/vscode-powershell#1256

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce allocations in the CodeLens providers #719

Reduce allocations in the CodeLens providers #719

rjmholt commented Aug 8, 2018

rkeithhill Aug 8, 2018

rjmholt Aug 8, 2018

TylerLeonhardt Aug 9, 2018

SeeminglyScience left a comment

SeeminglyScience Aug 8, 2018

rjmholt Aug 8, 2018

SeeminglyScience Aug 9, 2018

rjmholt Aug 9, 2018

rjmholt Aug 9, 2018

SeeminglyScience Aug 8, 2018

rjmholt Aug 8, 2018

SeeminglyScience Aug 8, 2018

rjmholt Aug 8, 2018

SeeminglyScience Aug 8, 2018

rjmholt Aug 8, 2018

TylerLeonhardt left a comment

TylerLeonhardt Aug 11, 2018

rjmholt Aug 12, 2018

TylerLeonhardt Aug 16, 2018

TylerLeonhardt Aug 11, 2018

TylerLeonhardt Aug 11, 2018

rjmholt Aug 12, 2018

TylerLeonhardt Aug 16, 2018

TylerLeonhardt Aug 16, 2018

rjmholt Aug 16, 2018

rjmholt Aug 16, 2018

rjmholt commented Aug 17, 2018

Method	Mean	Error	StdDev	Scaled	ScaledSD	Gen 0	Allocated
ConcatWithSelectMany	36.28 us	0.7209 us	0.8302 us	1.00	0.00	5.4321	16.82 KB
ConcatWithList	38.01 us	0.9687 us	2.8562 us	1.05	0.08	5.6763	17.6 KB

Reduce allocations in the CodeLens providers #719

Reduce allocations in the CodeLens providers #719

Conversation

rjmholt commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeeminglyScience left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerLeonhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmholt commented Aug 17, 2018