This repository has been archived by the owner. It is now read-only.

ILEmit backend for DependencyInjeciton #630

Merged
merged 10 commits into from Mar 19, 2018

Conversation

Projects
None yet
6 participants
@pakrym
Contributor

pakrym commented Mar 14, 2018

No description provided.

@rynowak

This comment has been minimized.

Member

rynowak commented Mar 14, 2018

I see Emit and I upvote ❤️ 🔥 🍺

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

This comment has been minimized.

@davidfowl

davidfowl Mar 14, 2018

Member

Private assets?

@@ -16,5 +16,6 @@ public ConstantCallSite(Type serviceType, object defaultValue)
public Type ServiceType => DefaultValue.GetType();
public Type ImplementationType => DefaultValue.GetType();
public CallSiteKind Kind { get; } = CallSiteKind.Constant;

This comment has been minimized.

@davidfowl

davidfowl Mar 14, 2018

Member

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

This comment has been minimized.

@pakrym

pakrym Mar 14, 2018

Contributor

So JIT can do it's JITty stuff

@pakrym

This comment has been minimized.

Contributor

pakrym commented Mar 14, 2018

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

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

image

image

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

This comment has been minimized.

@natemcmaster

natemcmaster Mar 14, 2018

Member

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

This comment has been minimized.

@davidfowl

davidfowl Mar 14, 2018

Member

Yah, what we actually really need is this dotnet/corefx#25944

This comment has been minimized.

@pakrym

pakrym Mar 14, 2018

Contributor

Yep, this is temporary.

if (ReferenceEquals(scope, rootScope)
&& _scopedServices.TryGetValue(serviceType, out scopedService))
&& _scopedServices.TryGetValue(serviceType, out var scopedService))

This comment has been minimized.

@rynowak

rynowak Mar 14, 2018

Member

PranavPoints++

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

This comment has been minimized.

@rynowak

rynowak Mar 14, 2018

Member

Do we support value types?

This comment has been minimized.

@davidfowl

davidfowl Mar 14, 2018

Member

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

This comment has been minimized.

@pakrym

pakrym Mar 14, 2018

Contributor

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

This comment has been minimized.

@davidfowl

davidfowl Mar 14, 2018

Member

Make sure we add tests for it.

This comment has been minimized.

@pakrym

pakrym Mar 14, 2018

Contributor

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

This comment has been minimized.

@rynowak

rynowak Mar 14, 2018

Member

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

This comment has been minimized.

@pakrym

pakrym Mar 14, 2018

Contributor

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

@jawn

This comment has been minimized.

jawn commented Mar 15, 2018

Typo alert in PR title:
DependencyInjeciton >
DependencyInjection

@@ -12,5 +12,7 @@ internal interface IServiceCallSite
{
Type ServiceType { get; }
Type ImplementationType { get; }
CallSiteKind Kind { get; }

This comment has been minimized.

@davidfowl

davidfowl Mar 15, 2018

Member

nit: space

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

This comment has been minimized.

@davidfowl

davidfowl Mar 15, 2018

Member

Add copyright everywhere

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

This comment has been minimized.

@davidfowl

davidfowl Mar 15, 2018

Member

Add a comment about ReferenceEqualsEqualityComparer

@@ -18,9 +18,10 @@ internal class ServiceProviderEngineScope : IServiceScope, IServiceProvider
public ServiceProviderEngineScope(ServiceProviderEngine engine)
{
Engine = engine;
ResolvedServices = new Dictionary<object, object>();

This comment has been minimized.

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

This comment has been minimized.

@davidfowl

davidfowl Mar 15, 2018

Member

What is 6?

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));

This comment has been minimized.

@davidfowl

davidfowl Mar 15, 2018

Member

64, true?

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

This comment has been minimized.

@davidfowl
@davidfowl

This comment has been minimized.

Member

davidfowl commented Mar 15, 2018

@jawn thanks 😄

@davidfowl davidfowl closed this Mar 15, 2018

@davidfowl davidfowl reopened this Mar 15, 2018

@davidfowl davidfowl changed the title from [WIP] ILEmit backend for DependencyInjeciton to ILEmit backend for DependencyInjeciton Mar 15, 2018

@davidfowl davidfowl changed the title from ILEmit backend for DependencyInjeciton to ILEmit backend for DependancyInjeciton Mar 15, 2018

@davidfowl davidfowl changed the title from ILEmit backend for DependancyInjeciton to ILEmit backend for DependancyInjeciton lenght Mar 15, 2018

@davidfowl davidfowl changed the title from ILEmit backend for DependancyInjeciton lenght to ILEmit backend for DependencyInjeciton Mar 15, 2018

@@ -17,5 +17,7 @@ public FactoryCallSite(Type serviceType, Func<IServiceProvider, object> factory)
public Type ServiceType { get; }
public Type ImplementationType => null;

This comment has been minimized.

@davidfowl

davidfowl Mar 16, 2018

Member

nit: Remove space

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

This comment has been minimized.

@davidfowl

davidfowl Mar 16, 2018

Member

Make this read only since Add returns a new struct?

public bool HasScope;
public ILEmitCallSiteAnalysisResult Add(ILEmitCallSiteAnalysisResult other)

This comment has been minimized.

@davidfowl

davidfowl Mar 16, 2018

Member

Pass via in?

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

This comment has been minimized.

@davidfowl

davidfowl Mar 16, 2018

Member

Write a general comment about why this exists.

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

This comment has been minimized.

@davidfowl

davidfowl Mar 16, 2018

Member

nit: Remove the ()

@@ -5,9 +5,9 @@
namespace Microsoft.Extensions.DependencyInjection.Tests
{
public class ServiceProviderCompiledContainerTests : ServiceProviderContainerTests
public class ServiceProviderILEmitContainerTests: ServiceProviderContainerTests

This comment has been minimized.

@davidfowl

davidfowl Mar 16, 2018

Member

nit: fix formatting

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 comment has been minimized.

@davidfowl

davidfowl Mar 16, 2018

Member

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

return null;
}

This comment has been minimized.

@davidfowl

davidfowl Mar 16, 2018

Member

nit: Remove extra space

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

This comment has been minimized.

@davidfowl

davidfowl Mar 16, 2018

Member

walks.....

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

This comment has been minimized.

@davidfowl

davidfowl Mar 16, 2018

Member

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

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

This comment has been minimized.

@davidfowl

davidfowl Mar 16, 2018

Member

nit: skit

context.Generator.BeginExceptionBlock();
// scope

This comment has been minimized.

@davidfowl

davidfowl Mar 16, 2018

Member

Fix the codegen comment here

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

This comment has been minimized.

@davidfowl

davidfowl Mar 16, 2018

Member

Add a comment here

@davidfowl

LGTM just the nits

@davidfowl

This comment has been minimized.

Member

davidfowl commented Mar 17, 2018

@pakrym why is the built red?

@Zoxive

This comment has been minimized.

Zoxive commented Mar 17, 2018

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 aspnet/AspNetCore#2737

@Zoxive

This comment has been minimized.

Zoxive commented Mar 17, 2018

@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]

@pakrym pakrym changed the base branch from dev to release/2.1 Mar 19, 2018

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

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details

@natemcmaster natemcmaster deleted the pakrym/il-emit-backend branch Nov 2, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.