Reference assemblies reachable from current module only by cston · Pull Request #25955 · dotnet/roslyn

cston · 2018-04-05T14:43:41Z

Customer scenario

Evaluating expressions in the Locals, Watch, or Callstack windows is slow when the process contains a large number of modules and not all modules are referenced by the current module.

Bugs this fixes

591114

Workarounds, if any

None

Risk

Medium risk. The user must opt-in to the fix via a registry setting, but there are some changes required to maintain the existing behavior alongside the fix.

Performance impact

No performance impact unless opted-in.

Is this a regression from a previous update?

No.

How was the bug found?

Customer reported.

cston · 2018-04-05T15:54:46Z

@tmat, @ivanbasov please review.
cc @jinujoseph

cston · 2018-04-05T17:12:56Z

src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/CSharpExpressionCompiler.cs

+            {
+                previous = null;
+            }
+            if (previous != null && previous.ModuleVersionId != moduleVersionId)


If the fix is disabled (if kind == AllAssemblies), we should not be setting or checking previous.ModuleVersionId. #Resolved

cston · 2018-04-05T17:14:05Z

src/ExpressionEvaluator/Core/Source/ExpressionCompiler/EEMetadataReferenceResolver.cs

+#endif
+            MetadataReference reference;
+            _referencesByIdentity.TryGetValue(referenceIdentity, out reference);
+            return (PortableExecutableReference)reference;


Should return highest version of assembly if exact version is missing and there is a higher version available. #Resolved

ivanbasov · 2018-04-09T18:29:02Z

src/ExpressionEvaluator/Core/Source/ExpressionCompiler/MetadataUtilities.cs

+            {
+                Debug.Assert(referencesBuilder.Count == 0);
+                // CommonReferenceManager<TCompilation, TAssemblySymbol>.Bind()
+                // expects COR library to be included in the explicit assemblies (see bug #...).


#... [](start = 90, length = 4)

Would you like to specify the bug #? #Closed

Removed see bug #.

ivanbasov · 2018-04-09T18:45:04Z

src/ExpressionEvaluator/CSharp/Test/ExpressionCompiler/ExpressionCompilerTestBase.cs

+            out int methodOrTypeToken,
+            out int localSignatureToken,
+            Guid moduleVersionIdOpt = default,
+            MakeAssemblyReferencesKind kind = MakeAssemblyReferencesKind.AllAssemblies) // TODO: Revert this change. No need for MakeAssemblyReferencesKind here.


TODO: Revert this change. [](start = 91, length = 25)

Is it for now or a bug should be created for this TODO? #Closed

ivanbasov · 2018-04-09T18:46:01Z

src/ExpressionEvaluator/CSharp/Test/ExpressionCompiler/ReferencedModulesTests.cs

+}");
+                VerifyResolutionRequests(context, (identityA1, identityA1, 1));
+
+                // TODO: Test { A1.M, B1.M, C.M } with all assemblies.


TODO [](start = 19, length = 4)

Should this test be added now? #Closed

ivanbasov · 2018-04-09T18:46:39Z

src/ExpressionEvaluator/CSharp/Test/ExpressionCompiler/ReferencedModulesTests.cs

