From 54fd5f432caac7eb8994b9b8f0dfe4f8da5c9a63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Matou=C5=A1ek?= Date: Sun, 19 Apr 2020 18:15:00 -0700 Subject: [PATCH] Avoid sharing ReferenceManager across script compilations that do not share previous submission. (#43405) --- .../Portable/Compilation/CSharpCompilation.cs | 14 ++++-- .../Symbol/Compilation/CompilationAPITests.cs | 38 +++++++++++++++ .../Compilation/VisualBasicCompilation.vb | 10 +++- .../Compilation/CompilationAPITests.vb | 46 +++++++++++++++---- 4 files changed, 94 insertions(+), 14 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs b/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs index 00e2e696737..e243ab285b4 100644 --- a/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs +++ b/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs @@ -338,6 +338,7 @@ protected override INamespaceSymbol CommonCreateErrorNamespaceSymbol(INamespaceS // // TODO: Consider reusing some results of the assembly binding to improve perf // since most of the binding work is similar. + // https://github.com/dotnet/roslyn/issues/43397 var compilation = new CSharpCompilation( assemblyName, @@ -609,7 +610,13 @@ public CSharpCompilation WithScriptCompilationInfo(CSharpScriptCompilationInfo? return this; } - // Reference binding doesn't depend on previous submission so we can reuse it. + // Metadata references are inherited from the previous submission, + // so we can only reuse the manager if we can guarantee that these references are the same. + // Check if the previous script compilation doesn't change. + + // TODO: Consider comparing the metadata references if they have been bound already. + // https://github.com/dotnet/roslyn/issues/43397 + bool reuseReferenceManager = ReferenceEquals(ScriptCompilationInfo?.PreviousScriptCompilation, info?.PreviousScriptCompilation); return new CSharpCompilation( this.AssemblyName, @@ -618,9 +625,9 @@ public CSharpCompilation WithScriptCompilationInfo(CSharpScriptCompilationInfo? info?.PreviousScriptCompilation, info?.ReturnTypeOpt, info?.GlobalsType, - info != null, + isSubmission: info != null, _referenceManager, - reuseReferenceManager: true, + reuseReferenceManager, syntaxAndDeclarations: _syntaxAndDeclarations); } @@ -912,6 +919,7 @@ public new CSharpCompilation ReplaceSyntaxTree(SyntaxTree oldTree, SyntaxTree? n // TODO(tomat): Consider comparing #r's of the old and the new tree. If they are exactly the same we could still reuse. // This could be a perf win when editing a script file in the IDE. The services create a new compilation every keystroke // that replaces the tree with a new one. + // https://github.com/dotnet/roslyn/issues/43397 var reuseReferenceManager = !oldTree.HasReferenceOrLoadDirectives() && !newTree.HasReferenceOrLoadDirectives(); syntaxAndDeclarations = syntaxAndDeclarations.ReplaceSyntaxTree(oldTree, newTree); diff --git a/src/Compilers/CSharp/Test/Symbol/Compilation/CompilationAPITests.cs b/src/Compilers/CSharp/Test/Symbol/Compilation/CompilationAPITests.cs index 43cf3e7753a..e1bc08661c9 100644 --- a/src/Compilers/CSharp/Test/Symbol/Compilation/CompilationAPITests.cs +++ b/src/Compilers/CSharp/Test/Symbol/Compilation/CompilationAPITests.cs @@ -2076,6 +2076,44 @@ class C { }", options: TestOptions.Script); Assert.False(arc.ReferenceManagerEquals(ars)); } + [Fact] + public void ReferenceManagerReuse_WithScriptCompilationInfo() + { + // Note: The following results would change if we optimized sharing more: https://github.com/dotnet/roslyn/issues/43397 + + var c1 = CSharpCompilation.CreateScriptCompilation("c1"); + Assert.NotNull(c1.ScriptCompilationInfo); + Assert.Null(c1.ScriptCompilationInfo.PreviousScriptCompilation); + + var c2 = c1.WithScriptCompilationInfo(null); + Assert.Null(c2.ScriptCompilationInfo); + Assert.True(c2.ReferenceManagerEquals(c1)); + + var c3 = c2.WithScriptCompilationInfo(new CSharpScriptCompilationInfo(previousCompilationOpt: null, returnType: typeof(int), globalsType: null)); + Assert.NotNull(c3.ScriptCompilationInfo); + Assert.Null(c3.ScriptCompilationInfo.PreviousScriptCompilation); + Assert.True(c3.ReferenceManagerEquals(c2)); + + var c4 = c3.WithScriptCompilationInfo(null); + Assert.Null(c4.ScriptCompilationInfo); + Assert.True(c4.ReferenceManagerEquals(c3)); + + var c5 = c4.WithScriptCompilationInfo(new CSharpScriptCompilationInfo(previousCompilationOpt: c1, returnType: typeof(int), globalsType: null)); + Assert.False(c5.ReferenceManagerEquals(c4)); + + var c6 = c5.WithScriptCompilationInfo(new CSharpScriptCompilationInfo(previousCompilationOpt: c1, returnType: typeof(bool), globalsType: null)); + Assert.True(c6.ReferenceManagerEquals(c5)); + + var c7 = c6.WithScriptCompilationInfo(new CSharpScriptCompilationInfo(previousCompilationOpt: c2, returnType: typeof(bool), globalsType: null)); + Assert.False(c7.ReferenceManagerEquals(c6)); + + var c8 = c7.WithScriptCompilationInfo(new CSharpScriptCompilationInfo(previousCompilationOpt: null, returnType: typeof(bool), globalsType: null)); + Assert.False(c8.ReferenceManagerEquals(c7)); + + var c9 = c8.WithScriptCompilationInfo(null); + Assert.True(c9.ReferenceManagerEquals(c8)); + } + private sealed class EvolvingTestReference : PortableExecutableReference { private readonly IEnumerator _metadataSequence; diff --git a/src/Compilers/VisualBasic/Portable/Compilation/VisualBasicCompilation.vb b/src/Compilers/VisualBasic/Portable/Compilation/VisualBasicCompilation.vb index 6375aafaa95..fac7ef23514 100644 --- a/src/Compilers/VisualBasic/Portable/Compilation/VisualBasicCompilation.vb +++ b/src/Compilers/VisualBasic/Portable/Compilation/VisualBasicCompilation.vb @@ -661,7 +661,13 @@ Namespace Microsoft.CodeAnalysis.VisualBasic Return Me End If - ' Reference binding doesn't depend on previous submission so we can reuse it. + ' Metadata references are inherited from the previous submission, + ' so we can only reuse the manager if we can guarantee that these references are the same. + ' Check if the previous script compilation doesn't change. + + ' TODO Consider comparing the metadata references if they have been bound already. + ' https://github.com/dotnet/roslyn/issues/43397 + Dim reuseReferenceManager = ScriptCompilationInfo?.PreviousScriptCompilation Is info?.PreviousScriptCompilation Return New VisualBasicCompilation( Me.AssemblyName, @@ -677,7 +683,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic info?.GlobalsType, info IsNot Nothing, _referenceManager, - reuseReferenceManager:=True) + reuseReferenceManager) End Function ''' diff --git a/src/Compilers/VisualBasic/Test/Semantic/Compilation/CompilationAPITests.vb b/src/Compilers/VisualBasic/Test/Semantic/Compilation/CompilationAPITests.vb index a93ed6e2571..440f29e54b1 100644 --- a/src/Compilers/VisualBasic/Test/Semantic/Compilation/CompilationAPITests.vb +++ b/src/Compilers/VisualBasic/Test/Semantic/Compilation/CompilationAPITests.vb @@ -2049,15 +2049,6 @@ End Class Assert.False(c1.ReferenceManagerEquals(c2)) End Sub - - Public Sub ReferenceManagerReuse_WithPreviousSubmission() - Dim s1 = VisualBasicCompilation.CreateScriptCompilation("s1") - Dim s2 = VisualBasicCompilation.CreateScriptCompilation("s2") - - Dim s3 = s2.WithScriptCompilationInfo(s2.ScriptCompilationInfo.WithPreviousScriptCompilation(s1)) - Assert.True(s2.ReferenceManagerEquals(s3)) - End Sub - Public Sub ReferenceManagerReuse_WithXmlFileResolver() Dim c1 = VisualBasicCompilation.Create("c", options:=TestOptions.ReleaseDll) @@ -2176,6 +2167,43 @@ End Class Assert.False(arc.ReferenceManagerEquals(ars)) End Sub + + Public Sub ReferenceManagerReuse_WithScriptCompilationInfo() + ' Note The following results would change if we optimized sharing more: https://github.com/dotnet/roslyn/issues/43397 + + Dim c1 = VisualBasicCompilation.CreateScriptCompilation("c1") + Assert.NotNull(c1.ScriptCompilationInfo) + Assert.Null(c1.ScriptCompilationInfo.PreviousScriptCompilation) + + Dim c2 = c1.WithScriptCompilationInfo(Nothing) + Assert.Null(c2.ScriptCompilationInfo) + Assert.True(c2.ReferenceManagerEquals(c1)) + + Dim c3 = c2.WithScriptCompilationInfo(New VisualBasicScriptCompilationInfo(previousCompilationOpt:=Nothing, returnType:=GetType(Integer), globalsType:=Nothing)) + Assert.NotNull(c3.ScriptCompilationInfo) + Assert.Null(c3.ScriptCompilationInfo.PreviousScriptCompilation) + Assert.True(c3.ReferenceManagerEquals(c2)) + + Dim c4 = c3.WithScriptCompilationInfo(Nothing) + Assert.Null(c4.ScriptCompilationInfo) + Assert.True(c4.ReferenceManagerEquals(c3)) + + Dim c5 = c4.WithScriptCompilationInfo(New VisualBasicScriptCompilationInfo(previousCompilationOpt:=c1, returnType:=GetType(Integer), globalsType:=Nothing)) + Assert.False(c5.ReferenceManagerEquals(c4)) + + Dim c6 = c5.WithScriptCompilationInfo(New VisualBasicScriptCompilationInfo(previousCompilationOpt:=c1, returnType:=GetType(Boolean), globalsType:=Nothing)) + Assert.True(c6.ReferenceManagerEquals(c5)) + + Dim c7 = c6.WithScriptCompilationInfo(New VisualBasicScriptCompilationInfo(previousCompilationOpt:=c2, returnType:=GetType(Boolean), globalsType:=Nothing)) + Assert.False(c7.ReferenceManagerEquals(c6)) + + Dim c8 = c7.WithScriptCompilationInfo(New VisualBasicScriptCompilationInfo(previousCompilationOpt:=Nothing, returnType:=GetType(Boolean), globalsType:=Nothing)) + Assert.False(c8.ReferenceManagerEquals(c7)) + + Dim c9 = c8.WithScriptCompilationInfo(Nothing) + Assert.True(c9.ReferenceManagerEquals(c8)) + End Sub + Private Class EvolvingTestReference Inherits PortableExecutableReference -- GitLab