ILEmit backend for DependencyInjeciton by pakrym · Pull Request #630 · aspnet/DependencyInjection

pakrym · 2018-03-14T01:02:30Z

No description provided.

rynowak · 2018-03-14T01:28:15Z

I see Emit and I upvote ❤️ 🔥 🍺

davidfowl · 2018-03-14T02:39:02Z

src/DI/DI.csproj


  <ItemGroup>
    <PackageReference Include="Microsoft.Extensions.TypeNameHelper.Sources" PrivateAssets="All" Version="$(MicrosoftExtensionsTypeNameHelperSourcesPackageVersion)" />
+    <PackageReference Include="System.Reflection.Emit" PrivateAssets="All" Version="$(SystemReflectionEmitPackageVersion)" />


Private assets?

davidfowl · 2018-03-14T02:40:00Z

src/DI/ServiceLookup/ConstantCallSite.cs


        public Type ServiceType => DefaultValue.GetType();
        public Type ImplementationType => DefaultValue.GetType();
+        public CallSiteKind Kind { get; } = CallSiteKind.Constant;


public CallSiteKind Kind => CallSiteKind.Constant; Why do you want a backing field?

So JIT can do it's JITty stuff

pakrym · 2018-03-14T03:49:39Z

Before/After `BuildServiceProvider` with Scoped->Scoped->Scoped tree

25% faster, ~3x less allocations in count and size

natemcmaster · 2018-03-14T06:48:25Z

src/DI/DI.csproj


  <ItemGroup>
    <PackageReference Include="Microsoft.Extensions.TypeNameHelper.Sources" PrivateAssets="All" Version="$(MicrosoftExtensionsTypeNameHelperSourcesPackageVersion)" />
+    <PackageReference Include="System.Reflection.Emit" PrivateAssets="All" Version="$(SystemReflectionEmitPackageVersion)" />


I'd recommend making these package references conditional on TargetFramework. System.Ref.Emit is not supported in all netstandard2.0-compatible platforms.

Yah, what we actually really need is this https://github.com/dotnet/corefx/issues/25944

Yep, this is temporary.

rynowak · 2018-03-14T08:51:06Z

src/DI/ServiceLookup/CallSiteValidator.cs

-            Type scopedService;
            if (ReferenceEquals(scope, rootScope)
-                && _scopedServices.TryGetValue(serviceType, out scopedService))
+                && _scopedServices.TryGetValue(serviceType, out var scopedService))


PranavPoints++

rynowak · 2018-03-14T08:54:45Z

src/DI/ServiceLookup/ILEmitResolverBuilder.cs

+            {
+                VisitCallSite(parameterCallSite, argument);
+            }
+            argument.Generator.Emit(OpCodes.Newobj, constructorCallSite.ConstructorInfo);


Do we support value types?

Has anyone ever put a struct in the container? Now I'm curious...

I'm sure someone did and we have to support it.

Make sure we add tests for it.

It didn't work in expression either, I tried adding tests and they fail.

Yeah but the IL for newing up a struct is different soooo

Nono, I'm sure they would fail in my code. They are failing in old Expression generating code too.

jawn · 2018-03-15T20:13:31Z

Typo alert in PR title:
DependencyInjeciton >
DependencyInjection

davidfowl · 2018-03-15T22:38:26Z

src/DI/ServiceLookup/IServiceCallSite.cs

        Type ServiceType { get; }
        Type ImplementationType { get; }
+        CallSiteKind Kind { get; }
+


davidfowl · 2018-03-15T22:38:57Z

src/DI/ServiceLookup/ReferenceEqualsEqualityComparer.cs

@@ -0,0 +1,15 @@
+using System.Collections.Generic;


Add copyright everywhere

davidfowl · 2018-03-15T22:39:44Z

src/DI/ServiceLookup/ServiceProviderEngine.cs

            CallSiteFactory.Add(typeof(IServiceProvider), new ServiceProviderCallSite());
            CallSiteFactory.Add(typeof(IServiceScopeFactory), new ServiceScopeFactoryCallSite());
