Avoid dictionary lookup for singleton services #52035

davidfowl · 2021-04-29T04:58:52Z

Stash the instance on the callsite itself and avoid the resolved services lookup.
This mainly benefits the reflection only path as the ILEmit codegen path will replace this code with an optimized delegate. This should help resolving singletons in the blazor code path and for nativeAOT.
The other benefit is that the concurrent dictionary that was used before to store singletons will be empty in the common case.
This change also introduces callsite caching (memoization). Today callsites are cached but only the root type (A -> B -> C), GetService<A> will only cache the A callsite. This change will result in caching A, B and C.

~~PS: Still testing this.~~

- Stash the instance on the callsite itself and avoid the resolved services lookup.

ghost · 2021-04-29T04:58:58Z

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

Stash the instance on the callsite itself and avoid the resolved services lookup.

Author:	davidfowl
Assignees:	-
Labels:	`area-Extensions-DependencyInjection`
Milestone:	-

- Don't lock if the callsite value is already resolved - Make the value on ServiceCallsite volatile

- cache callsites - update tests

davidfowl · 2021-04-29T15:03:16Z

/azp list

azure-pipelines · 2021-04-29T15:03:22Z

CI/CD Pipelines for this repository: coreclr-gc-longrunning coreclr-gc-simulator runtime-coreclr outerloop runtime-coreclr jitstress runtime-coreclr jitstressregs runtime-coreclr jitstress2-jitstressregs runtime-coreclr gcstress0x3-gcstress0xc runtime-coreclr gcstress-extra runtime-coreclr r2r-extra runtime-coreclr jitstress-isas-x86 runtime-coreclr jitstress-isas-arm runtime-coreclr jitstressregs-x86 runtime-coreclr libraries-jitstressregs runtime-coreclr libraries-jitstress2-jitstressregs runtime-coreclr r2r runtime-coreclr runincontext runtime-coreclr crossgen2 runtime-libraries-coreclr outerloop runtime-libraries-coreclr outerloop-windows runtime-libraries-coreclr outerloop-linux runtime-libraries-coreclr outerloop-osx runtime runtime-libraries enterprise-linux runtime-libraries stress-http runtime-libraries stress-ssl runtime-dev-innerloop runtime-coreclr crossgen2 outerloop coreclr-release-outerloop-nightly sync-runtime-to-mono runtime-coreclr crossgen2-composite runtime-jit-experimental runtime-coreclr libraries-jitstress runtime-coreclr libraries-gcstress0x3-gcstress0xc runtime-coreclr libraries-gcstress-extra dotnet-linker-tests runtime-coreclr ilasm runtime-coreclr clrinterpreter runtime-coreclr crossgen2-composite gcstress runtime-libraries-mono outerloop runtime-staging runtime-richnav runtime-coreclr gc-largepages

davidfowl · 2021-04-29T15:03:39Z

/azp run runtime

davidfowl · 2021-04-29T15:03:56Z

/azp run runtime-staging

azure-pipelines · 2021-04-29T15:04:06Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2021-04-29T15:04:11Z

Azure Pipelines successfully started running 1 pipeline(s).

src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceCallSite.cs

src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteFactory.cs

pakrym · 2021-04-29T15:34:40Z

LGTM, good change.

src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceCacheKey.cs

...raries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs

eerhardt

...raries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs

Tornhoof · 2021-04-30T09:18:25Z

src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceCacheKey.cs

