Skip to content

For perf, use the new ConstructorInvoker APIs for ActivatorUtilities.CreateFactory - #89573

Merged
steveharter merged 6 commits into
dotnet:mainfrom
steveharter:DiPerf
Aug 3, 2023
Merged

For perf, use the new ConstructorInvoker APIs for ActivatorUtilities.CreateFactory#89573
steveharter merged 6 commits into
dotnet:mainfrom
steveharter:DiPerf

Conversation

@steveharter

@stevehartersteveharter commented Jul 27, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes#66153 which also contains additional information on benchmarks here.

This PR changes DI to use the new zero-alloc invoke APIs and is expected to close out the DI+Blazor perf work for v8 -- Blazor apps can use DI ActivatorUtilities.CreateFactory without bringing in the large System.Linq.Expressions assembly and in a performant manner (but not quite as fast as having Blazor use Linq Expressions instead of reflection).

In summary, for Blazor, this PR makes CreateFactory ~1.3-1.5x faster for common "fast paths" measured under a Blazor client app. However, when run under CorClr+Windows, there is a much larger ~2-3x gain. The difference appears to be additional overhead in Mono interpreter lambda methods with variable capture. For NativeAOT, this PR is expected to make CreateFactory also around ~1.5x faster based on reflection methods being 1.3x-1.7x faster with the new zero-alloc APIs.

Background:

Note the reflection path is used (vs. Expressions) when RuntimeFeature.IsDynamicCodeCompiled == false which will be the case for Blazor client and NativeAOT . For NativeAOT, using expressions would cause them to be interpreted for NativeAOT which is very slow, and for Blazor using expressions would bring along the very large System.Linq.Expressions assembly where a smaller (trimmed) size is important. Note that this PR does not change the semantics here (that was done previously -- see the links above); only the CPU performance is improved.

@stevehartersteveharter self-assigned this Jul 27, 2023
@ghost

ghost commented Jul 27, 2023

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection
See info in area-owners.md if you want to be subscribed.

Issue Details

[verifying tests]

Author:steveharter
Assignees:steveharter
Labels:

tenet-performance, area-Extensions-DependencyInjection

Milestone:-

@stevehartersteveharter mentioned this pull request Jul 27, 2023
Closed
6 tasks
@stevehartersteveharter added this to the 8.0.0 milestone Jul 27, 2023
@steveharter
steveharter requested a review from buyaa-nJuly 27, 2023 16:58
@steveharter
steveharter marked this pull request as ready for review July 27, 2023 17:09
steveharter
steveharter commented Jul 27, 2023
View reviewed changes
Comment thread...libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ActivatorUtilitiesTests.cs

[Fact]
public void CreateFactory_CreatesFactoryMethod()
public void CreateFactory_CreatesFactoryMethod_4Types_3Injected()

@stevehartersteveharterJul 27, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This new test was added for coverage; the other new code paths already had coverage.

eerhardt
eerhardt reviewed Jul 27, 2023
View reviewed changes

@eerhardteerhardt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we have any perf numbers (both throughput and size) for this change?

Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs Outdated
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs Outdated
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs Outdated
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs Outdated
Comment thread...s/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.Reflection.cs Outdated
@build-analysisbuild-analysisBot mentioned this pull request Jul 27, 2023
Closed
davidfowl
davidfowl reviewed Aug 1, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
if (serviceProvider is null)
ThrowHelperArgumentNullExceptionServiceProvider();

object?[] constructorArguments = new object?[parameters.Length];

@davidfowldavidfowlAug 1, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not saying we should but we could pool this array now that we have Span support.

@steveharter

steveharter commented Aug 1, 2023

Copy link
Copy Markdown
ContributorAuthor

Do we have any perf numbers (both throughput and size) for this change?

See #85065 for the previous size gains of removing Linq.Expressions assembly (went from ~260k+ to 4k) when ActivatorUtilities.CreateFactory is used.

See #66153 which has additional details on CPU gains. Reflection APIs are much faster in general, and the DI CreateFactory is ~1.5x faster depending on the scenario, but still slower than using Linq expressions. We could investigate further as the Blazor gains were not as much as the Windows gains, percentage-wise.

buyaa-n
buyaa-n reviewed Aug 2, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Comment on lines +731 to +732
if (serviceProvider is null)
ThrowHelperArgumentNullExceptionServiceProvider();

@buyaa-nbuyaa-nAug 2, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NIT: Since it is within .NET 8 if-def could we use ArgumentNullException.ThrowIfNull(serviceProvider)? Here and 4 similar cases below

@stevehartersteveharterAug 3, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah there was an earlier discussion around this - it's at least consistent now across all cases using the helper method. In one case, under .NET 8, we couldn't use ThrowIfNull due to a compile warning around unused variable (but we still want to verify for consistency) and of course under .NetStandard.NetFx we can't use ThrowIfNull at all.

buyaa-n
buyaa-n approved these changes Aug 2, 2023
View reviewed changes

@buyaa-nbuyaa-n left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left a NIT, overall LGTM thanks!

@steveharter
steveharter merged commit c416966 into dotnet:mainAug 3, 2023
@steveharter
steveharter deleted the DiPerf branch August 3, 2023 12:31
@jkotas

jkotas commented Aug 4, 2023

Copy link
Copy Markdown
Member

This change badly broke DI for native AOT. Number of DI tests are failing on native AOT in this repo, and it soon going to affect ASP.NET and perflab. I think we need to revert this change.

Example of failure: https://helixre107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-pull-89969-merge-ce136d5e93d548e1a8/Microsoft.Extensions.Http.Tests/1/console.85a149a2.log?helixlogtype=result

[FAIL] Microsoft.Extensions.DependencyInjection.HttpClientFactoryServiceCollectionExtensionsTest.AddHttpClient_MessageHandler_Scope_SingletonDependency
System.Reflection.TargetParameterCountException : Parameter count mismatch.
at System.Reflection.DynamicInvokeInfo.ThrowForArgCountMismatch() + 0x7c
at System.Reflection.DynamicInvokeInfo.InvokeDirectWithFewArgs(Object, IntPtr, Span`1) + 0x1e8
at Internal.Reflection.Execution.MethodInvokers.InstanceMethodInvoker.InvokeDirectWithFewArgs(Object, Span`1) + 0x39
at Internal.Reflection.Execution.MethodInvokers.InstanceMethodInvoker.CreateInstanceWithFewArgs(Span`1) + 0x28
at System.Reflection.ConstructorInvoker.Invoke(Object, Object, Object, Object) + 0x5a
at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.ReflectionFactoryCanonicalFixed(ConstructorInvoker, ActivatorUtilities.FactoryParameterContext[], Type, IServiceProvider, Object[]) + 0x2f9
at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.<>c__DisplayClass14_2.<CreateFactoryReflection>b__7(IServiceProvider serviceProvider, Object[] arguments) + 0x27
at Microsoft.Extensions.Http.DefaultTypedHttpClientFactory`1.CreateClient(HttpClient) + 0x59
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitDisposeCache(ServiceCallSite, RuntimeResolverContext) + 0xe
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSite(ServiceCallSite callSite, TArgument argument) + 0xb5
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.Resolve(ServiceCallSite, ServiceProviderEngineScope) + 0x3d
at Microsoft.Extensions.DependencyInjection.ServiceProvider.GetService(ServiceIdentifier, ServiceProviderEngineScope) + 0xa3
at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider, Type) + 0x99
at Microsoft.Extensions.DependencyInjection.HttpClientFactoryServiceCollectionExtensionsTest.<AddHttpClient_MessageHandler_Scope_SingletonDependency>d__39.MoveNext() + 0x2b4