+
+            RealizedServices = new ConcurrentDictionary<Type, Func<ServiceProviderEngineScope, object>>(new ReferenceEqualsEqualityComparer<Type>());


Add a comment about ReferenceEqualsEqualityComparer

davidfowl · 2018-03-15T22:44:40Z

src/DI/ServiceLookup/ServiceProviderEngineScope.cs

        public ServiceProviderEngineScope(ServiceProviderEngine engine)
        {
            Engine = engine;
+            ResolvedServices = new Dictionary<object, object>();


davidfowl · 2018-03-15T22:45:20Z

src/DI/ServiceLookup/ILEmit/ILEmitCallSiteAnalyzer.cs

+
+        protected override ILEmitCallSiteAnalysisResult VisitIEnumerable(IEnumerableCallSite enumerableCallSite, object argument)
+        {
+            var result = new ILEmitCallSiteAnalysisResult(6);


davidfowl · 2018-03-15T22:46:19Z

src/DI/ServiceLookup/ILEmit/ILEmitCallSiteAnalyzer.cs

+
+        protected override ILEmitCallSiteAnalysisResult VisitSingleton(SingletonCallSite singletonCallSite, object argument) => VisitCallSite(singletonCallSite.ServiceCallSite, argument);
+
+        protected override ILEmitCallSiteAnalysisResult VisitScoped(ScopedCallSite scopedCallSite, object argument) => new ILEmitCallSiteAnalysisResult(64, true).Add(VisitCallSite(scopedCallSite.ServiceCallSite, argument));


davidfowl · 2018-03-15T22:52:38Z

src/DI/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs

+
+        private static bool BeginCaptureDisposable(Type implType, ILEmitResolverBuilderContext argument)
+        {
+            var shouldCapture = !(implType != null && !typeof(IDisposable).GetTypeInfo().IsAssignableFrom(implType.GetTypeInfo()));


davidfowl · 2018-03-15T22:53:36Z

@jawn thanks 😄

davidfowl · 2018-03-16T06:24:53Z

src/DI/ServiceLookup/FactoryCallSite.cs


        public Type ServiceType { get; }
        public Type ImplementationType => null;
+


nit: Remove space

davidfowl · 2018-03-16T06:25:35Z

src/DI/ServiceLookup/ILEmit/ILEmitCallSiteAnalysisResult.cs

+
+namespace Microsoft.Extensions.DependencyInjection.ServiceLookup
+{
+    internal struct ILEmitCallSiteAnalysisResult


Make this read only since Add returns a new struct?

davidfowl · 2018-03-16T06:25:55Z

src/DI/ServiceLookup/ILEmit/ILEmitCallSiteAnalysisResult.cs

+
+        public bool HasScope;
+
+        public ILEmitCallSiteAnalysisResult Add(ILEmitCallSiteAnalysisResult other)


Pass via in?

davidfowl · 2018-03-16T06:26:29Z

src/DI/ServiceLookup/ILEmit/ILEmitCallSiteAnalyzer.cs

+
+namespace Microsoft.Extensions.DependencyInjection.ServiceLookup
+{
+    internal sealed class ILEmitCallSiteAnalyzer : CallSiteVisitor<object, ILEmitCallSiteAnalysisResult>


Write a general comment about why this exists.

davidfowl · 2018-03-16T06:27:16Z

test/DI.Tests/ServiceProviderILEmitContainerTests.cs

    {
        protected override IServiceProvider CreateServiceProvider(IServiceCollection collection) =>
-            collection.BuildServiceProvider(new ServiceProviderOptions { Mode = ServiceProviderMode.Compiled });
+            collection.BuildServiceProvider(new ServiceProviderOptions() { Mode = ServiceProviderMode.ILEmit});


nit: Remove the ()

davidfowl · 2018-03-16T06:27:26Z

test/DI.Tests/ServiceProviderILEmitContainerTests.cs

 namespace Microsoft.Extensions.DependencyInjection.Tests
 {
-    public class ServiceProviderCompiledContainerTests : ServiceProviderContainerTests
+    public class ServiceProviderILEmitContainerTests: ServiceProviderContainerTests


nit: fix formatting

davidfowl · 2018-03-16T06:31:13Z

src/DI/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs

+
+        private Func<ServiceProviderEngineScope, object> BuildType(IServiceCallSite callSite)
+        {
+            var dynamicMethod = new DynamicMethod("ResolveService", MethodAttributes.Public | MethodAttributes.Static, CallingConventions.Standard, typeof(object), new [] {typeof(ILEmitResolverBuilderRuntimeContext), typeof(ServiceProviderEngineScope) }, GetType(), true);


This line is too long. Comment on why you had to pass true as the last argument and use the named parameter syntax.

davidfowl · 2018-03-16T06:40:51Z

src/DI/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs

+            return null;
+        }
+
+


nit: Remove extra space

davidfowl · 2018-03-16T22:23:10Z

src/DI/ServiceLookup/ILEmit/ILEmitCallSiteAnalyzer.cs

+
+namespace Microsoft.Extensions.DependencyInjection.ServiceLookup
+{
+    // This class walkes the service scope tree and tries to calculate approximate


davidfowl · 2018-03-16T22:23:53Z

src/DI/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs

+            {
+                VisitCallSite(parameterCallSite, argument);
+            }
+            argument.Generator.Emit(OpCodes.Newobj, constructorCallSite.ConstructorInfo);


Add the comment // new T(...)

davidfowl · 2018-03-16T22:56:31Z

src/DI/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs

+
+        private Func<ServiceProviderEngineScope, object> BuildType(IServiceCallSite callSite)
+        {
+            // We need to skit visibility checks because services/constructors might be private


davidfowl · 2018-03-16T22:57:03Z

src/DI/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs

+
+                context.Generator.BeginExceptionBlock();
+
+                // scope


Fix the codegen comment here

davidfowl · 2018-03-16T22:57:20Z

src/DI/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs

+
+        private static void EndCaptureDisposable(ILEmitResolverBuilderContext argument)
+        {
+            argument.Generator.Emit(OpCodes.Callvirt, ExpressionResolverBuilder.CaptureDisposableMethodInfo);


Add a comment here

davidfowl

LGTM just the nits

davidfowl · 2018-03-17T04:02:37Z

@pakrym why is the built red?

Zoxive · 2018-03-17T04:33:24Z

I see the new test does 350 dependencies deep. Is this the new "limit"? (I see there are test files still included for 999) I'll grab this branch and try it out tomorrow with my breaking project from dotnet/aspnetcore#2737

Zoxive · 2018-03-17T13:17:43Z

@pakrym Works great. Also as @davidfowl mentioned about the build failing i had to add readonly to the struct properties to get it to compile locally as well.

C:\projects\dependencyinjection\src\DI\ServiceLookup\ILEmit\ILEmitCallSiteAnalysisResult.cs(19,20): error CS8340: Instance fields of readonly structs must be readonly. [C:\projects\dependencyinjection\src\DI\DI.csproj]

davidfowl reviewed Mar 14, 2018

View reviewed changes

natemcmaster reviewed Mar 14, 2018

View reviewed changes

rynowak reviewed Mar 14, 2018

View reviewed changes

davidfowl reviewed Mar 15, 2018

View reviewed changes

davidfowl closed this Mar 15, 2018

davidfowl reopened this Mar 15, 2018

davidfowl changed the title ~~[WIP] ILEmit backend for DependencyInjeciton~~ ILEmit backend for DependencyInjeciton Mar 15, 2018

davidfowl changed the title ~~ILEmit backend for DependencyInjeciton~~ ILEmit backend for DependancyInjeciton Mar 15, 2018

davidfowl changed the title ~~ILEmit backend for DependancyInjeciton~~ ILEmit backend for DependancyInjeciton lenght Mar 15, 2018

davidfowl changed the title ~~ILEmit backend for DependancyInjeciton lenght~~ ILEmit backend for DependencyInjeciton Mar 15, 2018

davidfowl reviewed Mar 16, 2018

View reviewed changes

src/DI/ServiceLookup/FactoryCallSite.cs

public Type ServiceType { get; }

public Type ImplementationType => null;

Copy link

Member

davidfowl Mar 16, 2018

Choose a reason for hiding this comment

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

nit: Remove space

davidfowl reviewed Mar 16, 2018

View reviewed changes

src/DI/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs Outdated

return null;

}

Copy link

Member

davidfowl Mar 16, 2018

Choose a reason for hiding this comment

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

nit: Remove extra space

pakrym force-pushed the pakrym/il-emit-backend branch from 10f7b69 to c0ccdd2 Compare March 16, 2018 22:15

davidfowl reviewed Mar 16, 2018

View reviewed changes

pakrym force-pushed the pakrym/il-emit-backend branch 2 times, most recently from fdb06da to c1d0447 Compare March 16, 2018 22:38

davidfowl reviewed Mar 16, 2018

View reviewed changes

src/DI/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs

context.Generator.BeginExceptionBlock();

// scope

Copy link

Member

davidfowl Mar 16, 2018

Choose a reason for hiding this comment

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

Fix the codegen comment here

davidfowl reviewed Mar 16, 2018

View reviewed changes

davidfowl approved these changes Mar 16, 2018

View reviewed changes

pakrym force-pushed the pakrym/il-emit-backend branch from 8198538 to 7a34593 Compare March 19, 2018 16:02

pakrym changed the base branch from dev to release/2.1 March 19, 2018 16:08

pakrym added 10 commits March 19, 2018 09:08

Add ILEmit backend

f89905c

More things

ebeda60

Some optimizations and method size detection experiment

4b02a45

Cross compile and disabled tests

88343a9

Test more for RELEASE

dbc1ad5

Better name

75681af

Cross complile to app2.0 and add more tests

1665d80

Undo perf multi targeting

294ca9d

PR comments

2381852

More PR Comments

bec355c

pakrym force-pushed the pakrym/il-emit-backend branch from 7a34593 to bec355c Compare March 19, 2018 16:11

pakrym merged commit d5b3851 into release/2.1 Mar 19, 2018

jkotas mentioned this pull request Apr 17, 2018

samples/WebApi doesn't work with .NET Core 2.1 dotnet/corert#5720

Closed

natemcmaster deleted the pakrym/il-emit-backend branch November 2, 2018 16:23

pakrym mentioned this pull request Apr 25, 2020

Emit shorter opcodes in ILGenerator.Emit(OpCode, int) dotnet/runtime#35427

Merged


		protected override ILEmitCallSiteAnalysisResult VisitSingleton(SingletonCallSite singletonCallSite, object argument) => VisitCallSite(singletonCallSite.ServiceCallSite, argument);

		protected override ILEmitCallSiteAnalysisResult VisitScoped(ScopedCallSite scopedCallSite, object argument) => new ILEmitCallSiteAnalysisResult(64, true).Add(VisitCallSite(scopedCallSite.ServiceCallSite, argument));


		public Type ServiceType { get; }
		public Type ImplementationType => null;


		public bool HasScope;

		public ILEmitCallSiteAnalysisResult Add(ILEmitCallSiteAnalysisResult other)

Conversation

pakrym commented Mar 14, 2018

Uh oh!

rynowak commented Mar 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl Mar 14, 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!

Choose a reason for hiding this comment

Uh oh!

pakrym commented Mar 14, 2018

Before/After BuildServiceProvider with Scoped->Scoped->Scoped tree

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jawn commented Mar 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl Mar 15, 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!

Choose a reason for hiding this comment

Uh oh!

davidfowl commented Mar 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl Mar 16, 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!

davidfowl Mar 14, 2018 •

edited

Loading

Before/After `BuildServiceProvider` with Scoped->Scoped->Scoped tree

davidfowl Mar 15, 2018 •

edited

Loading

davidfowl Mar 16, 2018 •

edited

Loading

Zoxive commented Mar 17, 2018 •

edited

Loading