Skip to content

Reduce allocations in the CodeLens providers #719

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 8 commits into from
Aug 24, 2018
Merged

Reduce allocations in the CodeLens providers #719

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
dccea93
Stop creating new JsonSerializers
Aug 8, 2018
d9d1d35
Reduce allocating operations in CodeLens providers
rjmholt Aug 8, 2018
4d200bd
Fix style mistake
rjmholt Aug 8, 2018
dc09c18
Fix style mistake
rjmholt Aug 8, 2018
76d7aad
Remove unused class
rjmholt Aug 8, 2018
4136b56
Use types instead of var
rjmholt Aug 8, 2018
4f7230f
Use SelectMany in simple non-linq case
rjmholt Aug 10, 2018
66d976c
Fix empty array handling
Aug 16, 2018
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
144 changes: 94 additions & 50 deletions src/PowerShellEditorServices.Host/CodeLens/CodeLensFeature.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using Microsoft.PowerShell.EditorServices.Utility;
using Newtonsoft.Json;
using Newtonsoft.Json.Linq;
using System.Collections.Generic;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
Expand All @@ -17,43 +18,43 @@

namespace Microsoft.PowerShell.EditorServices.CodeLenses
{
/// <summary>
/// Implements the CodeLens feature for EditorServices.
/// </summary>
internal class CodeLensFeature :
FeatureComponentBase<ICodeLensProvider>,
ICodeLenses
{
private EditorSession editorSession;

private JsonSerializer jsonSerializer =
JsonSerializer.Create(
Constants.JsonSerializerSettings);

public CodeLensFeature(
EditorSession editorSession,
IMessageHandlers messageHandlers,
ILogger logger)
: base(logger)
{
this.editorSession = editorSession;

messageHandlers.SetRequestHandler(
CodeLensRequest.Type,
this.HandleCodeLensRequest);

messageHandlers.SetRequestHandler(
CodeLensResolveRequest.Type,
this.HandleCodeLensResolveRequest);
}

/// <summary>
/// Create a new CodeLens instance around a given editor session
/// from the component registry.
/// </summary>
/// <param name="components">
/// The component registry to provider other components and to register the CodeLens provider in.
/// </param>
/// <param name="editorSession">The editor session context of the CodeLens provider.</param>
/// <returns>A new CodeLens provider for the given editor session.</returns>
public static CodeLensFeature Create(
IComponentRegistry components,
EditorSession editorSession)
{
var codeLenses =
new CodeLensFeature(
editorSession,
components.Get<IMessageHandlers>(),
JsonSerializer.Create(Constants.JsonSerializerSettings),
components.Get<ILogger>());

var messageHandlers = components.Get<IMessageHandlers>();

messageHandlers.SetRequestHandler(
CodeLensRequest.Type,
codeLenses.HandleCodeLensRequest);

messageHandlers.SetRequestHandler(
CodeLensResolveRequest.Type,
codeLenses.HandleCodeLensResolveRequest);

codeLenses.Providers.Add(
new ReferencesCodeLensProvider(
editorSession));
Expand All @@ -67,42 +68,82 @@ public static CodeLensFeature Create(
return codeLenses;
}

/// <summary>
/// The editor session context to get workspace and language server data from.
/// </summary>
private readonly EditorSession _editorSession;

/// <summary>
/// The json serializer instance for CodeLens object translation.
/// </summary>
private readonly JsonSerializer _jsonSerializer;

/// <summary>
///
/// </summary>
/// <param name="editorSession"></param>
/// <param name="jsonSerializer"></param>
/// <param name="logger"></param>
private CodeLensFeature(
EditorSession editorSession,
JsonSerializer jsonSerializer,
ILogger logger)
: base(logger)
{
_editorSession = editorSession;
_jsonSerializer = jsonSerializer;
}

/// <summary>
/// Get all the CodeLenses for a given script file.
/// </summary>
/// <param name="scriptFile">The PowerShell script file to get CodeLenses for.</param>
/// <returns>All generated CodeLenses for the given script file.</returns>
public CodeLens[] ProvideCodeLenses(ScriptFile scriptFile)
{
return
this.InvokeProviders(p => p.ProvideCodeLenses(scriptFile))
.SelectMany(r => r)
.ToArray();
var acc = new List<CodeLens>();
foreach (CodeLens[] providerResult in InvokeProviders(provider => provider.ProvideCodeLenses(scriptFile)))
{
acc.AddRange(providerResult);
}

return acc.ToArray();
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

/// <summary>
/// Handles a request for CodeLenses from VSCode.
/// </summary>
/// <param name="codeLensParams">Parameters on the CodeLens request that was received.</param>
/// <param name="requestContext"></param>
private async Task HandleCodeLensRequest(
CodeLensRequest codeLensParams,
RequestContext<LanguageServer.CodeLens[]> requestContext)
{
JsonSerializer jsonSerializer =
JsonSerializer.Create(
Constants.JsonSerializerSettings);
ScriptFile scriptFile = _editorSession.Workspace.GetFile(
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

100% agreed on that rule!

Copy link
Member

Choose a reason for hiding this comment

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

Also 100% agree!

codeLensParams.TextDocument.Uri);

var scriptFile =
this.editorSession.Workspace.GetFile(
codeLensParams.TextDocument.Uri);
CodeLens[] codeLensResults = ProvideCodeLenses(scriptFile);

var codeLenses =
this.ProvideCodeLenses(scriptFile)
.Select(
codeLens =>
codeLens.ToProtocolCodeLens(
new CodeLensData
{
Uri = codeLens.File.ClientFilePath,
ProviderId = codeLens.Provider.ProviderId
},
this.jsonSerializer))
.ToArray();

await requestContext.SendResult(codeLenses);
var codeLensResponse = new LanguageServer.CodeLens[codeLensResults.Length];
for (int i = 0; i < codeLensResults.Length; i++)
{
codeLensResponse[i] = codeLensResults[i].ToProtocolCodeLens(
new CodeLensData
{
Uri = codeLensResults[i].File.ClientFilePath,
ProviderId = codeLensResults[i].Provider.ProviderId
},
_jsonSerializer);
}

await requestContext.SendResult(codeLensResponse);
}

/// <summary>
/// Handle a CodeLens resolve request from VSCode.
/// </summary>
/// <param name="codeLens">The CodeLens to be resolved/updated.</param>
/// <param name="requestContext"></param>
private async Task HandleCodeLensResolveRequest(
LanguageServer.CodeLens codeLens,
RequestContext<LanguageServer.CodeLens> requestContext)
Expand All @@ -113,13 +154,13 @@ private async Task HandleCodeLensResolveRequest(
CodeLensData codeLensData = codeLens.Data.ToObject<CodeLensData>();

ICodeLensProvider originalProvider =
this.Providers.FirstOrDefault(
Providers.FirstOrDefault(
provider => provider.ProviderId.Equals(codeLensData.ProviderId));

if (originalProvider != null)
{
ScriptFile scriptFile =
this.editorSession.Workspace.GetFile(
_editorSession.Workspace.GetFile(
codeLensData.Uri);

ScriptRegion region = new ScriptRegion
Expand All @@ -143,7 +184,7 @@ await originalProvider.ResolveCodeLensAsync(

await requestContext.SendResult(
resolvedCodeLens.ToProtocolCodeLens(
this.jsonSerializer));
_jsonSerializer));
}
else
{
Expand All @@ -153,6 +194,9 @@ await requestContext.SendError(
}
}

/// <summary>
/// Represents data expected back in an LSP CodeLens response.
/// </summary>
private class CodeLensData
{
public string Uri { get; set; }
Expand Down
107 changes: 63 additions & 44 deletions src/PowerShellEditorServices.Host/CodeLens/PesterCodeLensProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,71 +15,90 @@ namespace Microsoft.PowerShell.EditorServices.CodeLenses
{
internal class PesterCodeLensProvider : FeatureProviderBase, ICodeLensProvider
{
private static char[] QuoteChars = new char[] { '\'', '"'};
/// <summary>
/// The editor session context to provide CodeLenses for.
/// </summary>
private EditorSession _editorSession;

private EditorSession editorSession;
private IDocumentSymbolProvider symbolProvider;
/// <summary>
/// The symbol provider to get symbols from to build code lenses with.
/// </summary>
private IDocumentSymbolProvider _symbolProvider;

/// <summary>
/// Create a new Pester CodeLens provider for a given editor session.
/// </summary>
/// <param name="editorSession">The editor session context for which to provide Pester CodeLenses.</param>
public PesterCodeLensProvider(EditorSession editorSession)
{
this.editorSession = editorSession;
this.symbolProvider = new PesterDocumentSymbolProvider();
_editorSession = editorSession;
_symbolProvider = new PesterDocumentSymbolProvider();
}

private IEnumerable<CodeLens> GetPesterLens(
/// <summary>
/// Get the Pester CodeLenses for a given Pester symbol.
/// </summary>
/// <param name="pesterSymbol">The Pester symbol to get CodeLenses for.</param>
/// <param name="scriptFile">The script file the Pester symbol comes from.</param>
/// <returns>All CodeLenses for the given Pester symbol.</returns>
private CodeLens[] GetPesterLens(
PesterSymbolReference pesterSymbol,
ScriptFile scriptFile)
{
var clientCommands = new ClientCommand[]
var codeLensResults = new CodeLens[]
{
new ClientCommand(
"PowerShell.RunPesterTests",
"Run tests",
new object[]
{
scriptFile.ClientFilePath,
false, // Don't debug
pesterSymbol.TestName,
}),
new CodeLens(
this,
scriptFile,
pesterSymbol.ScriptRegion,
new ClientCommand(
"PowerShell.RunPesterTests",
"Run tests",
new object[] { scriptFile.ClientFilePath, false /* No debug */, pesterSymbol.TestName })),

new ClientCommand(
"PowerShell.RunPesterTests",
"Debug tests",
new object[]
{
scriptFile.ClientFilePath,
true, // Run in debugger
pesterSymbol.TestName,
}),
new CodeLens(
this,
scriptFile,
pesterSymbol.ScriptRegion,
new ClientCommand(
"PowerShell.RunPesterTests",
"Debug tests",
new object[] { scriptFile.ClientFilePath, true /* Run in debugger */, pesterSymbol.TestName })),
};

return
clientCommands.Select(
command =>
new CodeLens(
this,
scriptFile,
pesterSymbol.ScriptRegion,
command));
return codeLensResults;
}

/// <summary>
/// Get all Pester CodeLenses for a given script file.
/// </summary>
/// <param name="scriptFile">The script file to get Pester CodeLenses for.</param>
/// <returns>All Pester CodeLenses for the given script file.</returns>
public CodeLens[] ProvideCodeLenses(ScriptFile scriptFile)
{
var symbols =
this.symbolProvider
.ProvideDocumentSymbols(scriptFile);
var lenses = new List<CodeLens>();
foreach (SymbolReference symbol in _symbolProvider.ProvideDocumentSymbols(scriptFile))
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so, yes. Why do you ask?

Copy link
Member

Choose a reason for hiding this comment

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

spoke to @SeeminglyScience about this. We're good

{
if (symbol is PesterSymbolReference pesterSymbol)
{
if (pesterSymbol.Command != PesterCommandType.Describe)
{
continue;
}

var lenses =
symbols
.OfType<PesterSymbolReference>()
.Where(s => s.Command == PesterCommandType.Describe)
.SelectMany(s => this.GetPesterLens(s, scriptFile))
.Where(codeLens => codeLens != null)
.ToArray();
lenses.AddRange(GetPesterLens(pesterSymbol, scriptFile));
}
}

return lenses;
return lenses.ToArray();
Copy link
Collaborator

Choose a reason for hiding this comment

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

echo: SelectMany vs List > Array

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

/// <summary>
/// Resolve the CodeLens provision asynchronously -- just wraps the CodeLens argument in a task.
/// </summary>
/// <param name="codeLens">The code lens to resolve.</param>
/// <param name="cancellationToken"></param>
/// <returns>The given CodeLens, wrapped in a task.</returns>
public Task<CodeLens> ResolveCodeLensAsync(
CodeLens codeLens,
CancellationToken cancellationToken)
Expand Down
Loading