Explicit assembly loading #850

tomasherceg · 2020-06-14T11:55:29Z

Since PR #812 contained a lot of changes and was difficult to merge, I've extracted part of the changes as a separate PR.

Some static methods (FindType, GetAllAssemblies etc.) were moved from ReflectionUtils to CompiledAssemblyCache and changed to be instance methods. The logic inside these methods was not changed.
Added experimental flag ExplicitAssemblyLoading which disables aggressive loading of all referenced assemblies (including transitive references) and uses only assemblies explicitly listed in config.Markup.Assemblies.
Added simple performance tracking mechanism IStartupTracer for the DotVVM startup code.
Since various tests and custom JSON converters relied on static ReflectionUtils, there is CompiledAssemblyCache.Instance which contains a singleton assembly cache. In tests, it is possible that multiple instances of this class are created, which is unfortunate, but tolerable. In real DotVVM web apps, there should be always only instance created by DI.

…assemblies

exyi · 2020-06-16T10:09:30Z

Since various tests and custom JSON converters relied on static ReflectionUtils, there is CompiledAssemblyCache.Instance which contains a singleton assembly cache. ... In real DotVVM web apps, there should be always only instance created by DI.

No, the reason why we have everything in the DI is to allow multiple instances of DotVVM (and entire AspNetCore in a process). This approach will lead to collisions and fail horribly if I'd run multiple servers in the process.

However, it will be mostly fine to register all allowed assemblies in a single "cache" - have a union of both configurations.

exyi · 2020-06-16T10:11:15Z

The name CompiledAssemblyCache is terrible for a class that is used everywhere for doing reflection. I think should named SomethingContext, maybe DotvvmReflectionContext, but I don't know. But it is certainly not a cache of compiled assemblies, how "compiled"?

tomasherceg · 2020-06-16T10:41:27Z

The name CompiledAssemblyCache is terrible for a class that is used everywhere for doing reflection. I think should named SomethingContext, maybe DotvvmReflectionContext, but I don't know. But it is certainly not a cache of compiled assemblies, how "compiled"?

I am not sure why it was named that - it is quite old. Basically it maintains a list of assemblies that are used in Roslyn compilation, and caches Roslyn MetadataReferences.
DotvvmReflectionContext seems fine to me, or maybe DotvvmCompilationContext.

tomasherceg · 2020-06-16T10:47:43Z

No, the reason why we have everything in the DI is to allow multiple instances of DotVVM (and entire AspNetCore in a process). This approach will lead to collisions and fail horribly if I'd run multiple servers in the process.
Is there a way how to get things from DI from JsonConverter attributes?https://github.com/riganti/dotvvm/pull/850/files#diff-216e10f9b6e51de337bb86e303e2c5a4R19

Running two instances of DotVVM in a single app would be a nice thing, but I'm not sure how many people would even consider doing this.

exyi · 2020-06-16T10:17:41Z

src/DotVVM.Framework.Hosting.Owin/AppBuilderExtensions.cs