+
+        private static void VerifyAppDomainMetadataContext(AppDomain appDomain)
+        {
+            // TODO: Verify set of AssemblyMetadataContext instances.


TODO [](start = 15, length = 4)

? #Closed

ivanbasov

cston · 2018-04-10T19:30:49Z

cc @jaredpar

jaredpar · 2018-04-10T23:27:01Z

src/ExpressionEvaluator/Core/Source/ExpressionCompiler/ExpressionEvaluatorFatalError.cs

+        internal static bool GetBoolRegistryValue(string name)
+        {
+            var value = RegistryHelpers.GetRegistryValue(name);
+            if ((value != null) && (value is int))


if ((value != null) && (value is int)) [](start = 12, length = 38)

Isn't the null check redundant here? #Resolved

cston · 2018-04-13T16:12:36Z

@tmat, please review.
@ivanbasov, please take a second look since there are a number of changes (caching in particular) since your review, thanks.

ivanbasov · 2018-04-13T19:42:25Z

reviewed once again. LGTM. Thanks!

tmat · 2018-04-13T20:41:24Z

src/ExpressionEvaluator/Core/Source/ExpressionCompiler/EEMetadataReferenceResolver.cs

+        internal readonly Dictionary<AssemblyIdentity, (AssemblyIdentity Identity, int Count)> Requests = new Dictionary<AssemblyIdentity, (AssemblyIdentity Identity, int Count)>();
+#endif
+
+        internal EEMetadataReferenceResolver(AssemblyIdentityComparer identityComparer, Dictionary<string, ImmutableArray<(AssemblyIdentity, MetadataReference)>> referencesByIdentity)


Dictionary [](start = 88, length = 10)

IReadOnlyDictionary #Resolved

tmat · 2018-04-13T20:42:28Z

src/ExpressionEvaluator/Core/Source/ExpressionCompiler/EEMetadataReferenceResolver.cs

+        {
+            ImmutableArray<(AssemblyIdentity, MetadataReference)> references;
+            (AssemblyIdentity, MetadataReference) result = default;
+            if (_referencesByIdentity.TryGetValue(referenceIdentity.Name, out references))


references [](start = 78, length = 10)

nit: out var references #Resolved

tmat · 2018-04-13T20:43:11Z

src/ExpressionEvaluator/Core/Source/ExpressionCompiler/EEMetadataReferenceResolver.cs

+        public override PortableExecutableReference ResolveMissingAssembly(MetadataReference definition, AssemblyIdentity referenceIdentity)
+        {
+            ImmutableArray<(AssemblyIdentity, MetadataReference)> references;
+            (AssemblyIdentity, MetadataReference) result = default;


AssemblyIdentity, MetadataReference [](start = 13, length = 35)

Perhaps we could name these so that we don't need to wonder what Item1 and Item2 are below #Resolved

tmat · 2018-04-13T20:44:40Z

src/ExpressionEvaluator/Core/Source/ExpressionCompiler/EEMetadataReferenceResolver.cs

+            }
+#if DEBUG
+            (AssemblyIdentity Identity, int Count) request;
+            if (!Requests.TryGetValue(referenceIdentity, out request))


out r [](start = 57, length = 5)

out var? #Resolved

tmat · 2018-04-13T20:45:42Z

src/ExpressionEvaluator/Core/Source/ExpressionCompiler/EEMetadataReferenceResolver.cs

+            throw ExceptionUtilities.Unreachable;
+        }
+
+        private (AssemblyIdentity, MetadataReference) GetBestMatch(ImmutableArray<(AssemblyIdentity, MetadataReference)> references, AssemblyIdentity referenceIdentity)


(AssemblyIdentity, MetadataReference) [](start = 82, length = 37)

Perhaps we could name these so that we don't need to wonder what Item1 and Item2 are below #Resolved

tmat · 2018-04-13T20:58:57Z

src/ExpressionEvaluator/Core/Source/ExpressionCompiler/InstructionDecoder.cs

+
+        internal InstructionDecoder()
+        {
+            _useReferencedAssembliesOnly = ExpressionCompiler.GetUseReferencedAssembliesOnlySetting();


ExpressionCompiler.GetUseReferencedAssembliesOnlySetting(); [](start = 43, length = 59)

Shouldn't this rather flow from the ExpressionCompiler instance?

Yes, ideally. Added a comment for now.

In reply to: 181508668 [](ancestors = 181508668)

tmat · 2018-04-13T20:59:28Z

src/ExpressionEvaluator/Core/Source/ExpressionCompiler/MetadataContext.cs

+    // Wrapper around Guid to ensure callers have asked for the correct id
+    // rather than simply using the ModuleVersionId (which is unnecessary
+    // when the Compilation references all loaded assemblies).
+    internal struct MetadataContextId : IEquatable<MetadataContextId>


s [](start = 12, length = 2)

readonly struct #Resolved

tmat · 2018-04-13T20:59:40Z

src/ExpressionEvaluator/Core/Source/ExpressionCompiler/MetadataContext.cs

+        }
+    }
+
+    internal struct MetadataContext<TAssemblyContext>


[](start = 12, length = 1)

readonly struct #Resolved

tmat · 2018-04-13T21:00:02Z

src/ExpressionEvaluator/Core/Source/ExpressionCompiler/MetadataContext.cs

+                    throw ExceptionUtilities.UnexpectedValue(kind);
+            }
+        }
+    }


Move to a separate file #Resolved

tmat · 2018-04-13T21:05:23Z

src/ExpressionEvaluator/Core/Source/ExpressionCompiler/MetadataUtilities.cs

+        AllAssemblies,
+        AllReferences,
+        DirectReferencesOnly,
+    }


Move to separate file #Resolved

tmat · 2018-04-13T21:07:39Z

src/ExpressionEvaluator/Core/Source/ExpressionCompiler/MetadataUtilities.cs


+            // We don't walk the references of winmd assemblies currently. (That would require
+            // walking references from the current module and also the winmd assemblies.)
+            // So if there are any winmd assemblies, we'll use all assemblies.


