Skip to content

Commit 0b8b1b6

Browse files
jonsequiturjonsequitur
authored
Completion perf fixes (dotnet#1571)
* fix metadata caching in CSharpKernel * test refactoring for script and assembly refs * test fixes * address PR comments * move test to more appropriate class * test fixes
1 parent aa269ba commit 0b8b1b6

File tree

9 files changed

+327
-214
lines changed

9 files changed

+327
-214
lines changed

‎src/Microsoft.DotNet.Interactive.CSharp/CSharpKernel.cs

+16-8
Original file line numberDiff line numberDiff line change
@@ -313,7 +313,6 @@ await RunAsync(
313313

314314
public Task HandleAsync(ChangeWorkingDirectory command, KernelInvocationContext context)
315315
{
316-
_workingDirectory = command.WorkingDirectory;
317316
return Task.CompletedTask;
318317
}
319318

@@ -322,20 +321,15 @@ private async Task RunAsync(
322321
CancellationToken cancellationToken = default,
323322
Func<Exception, bool> catchException = default)
324323
{
325-
if (_workingDirectory == null)
326-
_workingDirectory = Directory.GetCurrentDirectory();
327-
328-
ScriptOptions = ScriptOptions
329-
.WithMetadataResolver(CachingMetadataResolver.Default.WithBaseDirectory(_workingDirectory))
330-
.WithSourceResolver(new SourceFileResolver(ImmutableArray<string>.Empty, _workingDirectory));
324+
UpdateScriptOptionsIfWorkingDirectoryChanged();
331325

332326
if (ScriptState is null)
333327
{
334328
ScriptState = await CSharpScript.RunAsync(
335329
code,
336330
ScriptOptions,
337331
cancellationToken: cancellationToken)
338-
.UntilCancelled(cancellationToken) ?? ScriptState;
332+
.UntilCancelled(cancellationToken) ?? ScriptState;
339333
}
340334
else
341335
{
@@ -356,6 +350,20 @@ private async Task RunAsync(
356350
{
357351
_workspace.UpdateWorkspace(ScriptState);
358352
}
353+
354+
void UpdateScriptOptionsIfWorkingDirectoryChanged()
355+
{
356+
var currentDir = Directory.GetCurrentDirectory();
357+
358+
if (!currentDir.Equals(_workingDirectory, StringComparison.Ordinal))
359+
{
360+
_workingDirectory = currentDir;
361+
362+
ScriptOptions = ScriptOptions
363+
.WithMetadataResolver(CachingMetadataResolver.Default.WithBaseDirectory(_workingDirectory))
364+
.WithSourceResolver(new SourceFileResolver(ImmutableArray<string>.Empty, _workingDirectory));
365+
}
366+
}
359367
}
360368

361369
public async Task HandleAsync(

‎src/Microsoft.DotNet.Interactive.CSharp/CachingMetadataResolver.cs

+27-23
Original file line numberDiff line numberDiff line change
@@ -13,34 +13,43 @@ namespace Microsoft.DotNet.Interactive.CSharp
1313
{
1414
internal sealed class CachingMetadataResolver : MetadataReferenceResolver, IEquatable<ScriptMetadataResolver>
1515
{
16+
private readonly ConcurrentDictionary<AssemblyIdentity, PortableExecutableReference> _resolvedAssembliesCache = new();
17+
private readonly ConcurrentDictionary<(string, string, MetadataReferenceProperties), ImmutableArray<PortableExecutableReference>> _xmlReferencesCache = new();
18+
1619
private readonly ScriptMetadataResolver _resolver;
17-
public static CachingMetadataResolver Default { get; } = new CachingMetadataResolver(ImmutableArray<string>.Empty, null);
20+
private readonly string _baseDirectory;
21+
22+
public static CachingMetadataResolver Default { get; } = new(ImmutableArray<string>.Empty, null);
1823

19-
private CachingMetadataResolver(ImmutableArray<string> searchPaths, string baseDirectoryOpt)
24+
private CachingMetadataResolver(ImmutableArray<string> searchPaths, string baseDirectory)
2025
{
26+
_baseDirectory = baseDirectory;
27+
2128
_resolver = ScriptMetadataResolver.Default
22-
.WithBaseDirectory(baseDirectoryOpt)
23-
.WithSearchPaths(searchPaths);
29+
.WithBaseDirectory(baseDirectory)
30+
.WithSearchPaths(searchPaths);
2431
}
2532

26-
public override ImmutableArray<PortableExecutableReference> ResolveReference(string reference, string baseFilePath, MetadataReferenceProperties properties)
27-
{
28-
var resolvedReferences = _resolver.ResolveReference(reference, baseFilePath, properties);
29-
var xmlResolvedReferences = resolvedReferences.Select(r => ResolveReferenceWithXmlDocumentationProvider(r.FilePath, properties)).ToImmutableArray();
30-
return xmlResolvedReferences;
31-
}
33+
public override ImmutableArray<PortableExecutableReference> ResolveReference(
34+
string reference,
35+
string baseFilePath,
36+
MetadataReferenceProperties properties) =>
37+
_xmlReferencesCache.GetOrAdd((reference, baseFilePath, properties), t =>
38+
{
39+
var resolvedReferences = _resolver.ResolveReference(t.Item1, t.Item2, t.Item3);
40+
var xmlResolvedReferences = resolvedReferences.Select(r => ResolveReferenceWithXmlDocumentationProvider(r.FilePath, properties)).ToImmutableArray();
41+
42+
return xmlResolvedReferences;
43+
});
3244

3345
public override bool ResolveMissingAssemblies => _resolver.ResolveMissingAssemblies;
3446

35-
private readonly ConcurrentDictionary<AssemblyIdentity, PortableExecutableReference> _resolvedAssembliesCache = new ConcurrentDictionary<AssemblyIdentity, PortableExecutableReference>();
36-
public override PortableExecutableReference ResolveMissingAssembly(MetadataReference definition, AssemblyIdentity referenceIdentity)
37-
{
38-
return _resolvedAssembliesCache.GetOrAdd(referenceIdentity, id => _resolver.ResolveMissingAssembly(definition, id));
39-
}
47+
public override PortableExecutableReference ResolveMissingAssembly(MetadataReference definition, AssemblyIdentity referenceIdentity) =>
48+
_resolvedAssembliesCache.GetOrAdd(referenceIdentity, id => _resolver.ResolveMissingAssembly(definition, id));
4049

4150
public CachingMetadataResolver WithBaseDirectory(string baseDirectory)
4251
{
43-
if (BaseDirectory == baseDirectory)
52+
if (_baseDirectory == baseDirectory)
4453
{
4554
return this;
4655
}
@@ -50,13 +59,8 @@ public CachingMetadataResolver WithBaseDirectory(string baseDirectory)
5059

5160
public ImmutableArray<string> SearchPaths => _resolver.SearchPaths;
5261

53-
public string BaseDirectory => _resolver.BaseDirectory;
54-
55-
internal static PortableExecutableReference ResolveReferenceWithXmlDocumentationProvider(string path, MetadataReferenceProperties properties = default(MetadataReferenceProperties))
56-
{
57-
var peReference = MetadataReference.CreateFromFile(path, properties, XmlDocumentationProvider.CreateFromFile(Path.ChangeExtension(path, ".xml")));
58-
return peReference;
59-
}
62+
internal static PortableExecutableReference ResolveReferenceWithXmlDocumentationProvider(string path, MetadataReferenceProperties properties = default) =>
63+
MetadataReference.CreateFromFile(path, properties, XmlDocumentationProvider.CreateFromFile(Path.ChangeExtension(path, ".xml")));
6064

6165
public bool Equals(ScriptMetadataResolver other) => _resolver.Equals(other);
6266

‎src/Microsoft.DotNet.Interactive.CSharp/InteractiveWorkspace.cs

+3-5
Original file line numberDiff line numberDiff line change
@@ -59,11 +59,10 @@ private static string ResolveRefAssemblyPath()
5959
{
6060
var latestRuntimeDirAndVersion =
6161
Directory.GetDirectories(appRefDir)
62-
.Select(dir => Path.GetFileName(dir))
62+
.Select(Path.GetFileName)
6363
.Select(dir => new { Directory = dir, Version = Version.TryParse(dir, out var version) ? version : new Version() })
6464
.OrderBy(dirPair => dirPair.Version)
65-
.Where(dirPair => dirPair.Version <= runtimeVersion)
66-
.LastOrDefault();
65+
.LastOrDefault(dirPair => dirPair.Version <= runtimeVersion);
6766
if (latestRuntimeDirAndVersion is { })
6867
{
6968
var refVersion = latestRuntimeDirAndVersion.Directory; // e.g., `5.0.0`
@@ -86,7 +85,7 @@ private static IReadOnlyCollection<MetadataReference> ResolveRefAssemblies()
8685
var resolved = CachingMetadataResolver.ResolveReferenceWithXmlDocumentationProvider(assemblyRef, MetadataReferenceProperties.Assembly);
8786
assemblyRefs.Add(resolved);
8887
}
89-
catch (Exception ex) when (ex is ArgumentNullException || ex is ArgumentException || ex is IOException)
88+
catch (Exception ex) when (ex is ArgumentNullException or ArgumentException or IOException)
9089
{
9190
// the only exceptions that can be thrown by `ResolveReferenceWithXmlDocumentationProvider` which
9291
// internally calls `XmlDocumentationProvider.CreateFromFile`
@@ -263,7 +262,6 @@ public Document ForkDocumentForLanguageServices(string code)
263262
var workingDocument = solution.GetDocument(workingDocumentId);
264263

265264
return workingDocument;
266-
267265
}
268266

269267
public void AddPackageManagerReference(MetadataReference reference)

‎src/Microsoft.DotNet.Interactive.Tests/LanguageKernelAssemblyReferenceTests.cs

+169
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,169 @@
1+
// Copyright (c) .NET Foundation and contributors. All rights reserved.
2+
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
3+
4+
using System;
5+
using System.IO;
6+
using System.Reflection;
7+
using FluentAssertions;
8+
using System.Threading.Tasks;
9+
using Microsoft.CodeAnalysis;
10+
using Microsoft.CodeAnalysis.CSharp;
11+
using Microsoft.DotNet.Interactive.Commands;
12+
using Microsoft.DotNet.Interactive.Events;
13+
using Microsoft.DotNet.Interactive.Tests.Utility;
14+
using Xunit;
15+
using Xunit.Abstractions;
16+
17+
namespace Microsoft.DotNet.Interactive.Tests
18+
{
19+
#pragma warning disable 8509
20+
public class LanguageKernelAssemblyReferenceTests : LanguageKernelTestBase
21+
{
22+
public LanguageKernelAssemblyReferenceTests(ITestOutputHelper output) : base(output)
23+
{
24+
}
25+
26+
[Theory]
27+
[InlineData(Language.CSharp)]
28+
[InlineData(Language.FSharp)]
29+
public async Task it_can_load_assembly_references_using_r_directive_single_submission(Language language)
30+
{
31+
var kernel = CreateKernel(language);
32+
33+
// F# strings treat \ as an escape character. So do C# strings, except #r in C# is special, and doesn't. F# usually uses @ strings for paths @"c:\temp\...."
34+
var dllPath = CreateDllInCurrentDirectory();
35+
36+
var source = language switch
37+
{
38+
Language.FSharp => $@"#r @""{dllPath.FullName}""
39+
typeof<Hello>.Name",
40+
41+
Language.CSharp => $@"#r ""{dllPath.FullName}""
42+
typeof(Hello).Name"
43+
};
44+
45+
await SubmitCode(kernel, source);
46+
47+
KernelEvents.Should().NotContainErrors();
48+
49+
KernelEvents
50+
.Should()
51+
.ContainSingle<ReturnValueProduced>()
52+
.Which
53+
.Value
54+
.Should()
55+
.Be("Hello");
56+
}
57+
58+
[Theory]
59+
[InlineData(Language.CSharp)]
60+
[InlineData(Language.FSharp)]
61+
public async Task it_can_load_assembly_references_using_r_directive_separate_submissions(Language language)
62+
{
63+
var kernel = CreateKernel(language);
64+
65+
// F# strings treat \ as an escape character. So do C# strings, except #r in C# is special, and doesn't. F# usually uses @ strings for paths @"c:\temp\...."
66+
var dllPath = CreateDllInCurrentDirectory();
67+
68+
var source = language switch
69+
{
70+
Language.FSharp => new[]
71+
{
72+
$"#r @\"{dllPath.FullName}\"",
73+
"typeof<Hello>.Name"
74+
},
75+
76+
Language.CSharp => new[]
77+
{
78+
$"#r \"{dllPath.FullName}\"",
79+
"typeof(Hello).Name"
80+
}
81+
};
82+
83+
await SubmitCode(kernel, source);
84+
85+
KernelEvents.Should().NotContainErrors();
86+
87+
KernelEvents
88+
.Should()
89+
.ContainSingle<ReturnValueProduced>()
90+
.Which
91+
.Value
92+
.Should()
93+
.Be("Hello");
94+
}
95+
96+
[Theory]
97+
[InlineData(Language.CSharp)]
98+
[InlineData(Language.FSharp)]
99+
public async Task it_can_load_assembly_references_using_r_directive_with_relative_path(Language language)
100+
{
101+
var kernel = CreateKernel(language);
102+
103+
var dllName = CreateDllInCurrentDirectory();
104+
105+
var code = language switch
106+
{
107+
Language.CSharp => $"#r \"{dllName.Name}\"",
108+
Language.FSharp => $"#r \"{dllName.Name}\""
109+
};
110+
111+
var command = new SubmitCode(code);
112+
113+
await kernel.SendAsync(command);
114+
115+
KernelEvents.Should().NotContainErrors();
116+
117+
KernelEvents.Should()
118+
.ContainSingle<CommandSucceeded>(c => c.Command == command);
119+
}
120+
121+
[Theory]
122+
[InlineData(Language.CSharp)]
123+
// [InlineData(Language.FSharp)] Not supported in F#
124+
public async Task it_can_load_assembly_references_using_r_directive_with_relative_path_after_user_code_changes_current_directory(Language language)
125+
{
126+
var currentDirectory = Directory.GetCurrentDirectory();
127+
DisposeAfterTest(() => Directory.SetCurrentDirectory(currentDirectory));
128+
129+
var kernel = CreateKernel(language);
130+
131+
//Even when a user changes the current directory, loading from a relative path is not affected.
132+
await kernel.SendAsync(new SubmitCode("System.IO.Directory.SetCurrentDirectory(\"..\")"));
133+
134+
var dllName = CreateDllInCurrentDirectory();
135+
136+
var code = language switch
137+
{
138+
Language.CSharp => $"#r \"{dllName.Name}\"\nnew Hello()",
139+
Language.FSharp => $"#r \"{dllName.Name}\"\nnew Hello()"
140+
};
141+
142+
var command = new SubmitCode(code);
143+
144+
await kernel.SendAsync(command);
145+
146+
KernelEvents.Should().NotContainErrors();
147+
148+
KernelEvents.Should()
149+
.ContainSingle<CommandSucceeded>(c => c.Command == command);
150+
}
151+
152+
private FileInfo CreateDllInCurrentDirectory()
153+
{
154+
var assemblyName = Guid.NewGuid().ToString("N");
155+
var dllName = assemblyName + ".dll";
156+
157+
var systemRefLocation = typeof(object).GetTypeInfo().Assembly.Location;
158+
var systemReference = MetadataReference.CreateFromFile(systemRefLocation);
159+
160+
CSharpCompilation.Create(assemblyName)
161+
.WithOptions(new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary))
162+
.AddReferences(systemReference)
163+
.AddSyntaxTrees(CSharpSyntaxTree.ParseText("public class Hello { }"))
164+
.Emit(dllName);
165+
166+
return new FileInfo(Path.Combine(Directory.GetCurrentDirectory(), dllName));
167+
}
168+
}
169+
}

0 commit comments

Comments
 (0)