jkotas added a commit that referenced this pull request Aug 4, 2023
@jkotas
Revert "For perf, use the new ConstructorInvoker APIs for ActivatorUt…
87f09e1
…ilities.CreateFactory (#89573)"
This reverts commit c416966.
@jkotasjkotas mentioned this pull request Aug 4, 2023
Merged
AaronRobinsonMSFT pushed a commit that referenced this pull request Aug 4, 2023
@jkotas
Revert "For perf, use the new ConstructorInvoker APIs for ActivatorUt…
ba196ce
…ilities.CreateFactory (#89573)" (#89992)
This reverts commit c416966.
@stevehartersteveharter mentioned this pull request Aug 7, 2023
Merged
@ghostghost locked as resolved and limited conversation to collaborators Sep 3, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@davidfowldavidfowldavidfowl left review comments
@eerhardteerhardteerhardt left review comments
+2 more reviewers
@IDisposableIDisposableIDisposable left review comments
@buyaa-nbuyaa-nbuyaa-n approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@stevehartersteveharter

Labels

area-Extensions-DependencyInjectiontenet-performancePerformance related issue

Projects

None yet

Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

Consider updating ActivatorUtilities.CreateFactory to use ILEmit when possible

6 participants

@steveharter@jkotas@davidfowl@IDisposable@eerhardt@buyaa-n
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
For perf, use the new ConstructorInvoker APIs for ActivatorUtilities.CreateFactory by steveharter · Pull Request #89573 · dotnet/runtime · GitHub
Skip to content
For perf, use the new ConstructorInvoker APIs for ActivatorUtilities.CreateFactory - #89573
Merged
steveharter merged 6 commits into
dotnet:mainfrom 
steveharter:DiPerf
Aug 3, 2023
Merged
For perf, use the new ConstructorInvoker APIs for ActivatorUtilities.CreateFactory#89573
steveharter merged 6 commits into
dotnet:mainfrom 
steveharter:DiPerf
Conversation
@steveharter
@stevehartersteveharter
commented
Jul 27, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes#66153 which also contains additional information on benchmarks here.
This PR changes DI to use the new zero-alloc invoke APIs and is expected to close out the DI+Blazor perf work for v8 -- Blazor apps can use DI ActivatorUtilities.CreateFactory without bringing in the large System.Linq.Expressions assembly and in a performant manner (but not quite as fast as having Blazor use Linq Expressions instead of reflection).
In summary, for Blazor, this PR makes CreateFactory ~1.3-1.5x faster for common "fast paths" measured under a Blazor client app. However, when run under CorClr+Windows, there is a much larger ~2-3x gain. The difference appears to be additional overhead in Mono interpreter lambda methods with variable capture. For NativeAOT, this PR is expected to make CreateFactory also around ~1.5x faster based on reflection methods being 1.3x-1.7x faster with the new zero-alloc APIs.
Background:
Note the reflection path is used (vs. Expressions) when RuntimeFeature.IsDynamicCodeCompiled == false which will be the case for Blazor client and NativeAOT . For NativeAOT, using expressions would cause them to be interpreted for NativeAOT which is very slow, and for Blazor using expressions would bring along the very large System.Linq.Expressions assembly where a smaller (trimmed) size is important. Note that this PR does not change the semantics here (that was done previously -- see the links above); only the CPU performance is improved.
@stevehartersteveharter
self-assigned this
Jul 27, 2023
@ghost
ghost
commented
Jul 27, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection

See info in area-owners.md if you want to be subscribed.
Issue Details
[verifying tests]
Author:steveharter
Assignees:steveharter
Labels:
tenet-performance, area-Extensions-DependencyInjection
Milestone:-
@stevehartersteveharter
mentioned this pull request
Jul 27, 2023

Closed

6 tasks
@stevehartersteveharter
added this to the 8.0.0 milestone
Jul 27, 2023
@steveharter
steveharter
requested a review
from buyaa-nJuly 27, 2023 16:58
@steveharter
steveharter
marked this pull request as ready for review
July 27, 2023 17:09
steveharter
steveharter
commented
Jul 27, 2023
View reviewed changes
Comment thread...libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ActivatorUtilitiesTests.cs

 [Fact]
 public void CreateFactory_CreatesFactoryMethod()
 public void CreateFactory_CreatesFactoryMethod_4Types_3Injected()
@stevehartersteveharterJul 27, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This new test was added for coverage; the other new code paths already had coverage.
eerhardt
eerhardt
reviewed
Jul 27, 2023
View reviewed changes
@eerhardteerhardt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Do we have any perf numbers (both throughput and size) for this change?
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment thread...s/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.Reflection.cs
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
Jul 27, 2023

Closed

davidfowl
davidfowl
reviewed
Aug 1, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
 if (serviceProvider is null)
 ThrowHelperArgumentNullExceptionServiceProvider();

 object?[] constructorArguments = new object?[parameters.Length];
@davidfowldavidfowlAug 1, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Not saying we should but we could pool this array now that we have Span support.
@steveharter
steveharter
commented
Aug 1, 2023

Copy link


Copy Markdown

ContributorAuthor
Do we have any perf numbers (both throughput and size) for this change?
See #85065 for the previous size gains of removing Linq.Expressions assembly (went from ~260k+ to 4k) when ActivatorUtilities.CreateFactory is used.
See #66153 which has additional details on CPU gains. Reflection APIs are much faster in general, and the DI CreateFactory is ~1.5x faster depending on the scenario, but still slower than using Linq expressions. We could investigate further as the Blazor gains were not as much as the Windows gains, percentage-wise.
buyaa-n
buyaa-n
reviewed
Aug 2, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs

Comment on lines

+731
 to 
+732

 if (serviceProvider is null)
 ThrowHelperArgumentNullExceptionServiceProvider();
@buyaa-nbuyaa-nAug 2, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

NIT: Since it is within .NET 8 if-def could we use ArgumentNullException.ThrowIfNull(serviceProvider)? Here and 4 similar cases below
@stevehartersteveharterAug 3, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yeah there was an earlier discussion around this - it's at least consistent now across all cases using the helper method. In one case, under .NET 8, we couldn't use ThrowIfNull due to a compile warning around unused variable (but we still want to verify for consistency) and of course under .NetStandard.NetFx we can't use ThrowIfNull at all.
buyaa-n
buyaa-n
approved these changes
Aug 2, 2023
View reviewed changes
@buyaa-nbuyaa-n
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Left a NIT, overall LGTM thanks!
@steveharter
steveharter
merged commit c416966
into
dotnet:mainAug 3, 2023
@steveharter
steveharter
deleted the

DiPerf

branch
August 3, 2023 12:31
@jkotas
jkotas
commented
Aug 4, 2023

Copy link


Copy Markdown

Member
This change badly broke DI for native AOT. Number of DI tests are failing on native AOT in this repo, and it soon going to affect ASP.NET and perflab. I think we need to revert this change.
Example of failure: https://helixre107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-pull-89969-merge-ce136d5e93d548e1a8/Microsoft.Extensions.Http.Tests/1/console.85a149a2.log?helixlogtype=result
[FAIL] Microsoft.Extensions.DependencyInjection.HttpClientFactoryServiceCollectionExtensionsTest.AddHttpClient_MessageHandler_Scope_SingletonDependency
System.Reflection.TargetParameterCountException : Parameter count mismatch.
at System.Reflection.DynamicInvokeInfo.ThrowForArgCountMismatch() + 0x7c
at System.Reflection.DynamicInvokeInfo.InvokeDirectWithFewArgs(Object, IntPtr, Span`1) + 0x1e8
at Internal.Reflection.Execution.MethodInvokers.InstanceMethodInvoker.InvokeDirectWithFewArgs(Object, Span`1) + 0x39
at Internal.Reflection.Execution.MethodInvokers.InstanceMethodInvoker.CreateInstanceWithFewArgs(Span`1) + 0x28
at System.Reflection.ConstructorInvoker.Invoke(Object, Object, Object, Object) + 0x5a
at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.ReflectionFactoryCanonicalFixed(ConstructorInvoker, ActivatorUtilities.FactoryParameterContext[], Type, IServiceProvider, Object[]) + 0x2f9
at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.<>c__DisplayClass14_2.<CreateFactoryReflection>b__7(IServiceProvider serviceProvider, Object[] arguments) + 0x27
at Microsoft.Extensions.Http.DefaultTypedHttpClientFactory`1.CreateClient(HttpClient) + 0x59
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitDisposeCache(ServiceCallSite, RuntimeResolverContext) + 0xe
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSite(ServiceCallSite callSite, TArgument argument) + 0xb5
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.Resolve(ServiceCallSite, ServiceProviderEngineScope) + 0x3d
at Microsoft.Extensions.DependencyInjection.ServiceProvider.GetService(ServiceIdentifier, ServiceProviderEngineScope) + 0xa3
at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider, Type) + 0x99
at Microsoft.Extensions.DependencyInjection.HttpClientFactoryServiceCollectionExtensionsTest.<AddHttpClient_MessageHandler_Scope_SingletonDependency>d__39.MoveNext() + 0x2b4

jkotas added a commit
that referenced
this pull request
Aug 4, 2023
@jkotas
Revert "For perf, use the new ConstructorInvoker APIs for ActivatorUt…
87f09e1
…ilities.CreateFactory (#89573)"
This reverts commit c416966.
@jkotasjkotas
mentioned this pull request
Aug 4, 2023
 Merged

AaronRobinsonMSFT pushed a commit
that referenced
this pull request
Aug 4, 2023
@jkotas
Revert "For perf, use the new ConstructorInvoker APIs for ActivatorUt…
ba196ce
…ilities.CreateFactory (#89573)" (#89992)
This reverts commit c416966.
@stevehartersteveharter
mentioned this pull request
Aug 7, 2023
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Sep 3, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@davidfowldavidfowldavidfowl left review comments
@eerhardteerhardteerhardt left review comments

+2 more reviewers

@IDisposableIDisposableIDisposable left review comments
@buyaa-nbuyaa-nbuyaa-n approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@stevehartersteveharter

Labels

area-Extensions-DependencyInjectiontenet-performancePerformance related issue

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

Consider updating ActivatorUtilities.CreateFactory to use ILEmit when possible


6 participants

@steveharter@jkotas@davidfowl@IDisposable@eerhardt@buyaa-n

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
For perf, use the new ConstructorInvoker APIs for ActivatorUtilities.CreateFactory by steveharter · Pull Request #89573 · dotnet/runtime · GitHub
Skip to content
For perf, use the new ConstructorInvoker APIs for ActivatorUtilities.CreateFactory - #89573
Merged
steveharter merged 6 commits into
dotnet:mainfrom 
steveharter:DiPerf
Aug 3, 2023
Merged
For perf, use the new ConstructorInvoker APIs for ActivatorUtilities.CreateFactory#89573
steveharter merged 6 commits into
dotnet:mainfrom 
steveharter:DiPerf
Conversation
@steveharter
@stevehartersteveharter
commented
Jul 27, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes#66153 which also contains additional information on benchmarks here.
This PR changes DI to use the new zero-alloc invoke APIs and is expected to close out the DI+Blazor perf work for v8 -- Blazor apps can use DI ActivatorUtilities.CreateFactory without bringing in the large System.Linq.Expressions assembly and in a performant manner (but not quite as fast as having Blazor use Linq Expressions instead of reflection).
In summary, for Blazor, this PR makes CreateFactory ~1.3-1.5x faster for common "fast paths" measured under a Blazor client app. However, when run under CorClr+Windows, there is a much larger ~2-3x gain. The difference appears to be additional overhead in Mono interpreter lambda methods with variable capture. For NativeAOT, this PR is expected to make CreateFactory also around ~1.5x faster based on reflection methods being 1.3x-1.7x faster with the new zero-alloc APIs.
Background:
Note the reflection path is used (vs. Expressions) when RuntimeFeature.IsDynamicCodeCompiled == false which will be the case for Blazor client and NativeAOT . For NativeAOT, using expressions would cause them to be interpreted for NativeAOT which is very slow, and for Blazor using expressions would bring along the very large System.Linq.Expressions assembly where a smaller (trimmed) size is important. Note that this PR does not change the semantics here (that was done previously -- see the links above); only the CPU performance is improved.
@stevehartersteveharter
self-assigned this
Jul 27, 2023
@ghost
ghost
commented
Jul 27, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection

See info in area-owners.md if you want to be subscribed.
Issue Details
[verifying tests]
Author:steveharter
Assignees:steveharter
Labels:
tenet-performance, area-Extensions-DependencyInjection
Milestone:-
@stevehartersteveharter
mentioned this pull request
Jul 27, 2023

Closed

6 tasks
@stevehartersteveharter
added this to the 8.0.0 milestone
Jul 27, 2023
@steveharter
steveharter
requested a review
from buyaa-nJuly 27, 2023 16:58
@steveharter
steveharter
marked this pull request as ready for review
July 27, 2023 17:09
steveharter
steveharter
commented
Jul 27, 2023
View reviewed changes
Comment thread...libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ActivatorUtilitiesTests.cs

 [Fact]
 public void CreateFactory_CreatesFactoryMethod()
 public void CreateFactory_CreatesFactoryMethod_4Types_3Injected()
@stevehartersteveharterJul 27, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This new test was added for coverage; the other new code paths already had coverage.
eerhardt
eerhardt
reviewed
Jul 27, 2023
View reviewed changes
@eerhardteerhardt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Do we have any perf numbers (both throughput and size) for this change?
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment thread...s/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.Reflection.cs
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
Jul 27, 2023

Closed

davidfowl
davidfowl
reviewed
Aug 1, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
 if (serviceProvider is null)
 ThrowHelperArgumentNullExceptionServiceProvider();

 object?[] constructorArguments = new object?[parameters.Length];
@davidfowldavidfowlAug 1, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Not saying we should but we could pool this array now that we have Span support.
@steveharter
steveharter
commented
Aug 1, 2023

Copy link


Copy Markdown

ContributorAuthor
Do we have any perf numbers (both throughput and size) for this change?
See #85065 for the previous size gains of removing Linq.Expressions assembly (went from ~260k+ to 4k) when ActivatorUtilities.CreateFactory is used.
See #66153 which has additional details on CPU gains. Reflection APIs are much faster in general, and the DI CreateFactory is ~1.5x faster depending on the scenario, but still slower than using Linq expressions. We could investigate further as the Blazor gains were not as much as the Windows gains, percentage-wise.
buyaa-n
buyaa-n
reviewed
Aug 2, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs

Comment on lines

+731
 to 
+732

 if (serviceProvider is null)
 ThrowHelperArgumentNullExceptionServiceProvider();
@buyaa-nbuyaa-nAug 2, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

NIT: Since it is within .NET 8 if-def could we use ArgumentNullException.ThrowIfNull(serviceProvider)? Here and 4 similar cases below
@stevehartersteveharterAug 3, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yeah there was an earlier discussion around this - it's at least consistent now across all cases using the helper method. In one case, under .NET 8, we couldn't use ThrowIfNull due to a compile warning around unused variable (but we still want to verify for consistency) and of course under .NetStandard.NetFx we can't use ThrowIfNull at all.
buyaa-n
buyaa-n
approved these changes
Aug 2, 2023
View reviewed changes
@buyaa-nbuyaa-n
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Left a NIT, overall LGTM thanks!
@steveharter
steveharter
merged commit c416966
into
dotnet:mainAug 3, 2023
@steveharter
steveharter
deleted the

DiPerf

branch
August 3, 2023 12:31
@jkotas
jkotas
commented
Aug 4, 2023

Copy link


Copy Markdown

Member
This change badly broke DI for native AOT. Number of DI tests are failing on native AOT in this repo, and it soon going to affect ASP.NET and perflab. I think we need to revert this change.
Example of failure: https://helixre107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-pull-89969-merge-ce136d5e93d548e1a8/Microsoft.Extensions.Http.Tests/1/console.85a149a2.log?helixlogtype=result
[FAIL] Microsoft.Extensions.DependencyInjection.HttpClientFactoryServiceCollectionExtensionsTest.AddHttpClient_MessageHandler_Scope_SingletonDependency
System.Reflection.TargetParameterCountException : Parameter count mismatch.
at System.Reflection.DynamicInvokeInfo.ThrowForArgCountMismatch() + 0x7c
at System.Reflection.DynamicInvokeInfo.InvokeDirectWithFewArgs(Object, IntPtr, Span`1) + 0x1e8
at Internal.Reflection.Execution.MethodInvokers.InstanceMethodInvoker.InvokeDirectWithFewArgs(Object, Span`1) + 0x39
at Internal.Reflection.Execution.MethodInvokers.InstanceMethodInvoker.CreateInstanceWithFewArgs(Span`1) + 0x28
at System.Reflection.ConstructorInvoker.Invoke(Object, Object, Object, Object) + 0x5a
at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.ReflectionFactoryCanonicalFixed(ConstructorInvoker, ActivatorUtilities.FactoryParameterContext[], Type, IServiceProvider, Object[]) + 0x2f9
at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.<>c__DisplayClass14_2.<CreateFactoryReflection>b__7(IServiceProvider serviceProvider, Object[] arguments) + 0x27
at Microsoft.Extensions.Http.DefaultTypedHttpClientFactory`1.CreateClient(HttpClient) + 0x59
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitDisposeCache(ServiceCallSite, RuntimeResolverContext) + 0xe
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSite(ServiceCallSite callSite, TArgument argument) + 0xb5
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.Resolve(ServiceCallSite, ServiceProviderEngineScope) + 0x3d
at Microsoft.Extensions.DependencyInjection.ServiceProvider.GetService(ServiceIdentifier, ServiceProviderEngineScope) + 0xa3
at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider, Type) + 0x99
at Microsoft.Extensions.DependencyInjection.HttpClientFactoryServiceCollectionExtensionsTest.<AddHttpClient_MessageHandler_Scope_SingletonDependency>d__39.MoveNext() + 0x2b4

jkotas added a commit
that referenced
this pull request
Aug 4, 2023
@jkotas
Revert "For perf, use the new ConstructorInvoker APIs for ActivatorUt…
87f09e1
…ilities.CreateFactory (#89573)"
This reverts commit c416966.
@jkotasjkotas
mentioned this pull request
Aug 4, 2023
 Merged

AaronRobinsonMSFT pushed a commit
that referenced
this pull request
Aug 4, 2023
@jkotas
Revert "For perf, use the new ConstructorInvoker APIs for ActivatorUt…
ba196ce
…ilities.CreateFactory (#89573)" (#89992)
This reverts commit c416966.
@stevehartersteveharter
mentioned this pull request
Aug 7, 2023
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Sep 3, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@davidfowldavidfowldavidfowl left review comments
@eerhardteerhardteerhardt left review comments

+2 more reviewers

@IDisposableIDisposableIDisposable left review comments
@buyaa-nbuyaa-nbuyaa-n approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@stevehartersteveharter

Labels

area-Extensions-DependencyInjectiontenet-performancePerformance related issue

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

Consider updating ActivatorUtilities.CreateFactory to use ILEmit when possible


6 participants

@steveharter@jkotas@davidfowl@IDisposable@eerhardt@buyaa-n

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
For perf, use the new ConstructorInvoker APIs for ActivatorUtilities.CreateFactory by steveharter · Pull Request #89573 · dotnet/runtime · GitHub
Skip to content
For perf, use the new ConstructorInvoker APIs for ActivatorUtilities.CreateFactory - #89573
Merged
steveharter merged 6 commits into
dotnet:mainfrom 
steveharter:DiPerf
Aug 3, 2023
Merged
For perf, use the new ConstructorInvoker APIs for ActivatorUtilities.CreateFactory#89573
steveharter merged 6 commits into
dotnet:mainfrom 
steveharter:DiPerf
Conversation
@steveharter
@stevehartersteveharter
commented
Jul 27, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes#66153 which also contains additional information on benchmarks here.
This PR changes DI to use the new zero-alloc invoke APIs and is expected to close out the DI+Blazor perf work for v8 -- Blazor apps can use DI ActivatorUtilities.CreateFactory without bringing in the large System.Linq.Expressions assembly and in a performant manner (but not quite as fast as having Blazor use Linq Expressions instead of reflection).
In summary, for Blazor, this PR makes CreateFactory ~1.3-1.5x faster for common "fast paths" measured under a Blazor client app. However, when run under CorClr+Windows, there is a much larger ~2-3x gain. The difference appears to be additional overhead in Mono interpreter lambda methods with variable capture. For NativeAOT, this PR is expected to make CreateFactory also around ~1.5x faster based on reflection methods being 1.3x-1.7x faster with the new zero-alloc APIs.
Background:
Note the reflection path is used (vs. Expressions) when RuntimeFeature.IsDynamicCodeCompiled == false which will be the case for Blazor client and NativeAOT . For NativeAOT, using expressions would cause them to be interpreted for NativeAOT which is very slow, and for Blazor using expressions would bring along the very large System.Linq.Expressions assembly where a smaller (trimmed) size is important. Note that this PR does not change the semantics here (that was done previously -- see the links above); only the CPU performance is improved.
@stevehartersteveharter
self-assigned this
Jul 27, 2023
@ghost
ghost
commented
Jul 27, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection

See info in area-owners.md if you want to be subscribed.
Issue Details
[verifying tests]
Author:steveharter
Assignees:steveharter
Labels:
tenet-performance, area-Extensions-DependencyInjection
Milestone:-
@stevehartersteveharter
mentioned this pull request
Jul 27, 2023

Closed

6 tasks
@stevehartersteveharter
added this to the 8.0.0 milestone
Jul 27, 2023
@steveharter
steveharter
requested a review
from buyaa-nJuly 27, 2023 16:58
@steveharter
steveharter
marked this pull request as ready for review
July 27, 2023 17:09
steveharter
steveharter
commented
Jul 27, 2023
View reviewed changes
Comment thread...libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ActivatorUtilitiesTests.cs

 [Fact]
 public void CreateFactory_CreatesFactoryMethod()
 public void CreateFactory_CreatesFactoryMethod_4Types_3Injected()
@stevehartersteveharterJul 27, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This new test was added for coverage; the other new code paths already had coverage.
eerhardt
eerhardt
reviewed
Jul 27, 2023
View reviewed changes
@eerhardteerhardt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Do we have any perf numbers (both throughput and size) for this change?
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment thread...s/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.Reflection.cs
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
Jul 27, 2023

Closed

davidfowl
davidfowl
reviewed
Aug 1, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
 if (serviceProvider is null)
 ThrowHelperArgumentNullExceptionServiceProvider();

 object?[] constructorArguments = new object?[parameters.Length];
@davidfowldavidfowlAug 1, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Not saying we should but we could pool this array now that we have Span support.
@steveharter
steveharter
commented
Aug 1, 2023

Copy link


Copy Markdown

ContributorAuthor
Do we have any perf numbers (both throughput and size) for this change?
See #85065 for the previous size gains of removing Linq.Expressions assembly (went from ~260k+ to 4k) when ActivatorUtilities.CreateFactory is used.
See #66153 which has additional details on CPU gains. Reflection APIs are much faster in general, and the DI CreateFactory is ~1.5x faster depending on the scenario, but still slower than using Linq expressions. We could investigate further as the Blazor gains were not as much as the Windows gains, percentage-wise.
buyaa-n
buyaa-n
reviewed
Aug 2, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs

Comment on lines

+731
 to 
+732

 if (serviceProvider is null)
 ThrowHelperArgumentNullExceptionServiceProvider();
@buyaa-nbuyaa-nAug 2, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

NIT: Since it is within .NET 8 if-def could we use ArgumentNullException.ThrowIfNull(serviceProvider)? Here and 4 similar cases below
@stevehartersteveharterAug 3, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yeah there was an earlier discussion around this - it's at least consistent now across all cases using the helper method. In one case, under .NET 8, we couldn't use ThrowIfNull due to a compile warning around unused variable (but we still want to verify for consistency) and of course under .NetStandard.NetFx we can't use ThrowIfNull at all.
buyaa-n
buyaa-n
approved these changes
Aug 2, 2023
View reviewed changes
@buyaa-nbuyaa-n
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Left a NIT, overall LGTM thanks!
@steveharter
steveharter
merged commit c416966
into
dotnet:mainAug 3, 2023
@steveharter
steveharter
deleted the

DiPerf

branch
August 3, 2023 12:31
@jkotas
jkotas
commented
Aug 4, 2023

Copy link


Copy Markdown

Member
This change badly broke DI for native AOT. Number of DI tests are failing on native AOT in this repo, and it soon going to affect ASP.NET and perflab. I think we need to revert this change.
Example of failure: https://helixre107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-pull-89969-merge-ce136d5e93d548e1a8/Microsoft.Extensions.Http.Tests/1/console.85a149a2.log?helixlogtype=result
[FAIL] Microsoft.Extensions.DependencyInjection.HttpClientFactoryServiceCollectionExtensionsTest.AddHttpClient_MessageHandler_Scope_SingletonDependency
System.Reflection.TargetParameterCountException : Parameter count mismatch.
at System.Reflection.DynamicInvokeInfo.ThrowForArgCountMismatch() + 0x7c
at System.Reflection.DynamicInvokeInfo.InvokeDirectWithFewArgs(Object, IntPtr, Span`1) + 0x1e8
at Internal.Reflection.Execution.MethodInvokers.InstanceMethodInvoker.InvokeDirectWithFewArgs(Object, Span`1) + 0x39
at Internal.Reflection.Execution.MethodInvokers.InstanceMethodInvoker.CreateInstanceWithFewArgs(Span`1) + 0x28
at System.Reflection.ConstructorInvoker.Invoke(Object, Object, Object, Object) + 0x5a
at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.ReflectionFactoryCanonicalFixed(ConstructorInvoker, ActivatorUtilities.FactoryParameterContext[], Type, IServiceProvider, Object[]) + 0x2f9
at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.<>c__DisplayClass14_2.<CreateFactoryReflection>b__7(IServiceProvider serviceProvider, Object[] arguments) + 0x27
at Microsoft.Extensions.Http.DefaultTypedHttpClientFactory`1.CreateClient(HttpClient) + 0x59
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitDisposeCache(ServiceCallSite, RuntimeResolverContext) + 0xe
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSite(ServiceCallSite callSite, TArgument argument) + 0xb5
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.Resolve(ServiceCallSite, ServiceProviderEngineScope) + 0x3d
at Microsoft.Extensions.DependencyInjection.ServiceProvider.GetService(ServiceIdentifier, ServiceProviderEngineScope) + 0xa3
at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider, Type) + 0x99
at Microsoft.Extensions.DependencyInjection.HttpClientFactoryServiceCollectionExtensionsTest.<AddHttpClient_MessageHandler_Scope_SingletonDependency>d__39.MoveNext() + 0x2b4

jkotas added a commit
that referenced
this pull request
Aug 4, 2023
@jkotas
Revert "For perf, use the new ConstructorInvoker APIs for ActivatorUt…
87f09e1
…ilities.CreateFactory (#89573)"
This reverts commit c416966.
@jkotasjkotas
mentioned this pull request
Aug 4, 2023
 Merged

AaronRobinsonMSFT pushed a commit
that referenced
this pull request
Aug 4, 2023
@jkotas
Revert "For perf, use the new ConstructorInvoker APIs for ActivatorUt…
ba196ce
…ilities.CreateFactory (#89573)" (#89992)
This reverts commit c416966.
@stevehartersteveharter
mentioned this pull request
Aug 7, 2023
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Sep 3, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@davidfowldavidfowldavidfowl left review comments
@eerhardteerhardteerhardt left review comments

+2 more reviewers

@IDisposableIDisposableIDisposable left review comments
@buyaa-nbuyaa-nbuyaa-n approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@stevehartersteveharter

Labels

area-Extensions-DependencyInjectiontenet-performancePerformance related issue

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

Consider updating ActivatorUtilities.CreateFactory to use ILEmit when possible


6 participants

@steveharter@jkotas@davidfowl@IDisposable@eerhardt@buyaa-n

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
For perf, use the new ConstructorInvoker APIs for ActivatorUtilities.CreateFactory by steveharter · Pull Request #89573 · dotnet/runtime · GitHub
Skip to content
For perf, use the new ConstructorInvoker APIs for ActivatorUtilities.CreateFactory - #89573
Merged
steveharter merged 6 commits into
dotnet:mainfrom 
steveharter:DiPerf
Aug 3, 2023
Merged
For perf, use the new ConstructorInvoker APIs for ActivatorUtilities.CreateFactory#89573
steveharter merged 6 commits into
dotnet:mainfrom 
steveharter:DiPerf
Conversation
@steveharter
@stevehartersteveharter
commented
Jul 27, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes#66153 which also contains additional information on benchmarks here.
This PR changes DI to use the new zero-alloc invoke APIs and is expected to close out the DI+Blazor perf work for v8 -- Blazor apps can use DI ActivatorUtilities.CreateFactory without bringing in the large System.Linq.Expressions assembly and in a performant manner (but not quite as fast as having Blazor use Linq Expressions instead of reflection).
In summary, for Blazor, this PR makes CreateFactory ~1.3-1.5x faster for common "fast paths" measured under a Blazor client app. However, when run under CorClr+Windows, there is a much larger ~2-3x gain. The difference appears to be additional overhead in Mono interpreter lambda methods with variable capture. For NativeAOT, this PR is expected to make CreateFactory also around ~1.5x faster based on reflection methods being 1.3x-1.7x faster with the new zero-alloc APIs.
Background:
Note the reflection path is used (vs. Expressions) when RuntimeFeature.IsDynamicCodeCompiled == false which will be the case for Blazor client and NativeAOT . For NativeAOT, using expressions would cause them to be interpreted for NativeAOT which is very slow, and for Blazor using expressions would bring along the very large System.Linq.Expressions assembly where a smaller (trimmed) size is important. Note that this PR does not change the semantics here (that was done previously -- see the links above); only the CPU performance is improved.
@stevehartersteveharter
self-assigned this
Jul 27, 2023
@ghost
ghost
commented
Jul 27, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection

See info in area-owners.md if you want to be subscribed.
Issue Details
[verifying tests]
Author:steveharter
Assignees:steveharter
Labels:
tenet-performance, area-Extensions-DependencyInjection
Milestone:-
@stevehartersteveharter
mentioned this pull request
Jul 27, 2023

Closed

6 tasks
@stevehartersteveharter
added this to the 8.0.0 milestone
Jul 27, 2023
@steveharter
steveharter
requested a review
from buyaa-nJuly 27, 2023 16:58
@steveharter
steveharter
marked this pull request as ready for review
July 27, 2023 17:09
steveharter
steveharter
commented
Jul 27, 2023
View reviewed changes
Comment thread...libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ActivatorUtilitiesTests.cs

 [Fact]
 public void CreateFactory_CreatesFactoryMethod()
 public void CreateFactory_CreatesFactoryMethod_4Types_3Injected()
@stevehartersteveharterJul 27, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This new test was added for coverage; the other new code paths already had coverage.
eerhardt
eerhardt
reviewed
Jul 27, 2023
View reviewed changes
@eerhardteerhardt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Do we have any perf numbers (both throughput and size) for this change?
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment thread...s/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.Reflection.cs
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
Jul 27, 2023

Closed

davidfowl
davidfowl
reviewed
Aug 1, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
 if (serviceProvider is null)
 ThrowHelperArgumentNullExceptionServiceProvider();

 object?[] constructorArguments = new object?[parameters.Length];
@davidfowldavidfowlAug 1, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Not saying we should but we could pool this array now that we have Span support.
@steveharter
steveharter
commented
Aug 1, 2023

Copy link


Copy Markdown

ContributorAuthor
Do we have any perf numbers (both throughput and size) for this change?
See #85065 for the previous size gains of removing Linq.Expressions assembly (went from ~260k+ to 4k) when ActivatorUtilities.CreateFactory is used.
See #66153 which has additional details on CPU gains. Reflection APIs are much faster in general, and the DI CreateFactory is ~1.5x faster depending on the scenario, but still slower than using Linq expressions. We could investigate further as the Blazor gains were not as much as the Windows gains, percentage-wise.
buyaa-n
buyaa-n
reviewed
Aug 2, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs

Comment on lines

+731
 to 
+732

 if (serviceProvider is null)
 ThrowHelperArgumentNullExceptionServiceProvider();
@buyaa-nbuyaa-nAug 2, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

NIT: Since it is within .NET 8 if-def could we use ArgumentNullException.ThrowIfNull(serviceProvider)? Here and 4 similar cases below
@stevehartersteveharterAug 3, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yeah there was an earlier discussion around this - it's at least consistent now across all cases using the helper method. In one case, under .NET 8, we couldn't use ThrowIfNull due to a compile warning around unused variable (but we still want to verify for consistency) and of course under .NetStandard.NetFx we can't use ThrowIfNull at all.
buyaa-n
buyaa-n
approved these changes
Aug 2, 2023
View reviewed changes
@buyaa-nbuyaa-n
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Left a NIT, overall LGTM thanks!
@steveharter
steveharter
merged commit c416966
into
dotnet:mainAug 3, 2023
@steveharter
steveharter
deleted the

DiPerf

branch
August 3, 2023 12:31
@jkotas
jkotas
commented
Aug 4, 2023

Copy link


Copy Markdown

Member
This change badly broke DI for native AOT. Number of DI tests are failing on native AOT in this repo, and it soon going to affect ASP.NET and perflab. I think we need to revert this change.
Example of failure: https://helixre107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-pull-89969-merge-ce136d5e93d548e1a8/Microsoft.Extensions.Http.Tests/1/console.85a149a2.log?helixlogtype=result
[FAIL] Microsoft.Extensions.DependencyInjection.HttpClientFactoryServiceCollectionExtensionsTest.AddHttpClient_MessageHandler_Scope_SingletonDependency
System.Reflection.TargetParameterCountException : Parameter count mismatch.
at System.Reflection.DynamicInvokeInfo.ThrowForArgCountMismatch() + 0x7c
at System.Reflection.DynamicInvokeInfo.InvokeDirectWithFewArgs(Object, IntPtr, Span`1) + 0x1e8
at Internal.Reflection.Execution.MethodInvokers.InstanceMethodInvoker.InvokeDirectWithFewArgs(Object, Span`1) + 0x39
at Internal.Reflection.Execution.MethodInvokers.InstanceMethodInvoker.CreateInstanceWithFewArgs(Span`1) + 0x28
at System.Reflection.ConstructorInvoker.Invoke(Object, Object, Object, Object) + 0x5a
at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.ReflectionFactoryCanonicalFixed(ConstructorInvoker, ActivatorUtilities.FactoryParameterContext[], Type, IServiceProvider, Object[]) + 0x2f9
at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.<>c__DisplayClass14_2.<CreateFactoryReflection>b__7(IServiceProvider serviceProvider, Object[] arguments) + 0x27
at Microsoft.Extensions.Http.DefaultTypedHttpClientFactory`1.CreateClient(HttpClient) + 0x59
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitDisposeCache(ServiceCallSite, RuntimeResolverContext) + 0xe
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSite(ServiceCallSite callSite, TArgument argument) + 0xb5
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.Resolve(ServiceCallSite, ServiceProviderEngineScope) + 0x3d
at Microsoft.Extensions.DependencyInjection.ServiceProvider.GetService(ServiceIdentifier, ServiceProviderEngineScope) + 0xa3
at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider, Type) + 0x99
at Microsoft.Extensions.DependencyInjection.HttpClientFactoryServiceCollectionExtensionsTest.<AddHttpClient_MessageHandler_Scope_SingletonDependency>d__39.MoveNext() + 0x2b4

jkotas added a commit
that referenced
this pull request
Aug 4, 2023
@jkotas
Revert "For perf, use the new ConstructorInvoker APIs for ActivatorUt…
87f09e1
…ilities.CreateFactory (#89573)"
This reverts commit c416966.
@jkotasjkotas
mentioned this pull request
Aug 4, 2023
 Merged

AaronRobinsonMSFT pushed a commit
that referenced
this pull request
Aug 4, 2023
@jkotas
Revert "For perf, use the new ConstructorInvoker APIs for ActivatorUt…
ba196ce
…ilities.CreateFactory (#89573)" (#89992)
This reverts commit c416966.
@stevehartersteveharter
mentioned this pull request
Aug 7, 2023
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Sep 3, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@davidfowldavidfowldavidfowl left review comments
@eerhardteerhardteerhardt left review comments

+2 more reviewers

@IDisposableIDisposableIDisposable left review comments
@buyaa-nbuyaa-nbuyaa-n approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@stevehartersteveharter

Labels

area-Extensions-DependencyInjectiontenet-performancePerformance related issue

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

Consider updating ActivatorUtilities.CreateFactory to use ILEmit when possible


6 participants

@steveharter@jkotas@davidfowl@IDisposable@eerhardt@buyaa-n

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
For perf, use the new ConstructorInvoker APIs for ActivatorUtilities.CreateFactory by steveharter · Pull Request #89573 · dotnet/runtime · GitHub
Skip to content
For perf, use the new ConstructorInvoker APIs for ActivatorUtilities.CreateFactory - #89573
Merged
steveharter merged 6 commits into
dotnet:mainfrom 
steveharter:DiPerf
Aug 3, 2023
Merged
For perf, use the new ConstructorInvoker APIs for ActivatorUtilities.CreateFactory#89573
steveharter merged 6 commits into
dotnet:mainfrom 
steveharter:DiPerf
Conversation
@steveharter
@stevehartersteveharter
commented
Jul 27, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes#66153 which also contains additional information on benchmarks here.
This PR changes DI to use the new zero-alloc invoke APIs and is expected to close out the DI+Blazor perf work for v8 -- Blazor apps can use DI ActivatorUtilities.CreateFactory without bringing in the large System.Linq.Expressions assembly and in a performant manner (but not quite as fast as having Blazor use Linq Expressions instead of reflection).
In summary, for Blazor, this PR makes CreateFactory ~1.3-1.5x faster for common "fast paths" measured under a Blazor client app. However, when run under CorClr+Windows, there is a much larger ~2-3x gain. The difference appears to be additional overhead in Mono interpreter lambda methods with variable capture. For NativeAOT, this PR is expected to make CreateFactory also around ~1.5x faster based on reflection methods being 1.3x-1.7x faster with the new zero-alloc APIs.
Background:
Note the reflection path is used (vs. Expressions) when RuntimeFeature.IsDynamicCodeCompiled == false which will be the case for Blazor client and NativeAOT . For NativeAOT, using expressions would cause them to be interpreted for NativeAOT which is very slow, and for Blazor using expressions would bring along the very large System.Linq.Expressions assembly where a smaller (trimmed) size is important. Note that this PR does not change the semantics here (that was done previously -- see the links above); only the CPU performance is improved.
@stevehartersteveharter
self-assigned this
Jul 27, 2023
@ghost
ghost
commented
Jul 27, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection

See info in area-owners.md if you want to be subscribed.
Issue Details
[verifying tests]
Author:steveharter
Assignees:steveharter
Labels:
tenet-performance, area-Extensions-DependencyInjection
Milestone:-
@stevehartersteveharter
mentioned this pull request
Jul 27, 2023

Closed

6 tasks
@stevehartersteveharter
added this to the 8.0.0 milestone
Jul 27, 2023
@steveharter
steveharter
requested a review
from buyaa-nJuly 27, 2023 16:58
@steveharter
steveharter
marked this pull request as ready for review
July 27, 2023 17:09
steveharter
steveharter
commented
Jul 27, 2023
View reviewed changes
Comment thread...libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ActivatorUtilitiesTests.cs

 [Fact]
 public void CreateFactory_CreatesFactoryMethod()
 public void CreateFactory_CreatesFactoryMethod_4Types_3Injected()
@stevehartersteveharterJul 27, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This new test was added for coverage; the other new code paths already had coverage.
eerhardt
eerhardt
reviewed
Jul 27, 2023
View reviewed changes
@eerhardteerhardt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Do we have any perf numbers (both throughput and size) for this change?
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment thread...s/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.Reflection.cs
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
Jul 27, 2023

Closed

davidfowl
davidfowl
reviewed
Aug 1, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
 if (serviceProvider is null)
 ThrowHelperArgumentNullExceptionServiceProvider();

 object?[] constructorArguments = new object?[parameters.Length];
@davidfowldavidfowlAug 1, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Not saying we should but we could pool this array now that we have Span support.
@steveharter
steveharter
commented
Aug 1, 2023

Copy link


Copy Markdown

ContributorAuthor
Do we have any perf numbers (both throughput and size) for this change?
See #85065 for the previous size gains of removing Linq.Expressions assembly (went from ~260k+ to 4k) when ActivatorUtilities.CreateFactory is used.
See #66153 which has additional details on CPU gains. Reflection APIs are much faster in general, and the DI CreateFactory is ~1.5x faster depending on the scenario, but still slower than using Linq expressions. We could investigate further as the Blazor gains were not as much as the Windows gains, percentage-wise.
buyaa-n
buyaa-n
reviewed
Aug 2, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs

Comment on lines

+731
 to 
+732

 if (serviceProvider is null)
 ThrowHelperArgumentNullExceptionServiceProvider();
@buyaa-nbuyaa-nAug 2, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

NIT: Since it is within .NET 8 if-def could we use ArgumentNullException.ThrowIfNull(serviceProvider)? Here and 4 similar cases below
@stevehartersteveharterAug 3, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yeah there was an earlier discussion around this - it's at least consistent now across all cases using the helper method. In one case, under .NET 8, we couldn't use ThrowIfNull due to a compile warning around unused variable (but we still want to verify for consistency) and of course under .NetStandard.NetFx we can't use ThrowIfNull at all.
buyaa-n
buyaa-n
approved these changes
Aug 2, 2023
View reviewed changes
@buyaa-nbuyaa-n
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Left a NIT, overall LGTM thanks!
@steveharter
steveharter
merged commit c416966
into
dotnet:mainAug 3, 2023
@steveharter
steveharter
deleted the

DiPerf

branch
August 3, 2023 12:31
@jkotas
jkotas
commented
Aug 4, 2023

Copy link


Copy Markdown

Member
This change badly broke DI for native AOT. Number of DI tests are failing on native AOT in this repo, and it soon going to affect ASP.NET and perflab. I think we need to revert this change.
Example of failure: https://helixre107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-pull-89969-merge-ce136d5e93d548e1a8/Microsoft.Extensions.Http.Tests/1/console.85a149a2.log?helixlogtype=result
[FAIL] Microsoft.Extensions.DependencyInjection.HttpClientFactoryServiceCollectionExtensionsTest.AddHttpClient_MessageHandler_Scope_SingletonDependency
System.Reflection.TargetParameterCountException : Parameter count mismatch.
at System.Reflection.DynamicInvokeInfo.ThrowForArgCountMismatch() + 0x7c
at System.Reflection.DynamicInvokeInfo.InvokeDirectWithFewArgs(Object, IntPtr, Span`1) + 0x1e8
at Internal.Reflection.Execution.MethodInvokers.InstanceMethodInvoker.InvokeDirectWithFewArgs(Object, Span`1) + 0x39
at Internal.Reflection.Execution.MethodInvokers.InstanceMethodInvoker.CreateInstanceWithFewArgs(Span`1) + 0x28
at System.Reflection.ConstructorInvoker.Invoke(Object, Object, Object, Object) + 0x5a
at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.ReflectionFactoryCanonicalFixed(ConstructorInvoker, ActivatorUtilities.FactoryParameterContext[], Type, IServiceProvider, Object[]) + 0x2f9
at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.<>c__DisplayClass14_2.<CreateFactoryReflection>b__7(IServiceProvider serviceProvider, Object[] arguments) + 0x27
at Microsoft.Extensions.Http.DefaultTypedHttpClientFactory`1.CreateClient(HttpClient) + 0x59
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitDisposeCache(ServiceCallSite, RuntimeResolverContext) + 0xe
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSite(ServiceCallSite callSite, TArgument argument) + 0xb5
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.Resolve(ServiceCallSite, ServiceProviderEngineScope) + 0x3d
at Microsoft.Extensions.DependencyInjection.ServiceProvider.GetService(ServiceIdentifier, ServiceProviderEngineScope) + 0xa3
at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider, Type) + 0x99
at Microsoft.Extensions.DependencyInjection.HttpClientFactoryServiceCollectionExtensionsTest.<AddHttpClient_MessageHandler_Scope_SingletonDependency>d__39.MoveNext() + 0x2b4

jkotas added a commit
that referenced
this pull request
Aug 4, 2023
@jkotas
Revert "For perf, use the new ConstructorInvoker APIs for ActivatorUt…
87f09e1
…ilities.CreateFactory (#89573)"
This reverts commit c416966.
@jkotasjkotas
mentioned this pull request
Aug 4, 2023
 Merged

AaronRobinsonMSFT pushed a commit
that referenced
this pull request
Aug 4, 2023
@jkotas
Revert "For perf, use the new ConstructorInvoker APIs for ActivatorUt…
ba196ce
…ilities.CreateFactory (#89573)" (#89992)
This reverts commit c416966.
@stevehartersteveharter
mentioned this pull request
Aug 7, 2023
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Sep 3, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@davidfowldavidfowldavidfowl left review comments
@eerhardteerhardteerhardt left review comments

+2 more reviewers

@IDisposableIDisposableIDisposable left review comments
@buyaa-nbuyaa-nbuyaa-n approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@stevehartersteveharter

Labels

area-Extensions-DependencyInjectiontenet-performancePerformance related issue

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

Consider updating ActivatorUtilities.CreateFactory to use ILEmit when possible


6 participants

@steveharter@jkotas@davidfowl@IDisposable@eerhardt@buyaa-n

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
For perf, use the new ConstructorInvoker APIs for ActivatorUtilities.CreateFactory by steveharter · Pull Request #89573 · dotnet/runtime · GitHub
Skip to content
For perf, use the new ConstructorInvoker APIs for ActivatorUtilities.CreateFactory - #89573
Merged
steveharter merged 6 commits into
dotnet:mainfrom 
steveharter:DiPerf
Aug 3, 2023
Merged
For perf, use the new ConstructorInvoker APIs for ActivatorUtilities.CreateFactory#89573
steveharter merged 6 commits into
dotnet:mainfrom 
steveharter:DiPerf
Conversation
@steveharter
@stevehartersteveharter
commented
Jul 27, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes#66153 which also contains additional information on benchmarks here.
This PR changes DI to use the new zero-alloc invoke APIs and is expected to close out the DI+Blazor perf work for v8 -- Blazor apps can use DI ActivatorUtilities.CreateFactory without bringing in the large System.Linq.Expressions assembly and in a performant manner (but not quite as fast as having Blazor use Linq Expressions instead of reflection).
In summary, for Blazor, this PR makes CreateFactory ~1.3-1.5x faster for common "fast paths" measured under a Blazor client app. However, when run under CorClr+Windows, there is a much larger ~2-3x gain. The difference appears to be additional overhead in Mono interpreter lambda methods with variable capture. For NativeAOT, this PR is expected to make CreateFactory also around ~1.5x faster based on reflection methods being 1.3x-1.7x faster with the new zero-alloc APIs.
Background:
Note the reflection path is used (vs. Expressions) when RuntimeFeature.IsDynamicCodeCompiled == false which will be the case for Blazor client and NativeAOT . For NativeAOT, using expressions would cause them to be interpreted for NativeAOT which is very slow, and for Blazor using expressions would bring along the very large System.Linq.Expressions assembly where a smaller (trimmed) size is important. Note that this PR does not change the semantics here (that was done previously -- see the links above); only the CPU performance is improved.
@stevehartersteveharter
self-assigned this
Jul 27, 2023
@ghost
ghost
commented
Jul 27, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection

See info in area-owners.md if you want to be subscribed.
Issue Details
[verifying tests]
Author:steveharter
Assignees:steveharter
Labels:
tenet-performance, area-Extensions-DependencyInjection
Milestone:-
@stevehartersteveharter
mentioned this pull request
Jul 27, 2023

Closed

6 tasks
@stevehartersteveharter
added this to the 8.0.0 milestone
Jul 27, 2023
@steveharter
steveharter
requested a review
from buyaa-nJuly 27, 2023 16:58
@steveharter
steveharter
marked this pull request as ready for review
July 27, 2023 17:09
steveharter
steveharter
commented
Jul 27, 2023
View reviewed changes
Comment thread...libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ActivatorUtilitiesTests.cs

 [Fact]
 public void CreateFactory_CreatesFactoryMethod()
 public void CreateFactory_CreatesFactoryMethod_4Types_3Injected()
@stevehartersteveharterJul 27, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This new test was added for coverage; the other new code paths already had coverage.
eerhardt
eerhardt
reviewed
Jul 27, 2023
View reviewed changes
@eerhardteerhardt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Do we have any perf numbers (both throughput and size) for this change?
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment thread...s/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.Reflection.cs
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
Jul 27, 2023

Closed

davidfowl
davidfowl
reviewed
Aug 1, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
 if (serviceProvider is null)
 ThrowHelperArgumentNullExceptionServiceProvider();

 object?[] constructorArguments = new object?[parameters.Length];
@davidfowldavidfowlAug 1, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Not saying we should but we could pool this array now that we have Span support.
@steveharter
steveharter
commented
Aug 1, 2023

Copy link


Copy Markdown

ContributorAuthor
Do we have any perf numbers (both throughput and size) for this change?
See #85065 for the previous size gains of removing Linq.Expressions assembly (went from ~260k+ to 4k) when ActivatorUtilities.CreateFactory is used.
See #66153 which has additional details on CPU gains. Reflection APIs are much faster in general, and the DI CreateFactory is ~1.5x faster depending on the scenario, but still slower than using Linq expressions. We could investigate further as the Blazor gains were not as much as the Windows gains, percentage-wise.
buyaa-n
buyaa-n
reviewed
Aug 2, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs

Comment on lines

+731
 to 
+732

 if (serviceProvider is null)
 ThrowHelperArgumentNullExceptionServiceProvider();
@buyaa-nbuyaa-nAug 2, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

NIT: Since it is within .NET 8 if-def could we use ArgumentNullException.ThrowIfNull(serviceProvider)? Here and 4 similar cases below
@stevehartersteveharterAug 3, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yeah there was an earlier discussion around this - it's at least consistent now across all cases using the helper method. In one case, under .NET 8, we couldn't use ThrowIfNull due to a compile warning around unused variable (but we still want to verify for consistency) and of course under .NetStandard.NetFx we can't use ThrowIfNull at all.
buyaa-n
buyaa-n
approved these changes
Aug 2, 2023
View reviewed changes
@buyaa-nbuyaa-n
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Left a NIT, overall LGTM thanks!
@steveharter
steveharter
merged commit c416966
into
dotnet:mainAug 3, 2023
@steveharter
steveharter
deleted the

DiPerf

branch
August 3, 2023 12:31
@jkotas
jkotas
commented
Aug 4, 2023

Copy link


Copy Markdown

Member
This change badly broke DI for native AOT. Number of DI tests are failing on native AOT in this repo, and it soon going to affect ASP.NET and perflab. I think we need to revert this change.
Example of failure: https://helixre107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-pull-89969-merge-ce136d5e93d548e1a8/Microsoft.Extensions.Http.Tests/1/console.85a149a2.log?helixlogtype=result
[FAIL] Microsoft.Extensions.DependencyInjection.HttpClientFactoryServiceCollectionExtensionsTest.AddHttpClient_MessageHandler_Scope_SingletonDependency
System.Reflection.TargetParameterCountException : Parameter count mismatch.
at System.Reflection.DynamicInvokeInfo.ThrowForArgCountMismatch() + 0x7c
at System.Reflection.DynamicInvokeInfo.InvokeDirectWithFewArgs(Object, IntPtr, Span`1) + 0x1e8
at Internal.Reflection.Execution.MethodInvokers.InstanceMethodInvoker.InvokeDirectWithFewArgs(Object, Span`1) + 0x39
at Internal.Reflection.Execution.MethodInvokers.InstanceMethodInvoker.CreateInstanceWithFewArgs(Span`1) + 0x28
at System.Reflection.ConstructorInvoker.Invoke(Object, Object, Object, Object) + 0x5a
at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.ReflectionFactoryCanonicalFixed(ConstructorInvoker, ActivatorUtilities.FactoryParameterContext[], Type, IServiceProvider, Object[]) + 0x2f9
at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.<>c__DisplayClass14_2.<CreateFactoryReflection>b__7(IServiceProvider serviceProvider, Object[] arguments) + 0x27
at Microsoft.Extensions.Http.DefaultTypedHttpClientFactory`1.CreateClient(HttpClient) + 0x59
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitDisposeCache(ServiceCallSite, RuntimeResolverContext) + 0xe
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSite(ServiceCallSite callSite, TArgument argument) + 0xb5
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.Resolve(ServiceCallSite, ServiceProviderEngineScope) + 0x3d
at Microsoft.Extensions.DependencyInjection.ServiceProvider.GetService(ServiceIdentifier, ServiceProviderEngineScope) + 0xa3
at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider, Type) + 0x99
at Microsoft.Extensions.DependencyInjection.HttpClientFactoryServiceCollectionExtensionsTest.<AddHttpClient_MessageHandler_Scope_SingletonDependency>d__39.MoveNext() + 0x2b4

jkotas added a commit
that referenced
this pull request
Aug 4, 2023
@jkotas
Revert "For perf, use the new ConstructorInvoker APIs for ActivatorUt…
87f09e1
…ilities.CreateFactory (#89573)"
This reverts commit c416966.
@jkotasjkotas
mentioned this pull request
Aug 4, 2023
 Merged

AaronRobinsonMSFT pushed a commit
that referenced
this pull request
Aug 4, 2023
@jkotas
Revert "For perf, use the new ConstructorInvoker APIs for ActivatorUt…
ba196ce
…ilities.CreateFactory (#89573)" (#89992)
This reverts commit c416966.
@stevehartersteveharter
mentioned this pull request
Aug 7, 2023
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Sep 3, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@davidfowldavidfowldavidfowl left review comments
@eerhardteerhardteerhardt left review comments

+2 more reviewers

@IDisposableIDisposableIDisposable left review comments
@buyaa-nbuyaa-nbuyaa-n approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@stevehartersteveharter

Labels

area-Extensions-DependencyInjectiontenet-performancePerformance related issue

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

Consider updating ActivatorUtilities.CreateFactory to use ILEmit when possible


6 participants

@steveharter@jkotas@davidfowl@IDisposable@eerhardt@buyaa-n

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();
For perf, use the new ConstructorInvoker APIs for ActivatorUtilities.CreateFactory by steveharter · Pull Request #89573 · dotnet/runtime · GitHub
Skip to content
For perf, use the new ConstructorInvoker APIs for ActivatorUtilities.CreateFactory - #89573
Merged
steveharter merged 6 commits into
dotnet:mainfrom 
steveharter:DiPerf
Aug 3, 2023
Merged
For perf, use the new ConstructorInvoker APIs for ActivatorUtilities.CreateFactory#89573
steveharter merged 6 commits into
dotnet:mainfrom 
steveharter:DiPerf
Conversation
@steveharter
@stevehartersteveharter
commented
Jul 27, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Fixes#66153 which also contains additional information on benchmarks here.
This PR changes DI to use the new zero-alloc invoke APIs and is expected to close out the DI+Blazor perf work for v8 -- Blazor apps can use DI ActivatorUtilities.CreateFactory without bringing in the large System.Linq.Expressions assembly and in a performant manner (but not quite as fast as having Blazor use Linq Expressions instead of reflection).
In summary, for Blazor, this PR makes CreateFactory ~1.3-1.5x faster for common "fast paths" measured under a Blazor client app. However, when run under CorClr+Windows, there is a much larger ~2-3x gain. The difference appears to be additional overhead in Mono interpreter lambda methods with variable capture. For NativeAOT, this PR is expected to make CreateFactory also around ~1.5x faster based on reflection methods being 1.3x-1.7x faster with the new zero-alloc APIs.
Background:
Note the reflection path is used (vs. Expressions) when RuntimeFeature.IsDynamicCodeCompiled == false which will be the case for Blazor client and NativeAOT . For NativeAOT, using expressions would cause them to be interpreted for NativeAOT which is very slow, and for Blazor using expressions would bring along the very large System.Linq.Expressions assembly where a smaller (trimmed) size is important. Note that this PR does not change the semantics here (that was done previously -- see the links above); only the CPU performance is improved.
@stevehartersteveharter
self-assigned this
Jul 27, 2023
@ghost
ghost
commented
Jul 27, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection

See info in area-owners.md if you want to be subscribed.
Issue Details
[verifying tests]
Author:steveharter
Assignees:steveharter
Labels:
tenet-performance, area-Extensions-DependencyInjection
Milestone:-
@stevehartersteveharter
mentioned this pull request
Jul 27, 2023

Closed

6 tasks
@stevehartersteveharter
added this to the 8.0.0 milestone
Jul 27, 2023
@steveharter
steveharter
requested a review
from buyaa-nJuly 27, 2023 16:58
@steveharter
steveharter
marked this pull request as ready for review
July 27, 2023 17:09
steveharter
steveharter
commented
Jul 27, 2023
View reviewed changes
Comment thread...libraries/Microsoft.Extensions.DependencyInjection/tests/DI.Tests/ActivatorUtilitiesTests.cs

 [Fact]
 public void CreateFactory_CreatesFactoryMethod()
 public void CreateFactory_CreatesFactoryMethod_4Types_3Injected()
@stevehartersteveharterJul 27, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This new test was added for coverage; the other new code paths already had coverage.
eerhardt
eerhardt
reviewed
Jul 27, 2023
View reviewed changes
@eerhardteerhardt
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Do we have any perf numbers (both throughput and size) for this change?
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
Outdated

Comment thread...s/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.Reflection.cs
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
Jul 27, 2023

Closed

davidfowl
davidfowl
reviewed
Aug 1, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs
 if (serviceProvider is null)
 ThrowHelperArgumentNullExceptionServiceProvider();

 object?[] constructorArguments = new object?[parameters.Length];
@davidfowldavidfowlAug 1, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Not saying we should but we could pool this array now that we have Span support.
@steveharter
steveharter
commented
Aug 1, 2023

Copy link


Copy Markdown

ContributorAuthor
Do we have any perf numbers (both throughput and size) for this change?
See #85065 for the previous size gains of removing Linq.Expressions assembly (went from ~260k+ to 4k) when ActivatorUtilities.CreateFactory is used.
See #66153 which has additional details on CPU gains. Reflection APIs are much faster in general, and the DI CreateFactory is ~1.5x faster depending on the scenario, but still slower than using Linq expressions. We could investigate further as the Blazor gains were not as much as the Windows gains, percentage-wise.
buyaa-n
buyaa-n
reviewed
Aug 2, 2023
View reviewed changes
Comment threadsrc/libraries/Microsoft.Extensions.DependencyInjection.Abstractions/src/ActivatorUtilities.cs

Comment on lines

+731
 to 
+732

 if (serviceProvider is null)
 ThrowHelperArgumentNullExceptionServiceProvider();
@buyaa-nbuyaa-nAug 2, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

NIT: Since it is within .NET 8 if-def could we use ArgumentNullException.ThrowIfNull(serviceProvider)? Here and 4 similar cases below
@stevehartersteveharterAug 3, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Yeah there was an earlier discussion around this - it's at least consistent now across all cases using the helper method. In one case, under .NET 8, we couldn't use ThrowIfNull due to a compile warning around unused variable (but we still want to verify for consistency) and of course under .NetStandard.NetFx we can't use ThrowIfNull at all.
buyaa-n
buyaa-n
approved these changes
Aug 2, 2023
View reviewed changes
@buyaa-nbuyaa-n
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Left a NIT, overall LGTM thanks!
@steveharter
steveharter
merged commit c416966
into
dotnet:mainAug 3, 2023
@steveharter
steveharter
deleted the

DiPerf

branch
August 3, 2023 12:31
@jkotas
jkotas
commented
Aug 4, 2023

Copy link


Copy Markdown

Member
This change badly broke DI for native AOT. Number of DI tests are failing on native AOT in this repo, and it soon going to affect ASP.NET and perflab. I think we need to revert this change.
Example of failure: https://helixre107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-pull-89969-merge-ce136d5e93d548e1a8/Microsoft.Extensions.Http.Tests/1/console.85a149a2.log?helixlogtype=result
[FAIL] Microsoft.Extensions.DependencyInjection.HttpClientFactoryServiceCollectionExtensionsTest.AddHttpClient_MessageHandler_Scope_SingletonDependency
System.Reflection.TargetParameterCountException : Parameter count mismatch.
at System.Reflection.DynamicInvokeInfo.ThrowForArgCountMismatch() + 0x7c
at System.Reflection.DynamicInvokeInfo.InvokeDirectWithFewArgs(Object, IntPtr, Span`1) + 0x1e8
at Internal.Reflection.Execution.MethodInvokers.InstanceMethodInvoker.InvokeDirectWithFewArgs(Object, Span`1) + 0x39
at Internal.Reflection.Execution.MethodInvokers.InstanceMethodInvoker.CreateInstanceWithFewArgs(Span`1) + 0x28
at System.Reflection.ConstructorInvoker.Invoke(Object, Object, Object, Object) + 0x5a
at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.ReflectionFactoryCanonicalFixed(ConstructorInvoker, ActivatorUtilities.FactoryParameterContext[], Type, IServiceProvider, Object[]) + 0x2f9
at Microsoft.Extensions.DependencyInjection.ActivatorUtilities.<>c__DisplayClass14_2.<CreateFactoryReflection>b__7(IServiceProvider serviceProvider, Object[] arguments) + 0x27
at Microsoft.Extensions.Http.DefaultTypedHttpClientFactory`1.CreateClient(HttpClient) + 0x59
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitDisposeCache(ServiceCallSite, RuntimeResolverContext) + 0xe
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteVisitor`2.VisitCallSite(ServiceCallSite callSite, TArgument argument) + 0xb5
at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.Resolve(ServiceCallSite, ServiceProviderEngineScope) + 0x3d
at Microsoft.Extensions.DependencyInjection.ServiceProvider.GetService(ServiceIdentifier, ServiceProviderEngineScope) + 0xa3
at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider, Type) + 0x99
at Microsoft.Extensions.DependencyInjection.HttpClientFactoryServiceCollectionExtensionsTest.<AddHttpClient_MessageHandler_Scope_SingletonDependency>d__39.MoveNext() + 0x2b4

jkotas added a commit
that referenced
this pull request
Aug 4, 2023
@jkotas
Revert "For perf, use the new ConstructorInvoker APIs for ActivatorUt…
87f09e1
…ilities.CreateFactory (#89573)"
This reverts commit c416966.
@jkotasjkotas
mentioned this pull request
Aug 4, 2023
 Merged

AaronRobinsonMSFT pushed a commit
that referenced
this pull request
Aug 4, 2023
@jkotas
Revert "For perf, use the new ConstructorInvoker APIs for ActivatorUt…
ba196ce
…ilities.CreateFactory (#89573)" (#89992)
This reverts commit c416966.
@stevehartersteveharter
mentioned this pull request
Aug 7, 2023
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Sep 3, 2023
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@davidfowldavidfowldavidfowl left review comments
@eerhardteerhardteerhardt left review comments

+2 more reviewers

@IDisposableIDisposableIDisposable left review comments
@buyaa-nbuyaa-nbuyaa-n approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@stevehartersteveharter

Labels

area-Extensions-DependencyInjectiontenet-performancePerformance related issue

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

Consider updating ActivatorUtilities.CreateFactory to use ILEmit when possible


6 participants

@steveharter@jkotas@davidfowl@IDisposable@eerhardt@buyaa-n