@@ -5,7 +5,7 @@

 namespace Microsoft.Extensions.DependencyInjection.ServiceLookup
 {
-    internal struct ServiceCacheKey: IEquatable<ServiceCacheKey>
+    internal readonly struct ServiceCacheKey : IEquatable<ServiceCacheKey>


Not directly related to your changes, but the HashCode calculation will throw for the static Empty, the Type is null then, and GetHashCode will throw a null ref exception.
I suggest changing it to HashCode.Combine(Type, Slot) which takes care of the null

Will do that in a different PR.

I created #52115 to fix that nullref exception.

danmoseley · 2021-04-30T17:04:59Z

@dotnet/dnceng can you please investigate what happened to Android legs?

 Job 09719656-7f08-43c1-913f-c65ec565ba97 on Ubuntu.1804.Amd64.Android.Open is completed with 217 finished work items.
/__w/1/s/.packages/microsoft.dotnet.helix.sdk/6.0.0-beta.21222.1/tools/azure-pipelines/AzurePipelines.MultiQueue.targets(30,5): error : System.Net.Http.HttpRequestException: Response status code does not indicate success: 500 (Internal Server Error). [/__w/1/s/src/libraries/sendtohelixhelp.proj]
/__w/1/s/.packages/microsoft.dotnet.helix.sdk/6.0.0-beta.21222.1/tools/azure-pipelines/AzurePipelines.MultiQueue.targets(30,5): error :    at System.Net.Http.HttpResponseMessage.EnsureSuccessStatusCode() [/__w/1/s/src/libraries/sendtohelixhelp.proj]
/__w/1/s/.packages/microsoft.dotnet.helix.sdk/6.0.0-beta.21222.1/tools/azure-pipelines/AzurePipelines.MultiQueue.targets(30,5): error :    at Microsoft.DotNet.Helix.Sdk.CreateTestsForWorkItems.<>c__DisplayClass5_0.<<AttachResultFileToTestResultAsync>b__0>d.MoveNext() in /_/src/Microsoft.DotNet.Helix/Sdk/CreateFailedTestsForFailedWorkItems.cs:line 73 [/__w/1/s/src/libraries/sendtohelixhelp.proj]
/__w/1/s/.packages/microsoft.dotnet.helix.sdk/6.0.0-beta.21222.1/tools/azure-pipelines/AzurePipelines.MultiQueue.targets(30,5): error : --- End of stack trace from previous location --- [/__w/1/s/src/libraries/sendtohelixhelp.proj]
/__w/1/s/.packages/microsoft.dotnet.helix.sdk/6.0.0-beta.21222.1/tools/azure-pipelines/AzurePipelines.MultiQueue.targets(30,5): error :    at Microsoft.DotNet.Helix.AzureDevOps.AzureDevOpsTask.<>c__DisplayClass13_0.<<RetryAsync>b__0>d.MoveNext() in /_/src/Microsoft.DotNet.Helix/Sdk/AzureDevOpsTask.cs:line 91 [/__w/1/s/src/libraries/sendtohelixhelp.proj]
/__w/1/s/.packages/microsoft.dotnet.helix.sdk/6.0.0-beta.21222.1/tools/azure-pipelines/AzurePipelines.MultiQueue.targets(30,5): error : --- End of stack trace from previous location --- [/__w/1/s/src/libraries/sendtohelixhelp.proj]
/__w/1/s/.packages/microsoft.dotnet.helix.sdk/6.0.0-beta.21222.1/tools/azure-pipelines/AzurePipelines.MultiQueue.targets(30,5): error :    at Microsoft.DotNet.Helix.AzureDevOps.AzureDevOpsTask.RetryAsync[T](Func`1 function, Action`1 logRetry, Func`2 isRetryable, CancellationToken cancellationToken) in /_/src/Microsoft.DotNet.Helix/Sdk/AzureDevOpsTask.cs:line 206 [/__w/1/s/src/libraries/sendtohelixhelp.proj]
/__w/1/s/.packages/microsoft.dotnet.helix.sdk/6.0.0-beta.21222.1/tools/azure-pipelines/AzurePipelines.MultiQueue.targets(30,5): error :    at Microsoft.DotNet.Helix.AzureDevOps.AzureDevOpsTask.RetryAsync(Func`1 function) in /_/src/Microsoft.DotNet.Helix/Sdk/AzureDevOpsTask.cs:line 86 [/__w/1/s/src/libraries/sendtohelixhelp.proj]
##[error].packages/microsoft.dotnet.helix.sdk/6.0.0-beta.21222.1/tools/azure-pipelines/AzurePipelines.MultiQueue.targets(30,5): error : (NETCORE_ENGINEERING_TELEMETRY=Helix) System.Net.Http.HttpRequestException: Response status code does not indicate success: 500 (Internal Server Error).
   at System.Net.Http.HttpResponseMessage.EnsureSuccessStatusCode()
   at Microsoft.DotNet.Helix.Sdk.CreateTestsForWorkItems.<>c__DisplayClass5_0.<<AttachResultFileToTestResultAsync>b__0>d.MoveNext() in /_/src/Microsoft.DotNet.Helix/Sdk/CreateFailedTestsForFailedWorkItems.cs:line 73

I will rerun them

missymessa · 2021-04-30T17:08:02Z

@danmoseley based on the error message, this looks related to the issue we noticed yesterday: https://github.com/dotnet/core-eng/issues/12969

danmoseley · 2021-04-30T17:09:21Z

Thanks @missymessa . I also notice that there were test failures (https://dev.azure.com/dnceng/9ee6d478-d288-47f7-aacc-f6e6d082ae6d/_apis/build/builds/1115588/logs/91) but none of them were reported here. Is that related too?

missymessa · 2021-04-30T17:12:32Z

Looks like the test failures are showing up in AzDO. Are you meaning that they're not showing up on the Build Result Analysis? I think that's due to not having proper multi-pipeline support right now.

danmoseley · 2021-04-30T17:14:10Z

What are these failures

    System.Runtime.InteropServices.Tests.MarshalComDisabledTests.CreateAggregatedObject_ThrowsNotSupportedException [FAIL]
      Assert.Throws() Failure
      Expected: typeof(System.NotSupportedException)
      Actual:   typeof(System.PlatformNotSupportedException): COM Interop is not supported on this platform.
      ---- System.PlatformNotSupportedException : COM Interop is not supported on this platform.
      Stack Trace:
        /_/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.NoCom.cs(23,0): at System.Runtime.InteropServices.Marshal.CreateAggregatedObject(IntPtr pOuter, Object o)
        /_/src/libraries/System.Runtime.InteropServices/tests/ComDisabled/System/Runtime/InteropServices/Marshal/MarshalComDisabledTests.cs(19,0): at System.Runtime.InteropServices.Tests.MarshalComDisabledTests.<>c__DisplayClass1_0.<CreateAggregatedObject_ThrowsNotSupportedException>b__0()
        ----- Inner Stack Trace -----
        /_/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.NoCom.cs(23,0): at System.Runtime.InteropServices.Marshal.CreateAggregatedObject(IntPtr pOuter, Object o)
        /_/src/libraries/System.Runtime.InteropServices/tests/ComDisabled/System/Runtime/InteropServices/Marshal/MarshalComDisabledTests.cs(19,0): at System.Runtime.InteropServices.Tests.MarshalComDisabledTests.<>c__DisplayClass1_0.<CreateAggregatedObject_ThrowsNotSupportedException>b__0()

https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-52035-merge-92c55ea76bff450dba/System.Runtime.InteropServices.ComDisabled.Tests/console.74d5849f.log?sv=2019-07-07&se=2021-05-20T07%3A25%3A35Z&sr=c&sp=rl&sig=EYVspnfSzruqdpqPZhSFmbpm9zO4vGi%2BbBsDfSpexnQ%3D

@elinor-fung ? I can't see a relevant change in a quick look.

danmoseley · 2021-04-30T17:16:32Z

It seems to be from #50662 @LakshanF

danmoseley · 2021-04-30T17:20:09Z

@missymessa it's not clear now because I clicked rerun failed tests 😞 but I thought I wasn't seeing failed checkmarks on this PR page. Perhaps I've lost the repro state now.

elinor-fung · 2021-04-30T19:14:17Z

Thanks, @danmoseley. We shouldn't be running those on Mono - #52128

@LakshanF looks like we missed seeing the introduced test failures on mono+Windows in runtime-staging (test failures show up as passed checks on the PR) - something to make sure to check as the COM trimming work continues.

Avoid dictionary lookup for singleton services

945c4e0

- Stash the instance on the callsite itself and avoid the resolved services lookup.

davidfowl requested a review from maryamariyan April 29, 2021 04:58

dotnet-issue-labeler bot added the area-Extensions-DependencyInjection label Apr 29, 2021

davidfowl requested a review from eerhardt April 29, 2021 04:58

davidfowl added 3 commits April 28, 2021 22:49

Small tweaks

27fb9e8

- Don't lock if the callsite value is already resolved - Make the value on ServiceCallsite volatile

Changed resolved services back to dictionary

ae637b6

Fixed tess

f4d8d38

- cache callsites - update tests

davidfowl requested a review from pakrym April 29, 2021 14:55

pakrym reviewed Apr 29, 2021

View reviewed changes

src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceCallSite.cs Outdated Show resolved Hide resolved

pakrym reviewed Apr 29, 2021

View reviewed changes

src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteFactory.cs Outdated Show resolved Hide resolved

eerhardt reviewed Apr 29, 2021

View reviewed changes

src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceCacheKey.cs Outdated Show resolved Hide resolved

eerhardt reviewed Apr 29, 2021

View reviewed changes

...raries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs Outdated Show resolved Hide resolved

eerhardt approved these changes Apr 29, 2021

View reviewed changes

maryamariyan approved these changes Apr 29, 2021

View reviewed changes

...raries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/CallSiteRuntimeResolver.cs Show resolved Hide resolved

davidfowl added 2 commits April 29, 2021 23:37

PR feedback and cleanup

b891ae1

Remove extra whitespace

345c0b7

Tornhoof reviewed Apr 30, 2021

View reviewed changes

Tornhoof mentioned this pull request Apr 30, 2021

Handle null Type in ServiceCacheKey.GetHashCode #52115

Merged

davidfowl merged commit 17b881c into main Apr 30, 2021

jkotas deleted the davidfowl/skip-second-lookup branch May 3, 2021 14:59

DrewScoggins mentioned this pull request May 5, 2021

[Perf] Changes at 4/30/2021 7:46:45 PM DrewScoggins/performance-2#5584

Open

maryamariyan mentioned this pull request May 7, 2021

HTTP2: Two remote server tests occasionally timing out in CI #1580

Closed

karelz added this to the 6.0.0 milestone May 20, 2021

ghost locked as resolved and limited conversation to collaborators Jun 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid dictionary lookup for singleton services #52035

Avoid dictionary lookup for singleton services #52035

davidfowl commented Apr 29, 2021 •

edited

Loading

ghost commented Apr 29, 2021

davidfowl commented Apr 29, 2021

azure-pipelines bot commented Apr 29, 2021

davidfowl commented Apr 29, 2021

davidfowl commented Apr 29, 2021

azure-pipelines bot commented Apr 29, 2021

azure-pipelines bot commented Apr 29, 2021

pakrym commented Apr 29, 2021

eerhardt left a comment

Tornhoof Apr 30, 2021

davidfowl Apr 30, 2021

Tornhoof Apr 30, 2021

danmoseley commented Apr 30, 2021

missymessa commented Apr 30, 2021

danmoseley commented Apr 30, 2021

missymessa commented Apr 30, 2021

danmoseley commented Apr 30, 2021

danmoseley commented Apr 30, 2021

danmoseley commented Apr 30, 2021

elinor-fung commented Apr 30, 2021

Avoid dictionary lookup for singleton services #52035

Avoid dictionary lookup for singleton services #52035

Conversation

davidfowl commented Apr 29, 2021 • edited Loading

ghost commented Apr 29, 2021

davidfowl commented Apr 29, 2021

azure-pipelines bot commented Apr 29, 2021

davidfowl commented Apr 29, 2021

davidfowl commented Apr 29, 2021

azure-pipelines bot commented Apr 29, 2021

azure-pipelines bot commented Apr 29, 2021

pakrym commented Apr 29, 2021

eerhardt left a comment

Choose a reason for hiding this comment

Tornhoof Apr 30, 2021

Choose a reason for hiding this comment

davidfowl Apr 30, 2021

Choose a reason for hiding this comment

Tornhoof Apr 30, 2021

Choose a reason for hiding this comment

danmoseley commented Apr 30, 2021

missymessa commented Apr 30, 2021

danmoseley commented Apr 30, 2021

missymessa commented Apr 30, 2021

danmoseley commented Apr 30, 2021

danmoseley commented Apr 30, 2021

danmoseley commented Apr 30, 2021

elinor-fung commented Apr 30, 2021

davidfowl commented Apr 29, 2021 •

edited

Loading