So if there are any winmd assemblies, we'll use all assemblies. [](start = 15, length = 63)

Should we have an issue for this? We might want to address this in 15.8

Logged #26157.

In reply to: 181510450 [](ancestors = 181510450)

tmat · 2018-04-13T21:12:58Z

src/ExpressionEvaluator/Core/Source/ExpressionCompiler/MetadataUtilities.cs

            var referencesBuilder = ArrayBuilder<MetadataReference>.GetInstance();
-            var identitiesBuilder = (identityComparer == null) ? null : ArrayBuilder<AssemblyIdentity>.GetInstance();
+            var identitiesBuilder = (kind == MakeAssemblyReferencesKind.DirectReferencesOnly) ? ArrayBuilder<AssemblyIdentity>.GetInstance() : null;
+            referencesByIdentity = (kind == MakeAssemblyReferencesKind.AllReferences) ? new Dictionary<string, ImmutableArray<(AssemblyIdentity, MetadataReference)>>(StringComparer.OrdinalIgnoreCase) : null;


referencesByIdentity [](start = 12, length = 20)

better name: referencesBySimpleName #Resolved

tmat · 2018-04-13T21:13:38Z

src/ExpressionEvaluator/Core/Source/ExpressionCompiler/MetadataUtilities.cs

+                    var identity = reader.ReadAssemblyIdentityOrThrow();
+                    if (referencesByIdentity != null)
+                    {
+                        string identityKey = identity.Name;


I'd inline this variable #Resolved

I think this would read better: referencesBySimpleName.TryGetValue(identity.Name, out references) #Resolved

tmat · 2018-04-13T21:13:50Z

src/ExpressionEvaluator/Core/Source/ExpressionCompiler/MetadataUtilities.cs

+                    {
+                        string identityKey = identity.Name;
+                        ImmutableArray<(AssemblyIdentity, MetadataReference)> references;
+                        if (!referencesByIdentity.TryGetValue(identityKey, out references))


out [](start = 75, length = 3)

out var #Resolved

tmat · 2018-04-13T21:14:53Z

src/ExpressionEvaluator/Core/Source/ExpressionCompiler/MetadataUtilities.cs

+                {
+                    var reader = metadata.MetadataReader;
+                    var identity = reader.ReadAssemblyIdentityOrThrow();
+                    if (referencesByIdentity != null)


(referencesByIdentity != null [](start = 23, length = 29)

Isn't this always true? #Resolved

tmat · 2018-04-13T21:24:34Z

    private static PortableExecutableReference MakeAssemblyMetadata(ModuleMetadata metadata, Dictionary<string, ModuleMetadata> modulesByName)

Rename to MakeAssemblyReference? #Resolved

Refers to: src/ExpressionEvaluator/Core/Source/ExpressionCompiler/MetadataUtilities.cs:315 in 54ecd0f. [](commit_id = 54ecd0f, deletion_comment = False)

tmat · 2018-04-13T22:07:35Z

src/ExpressionEvaluator/Core/Source/ExpressionCompiler/MetadataUtilities.cs

+                {
+                    referencesBuilder.Add(targetReference);
+                }
            }


There seems to be 2 code paths here that are interwoven but almost entirely distinct. Kind == AllReferences and otherwise. Consider refactoring to two different methods or blocks. #Resolved

tmat · 2018-04-13T22:37:44Z

src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/CompilationExtensions.cs

+        internal static CSharpCompilation ToCompilation(this ImmutableArray<MetadataBlock> metadataBlocks, Guid moduleVersionId, MakeAssemblyReferencesKind kind)
        {
+            Dictionary<string, ImmutableArray<(AssemblyIdentity, MetadataReference)>> referencesByIdentity;
+            var references = metadataBlocks.MakeAssemblyReferences(moduleVersionId, IdentityComparer, kind, out referencesByIdentity);


out [](start = 108, length = 4)

out var to avoid declaring the var with the very long type name #Resolved

tmat · 2018-04-13T22:39:11Z

src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/CompilationExtensions.cs

+        internal static CSharpCompilation ToCompilation(this ImmutableArray<MetadataBlock> metadataBlocks, Guid moduleVersionId, MakeAssemblyReferencesKind kind)
        {
+            Dictionary<string, ImmutableArray<(AssemblyIdentity, MetadataReference)>> referencesByIdentity;
+            var references = metadataBlocks.MakeAssemblyReferences(moduleVersionId, IdentityComparer, kind, out referencesByIdentity);


referencesByIdentity [](start = 112, length = 20)

referencesBySimpleName #Resolved

tmat · 2018-04-13T22:51:04Z

LGTM. Just some minor feedback.

cston · 2018-04-14T04:30:06Z

@MattGertz for 15.7 approval.
cc @jinujoseph

jinujoseph · 2018-04-18T22:08:43Z

approved to merge via link for 15.7.Preview5

jinujoseph · 2018-04-18T22:09:11Z

vendor test approval pending @richaverma1

cston · 2018-04-19T20:02:36Z

@dotnet-bot test windows_debug_vs-integration_prtest

cston added 2 commits April 5, 2018 07:38

Reference assemblies reachable from current module only

cea199b

Opt-in with registry key

45891b7

cston commented Apr 5, 2018

View reviewed changes

cston added 5 commits April 5, 2018 13:52

Allow multiple versions of an assembly

3a809fd

Misc.

d3135ac

Make parameters non-optional

f72b27b

Misc.

75bab1a

Cache Compilations by target Assembly

22f16e4

jcouv added the Area-Compilers label Apr 7, 2018

jcouv added this to the 15.7 milestone Apr 7, 2018

ivanbasov reviewed Apr 9, 2018

View reviewed changes

ivanbasov approved these changes Apr 9, 2018

View reviewed changes

Move cache re-use to ExpressionCompiler

8c515c5

Misc.

0ea29d0

jaredpar reviewed Apr 10, 2018

View reviewed changes

cston added 2 commits April 11, 2018 17:05

PR feedback

4b6819e

Add VB tests

54ecd0f

tmat reviewed Apr 13, 2018

View reviewed changes

PR feedback

abfc840

jinujoseph approved these changes Apr 15, 2018

View reviewed changes

jinujoseph added Needs Shiproom Approval Interactive-Debugging Approved to merge and removed Needs Shiproom Approval labels Apr 15, 2018

Add back EvaluationContext.CreateMethodContext overload

b3a640c

cston merged commit 7bebbc7 into dotnet:dev15.7.x Apr 23, 2018

cston deleted the refs-only branch April 23, 2018 22:54

Conversation

cston commented Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

How was the bug found?

Uh oh!

cston commented Apr 5, 2018

Uh oh!

cston Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivanbasov Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Apr 10, 2018

Choose a reason for hiding this comment

Uh oh!

ivanbasov Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivanbasov Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivanbasov Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivanbasov left a comment

Choose a reason for hiding this comment

Uh oh!

cston commented Apr 10, 2018

Uh oh!

jaredpar Apr 10, 2018 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston commented Apr 13, 2018

Uh oh!

ivanbasov commented Apr 13, 2018

Uh oh!

tmat Apr 13, 2018 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmat Apr 13, 2018 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmat Apr 13, 2018 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmat Apr 13, 2018 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmat Apr 13, 2018 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmat Apr 13, 2018

Choose a reason for hiding this comment

Uh oh!

cston Apr 13, 2018

Choose a reason for hiding this comment

Uh oh!

tmat Apr 13, 2018 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

cston commented Apr 5, 2018 •

edited

Loading

cston Apr 5, 2018 •

edited

Loading

cston Apr 5, 2018 •

edited

Loading

ivanbasov Apr 9, 2018 •

edited

Loading

ivanbasov Apr 9, 2018 •

edited

Loading

ivanbasov Apr 9, 2018 •

edited

Loading

ivanbasov Apr 9, 2018 •

edited

Loading

jaredpar Apr 10, 2018 •

edited by cston

Loading

tmat Apr 13, 2018 •

edited by cston

Loading

tmat Apr 13, 2018 •

edited by cston

Loading

tmat Apr 13, 2018 •

edited by cston

Loading

tmat Apr 13, 2018 •

edited by cston

Loading

tmat Apr 13, 2018 •

edited by cston

Loading

tmat Apr 13, 2018 •

edited by cston

Loading

tmat Apr 13, 2018 •

edited by cston

Loading

tmat Apr 13, 2018 •

edited by cston

Loading

tmat Apr 13, 2018 •

edited by cston

Loading

tmat Apr 13, 2018 •

edited by cston

Loading

tmat Apr 13, 2018 •

edited by cston

Loading

tmat Apr 13, 2018 •

edited by cston

Loading

tmat Apr 13, 2018 •

edited by cston

Loading

tmat Apr 13, 2018 •

edited by cston

Loading

tmat commented Apr 13, 2018 •

edited by cston

Loading

tmat Apr 13, 2018 •

edited by cston

Loading

tmat Apr 13, 2018 •

edited by cston

Loading

tmat Apr 13, 2018 •

edited by cston

Loading