+            startupTracer.TraceEvent(StartupTracingConstants.AddDotvvmStarted);
+
+            var config = DotvvmConfiguration.CreateDefault(s =>
+            {


we format the { on the first line in expressions.

exyi · 2020-06-16T10:19:51Z

src/DotVVM.Framework/DependencyInjection/DotvvmBuilderExtensions.cs

            services.Services.AddScoped<IRequestTracer>(s => {
                var config = s.GetRequiredService<DotvvmConfiguration>();
                return (config.Debug ? (IRequestTracer)s.GetService<DiagnosticsRequestTracer>() : null) ?? new NullRequestTracer();
            });

+            services.Services.AddScoped<DiagnosticsStartupTracer>();
+            services.Services.AddScoped<IStartupTracer>(s => {


Why Scoped? It is created once for the app startup anyway, right?

I removed this - I haven't noticed that this method is meant for the Diagnostic Window feature in VS extension that is not finished and probably doesn't work.

exyi · 2020-06-16T10:20:55Z

src/DotVVM.Framework.Hosting.AspNetCore/ServiceCollectionExtensions.cs


 namespace Microsoft.Extensions.DependencyInjection
 {
    public static class ServiceCollectionExtensions
    {
+        private static readonly DiagnosticsStartupTracer startupTracer = new DiagnosticsStartupTracer();


wait, where does the information from this tracer go?

It is added to the IServiceCollection container later (at the end of the file).
In ASP.NET Core, it needs to be a static field because we need it before the IServiceCollection is ready and because it is called from both ConfigureServices and Configure methods in Startup.
In OWIN it can be just a local variable as there is only one method for initialization of DotVVM.

exyi · 2020-06-16T10:22:07Z

src/DotVVM.Framework.Hosting.Owin/AppBuilderExtensions.cs

@@ -56,7 +57,11 @@ public static DotvvmConfiguration UseDotVVM<TStartup>(this IAppBuilder app, stri

        private static DotvvmConfiguration UseDotVVM(this IAppBuilder app, string applicationRootPath, bool useErrorPages, bool debug, IDotvvmServiceConfigurator configurator, IDotvvmStartup startup, Func<IServiceCollection, IServiceProvider> serviceProviderFactoryMethod = null, Action<DotvvmConfiguration> modifyConfiguration = null)
        {
-            var config = DotvvmConfiguration.CreateDefault(s => {
+            var startupTracer = new DiagnosticsStartupTracer();


This is super confusing - one tracer is registered in the DI and there is another? Which one should be used?

This one is correct, I removed the other one.

exyi · 2020-06-16T10:24:53Z

src/DotVVM.Framework/Diagnostics/DiagnosticsStartupTracer.cs

+                {
+                    if (sentEventsCount < events.Count)
+                    {
+                        informationSender.SendInformationAsync(BuildDiagnosticsInformation(events.Skip(sentEventsCount).ToList()));


I guess we should rather use a loop with await Task.Delay(10sec) so that we can await this task.

It is a loop so it would never end. Or maybe I don't understand.

Ok, I don't understand if the timer fires only once or every 10sec. In any case, why not use Task.Delay in an async method? We could await the task returned from SendInformationAsync

exyi · 2020-06-16T11:23:17Z

src/DotVVM.Framework/Compilation/DefaultViewCompiler.cs

        public virtual CSharpCompilation CreateCompilation(string assemblyName)
        {
            return CSharpCompilation.Create(assemblyName, options: new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary))
-                .AddReferences(referencedAssembliesCache.Value.Select(a => assemblyCache.GetAssemblyMetadata(a)));
+                .AddReferences(assemblyCache.GetAllAssemblies().Select(a => assemblyCache.GetAssemblyMetadata(a)));


This used to reference only some assemblies and now we reference everything when the experimental option is disabled, right? I don't know if that will break, but I'd guess there is a reson why the logic used to be so complex.

Good catch - I've distinguished between the referenced assemblies and "all" assemblies, and work only with referenced assemblies here.

exyi · 2020-06-16T11:51:16Z

src/DotVVM.Framework/Compilation/DefaultViewCompilerCodeEmitter.cs

@@ -167,7 +169,7 @@ public string EmitCreateObject(string typeName, object[] constructorArguments =
            }

            var typeSyntax = ReflectionUtils.IsFullName(typeName)
-                ? ParseTypeName(ReflectionUtils.FindType(typeName))
+                ? ParseTypeName(compiledAssemblyCache.FindType(typeName))


When I see this, the type parsing should not be done by emitter, can we move it to the caller?

exyi · 2020-06-16T11:51:34Z

src/DotVVM.Framework/Compilation/DefaultViewCompilerCodeEmitter.cs

@@ -769,7 +771,7 @@ public IEnumerable<SyntaxTree> BuildTree(string namespaceName, string className,
            UseType(BuilderDataContextType);

            var controlType = ReflectionUtils.IsFullName(ResultControlType)
-                ? ParseTypeName(ReflectionUtils.FindType(ResultControlType))
+                ? ParseTypeName(compiledAssemblyCache.FindType(ResultControlType))


Same here, type parsing should not be done by emitter.

exyi · 2020-06-16T11:56:06Z

src/DotVVM.Framework/Configuration/DotvvmGlobalExperimentalFeatureFlag.cs

+
+        public void Enable()
+        {
+            ThrowIfFrozen();


This is redundant.

exyi · 2020-06-16T11:57:52Z

src/DotVVM.Framework/ResourceManagement/ResourceRepositoryJsonConverter.cs

@@ -36,7 +37,7 @@ protected virtual IEnumerable<Type> ResolveAllTypesDerivedFromIResource(string d

        protected virtual IEnumerable<Assembly> GetAllAssembliesLoadedAssemblies()
        {
-            return ReflectionUtils.GetAllAssemblies();
+            return AppDomain.CurrentDomain.GetAssemblies();


It seems to be unused, anyway. I don't know why it's virtual, though.

Removed together with ResolveAllTypesDerivedFromIResource that was unused too.

Compiler overrides this method for its purposes.

exyi · 2020-06-16T12:01:04Z

Running two instances of DotVVM in a single app would be a nice thing, but I'm not sure how many people would even consider doing this.

I did. It is what we do in the benchmarks. It is also a convenient way to run two separate apps that are on different domains in a single process (like admin + the actual app).

tomasherceg · 2020-06-16T12:44:45Z

I did. It is what we do in the benchmarks. It is also a convenient way to run two separate apps that are on different domains in a single process (like admin + the actual app).

Cool, good to know.

…mblies

src/DotVVM.Framework/Compilation/ViewCompilingVisitor.cs

exyi · 2020-07-11T22:41:11Z

src/DotVVM.Framework/Diagnostics/DiagnosticsStartupTracer.cs

+                {
+                    if (sentEventsCount < events.Count)
+                    {
+                        informationSender.SendInformationAsync(BuildDiagnosticsInformation(events.Skip(sentEventsCount).ToList()));


Ok, I don't understand if the timer fires only once or every 10sec. In any case, why not use Task.Delay in an async method? We could await the task returned from SendInformationAsync

tomasherceg · 2020-07-25T14:14:50Z

Ok, I don't understand if the timer fires only once or every 10sec. In any case, why not use Task.Delay in an async method? We could await the task returned from SendInformationAsync

I've got rid of the timer completely. When the DotVVM initialization ends, the events are sent using the DiagnosticsInformationSender. In case of additional events reported after this sending, the LateInfoReported event is triggered and there is a handler which sends these late events. And it returns task in canse anyone is interested.

exyi · 2020-08-01T15:22:42Z

src/DotVVM.Samples.Common/TextFileDiagnosticsInformationSender.cs

+{new string('-', 70 + 15 + 15)}
+{messages}";
+            }
+            File.AppendAllText(logFilePath, messages);


This may run in parallel, we should have a lock around this line (I know it's just samples, but still)

You're right - fixed.

exyi · 2020-08-01T15:29:39Z

src/DotVVM.Framework/Compilation/CompiledAssemblyCache.cs

+        }
+
+        public bool IsAssemblyNamespace(string fullName)
+            => GetAllNamespaces().Contains(fullName, StringComparer.Ordinal);


Hmm, I know this is just copy paste from the old utils. However, it would make more sense to create the HashSet once with the Ordinal comparer, and then cache it in a Lazy field.

…oading

exyi

Given we rename the CompiledAssemblyCache to something else in the next PR, this is ok :)

tomasherceg added 13 commits March 5, 2020 15:37

Startup performance tracking - unfinished

b033489

Startup performance diagnostics

da3b9d9

Redundant assembly loading code removed

057e2fb

Configruation - experimental feature for disabling auto-discovery of …

0749923

…assemblies

Refactoring - moving all assembly resolution to CompiledAssemblyCache

95ba4d2

Fixed compile error in DotVVM compiler

7c3ab3c

Merge branch 'master' into perf-startup

67401ea

Merge branch 'master' into explicit-assembly-loading

3c1b9fd

Fixed errors in tests

ba5b8e1

Unit test fixes

da208c4

Extracted some changes from perf-startup branch, cleanup

d9a1aaa

Experimental flag name changed so it doesn't contain the word Disable

e915179

Reverted unnecessary whitespace changes

a630147

tomasherceg added this to the Version 2.5 milestone Jun 14, 2020

exyi requested changes Jun 16, 2020

View reviewed changes

tomasherceg added 5 commits July 3, 2020 23:18

Explicit assemblies - tiny fixes from code review comments

baf9526

CompiledAssemblyCache - distinguished between referenced and all asse…

ab166f8

…mblies

TypeRegistry - CompiledAssemblyCache moved to closure

1aecff0

Merged changes from master

8d56dc5

Added startup tracing to startup view compilation

550dba4

exyi reviewed Jul 11, 2020

View reviewed changes

tomasherceg added 2 commits July 25, 2020 15:51

Tiny fixes in generating resolved type name

24bf006

Timer removed from DiagnosticsStartupTracer

63d72d9

exyi reviewed Aug 1, 2020

View reviewed changes

tomasherceg added 5 commits August 4, 2020 16:53

Fixed lookup for namespaces in compiled assembly cache

3cf8e17

TextFileDiagnosticsInformationSender - fixed race condition

5a8259e

Fixed breaking change in namespace resolution

091d3e4

Fixed samples - missing assemblies added

f1d27f6

Merge remote-tracking branch 'origin/master' into explicit-assembly-l…

4638be4

…oading

exyi approved these changes Aug 7, 2020

View reviewed changes

tomasherceg added 2 commits August 7, 2020 13:54

Fixled circular resource dependencies UI test

ff7bbfb

OWIN samples fixed

f2fe507

tomasherceg merged commit 78c2f51 into master Aug 7, 2020

tomasherceg deleted the explicit-assembly-loading branch August 7, 2020 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Explicit assembly loading #850

Explicit assembly loading #850

tomasherceg commented Jun 14, 2020

exyi commented Jun 16, 2020

exyi commented Jun 16, 2020

tomasherceg commented Jun 16, 2020

tomasherceg commented Jun 16, 2020

exyi Jun 16, 2020

exyi Jun 16, 2020

tomasherceg Jul 3, 2020

exyi Jun 16, 2020

tomasherceg Jul 3, 2020

exyi Jun 16, 2020

tomasherceg Jul 3, 2020

exyi Jun 16, 2020

tomasherceg Jul 3, 2020

exyi Jul 11, 2020

exyi Jun 16, 2020

tomasherceg Jul 3, 2020

exyi Jun 16, 2020

exyi Jun 16, 2020

exyi Jun 16, 2020

exyi Jun 16, 2020

tomasherceg Jul 3, 2020

quigamdev Aug 1, 2020

exyi commented Jun 16, 2020

tomasherceg commented Jun 16, 2020

exyi Jul 11, 2020

tomasherceg commented Jul 25, 2020

exyi Aug 1, 2020

tomasherceg Aug 4, 2020

exyi Aug 1, 2020

tomasherceg Aug 4, 2020

exyi left a comment

Explicit assembly loading #850

Explicit assembly loading #850

Conversation

tomasherceg commented Jun 14, 2020

exyi commented Jun 16, 2020

exyi commented Jun 16, 2020

tomasherceg commented Jun 16, 2020

tomasherceg commented Jun 16, 2020

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

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

exyi commented Jun 16, 2020

tomasherceg commented Jun 16, 2020

Choose a reason for hiding this comment

tomasherceg commented Jul 25, 2020

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

exyi left a comment

Choose a reason for hiding this comment