Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use dynamically compiled factory instead of Activator.CreateInstance in TypedClientBuilder #14615

Merged
merged 7 commits into from
Oct 8, 2019
Merged

Use dynamically compiled factory instead of Activator.CreateInstance in TypedClientBuilder #14615

merged 7 commits into from
Oct 8, 2019

Conversation

khellang
Copy link
Member

@khellang khellang commented Oct 1, 2019 •
edited
Loading

This should shave off some milliseconds every time a typed client is constructed 😄

There's already tests covering these paths, but I added an extra benchmark, shown below.

Before

BenchmarkDotNet=v0.10.13, OS=Windows 10.0.17134
Intel Core i7-6820HQ CPU 2.70GHz (Skylake), 1 CPU, 8 logical cores and 4 physical cores
.NET Core SDK=5.0.100-alpha1-013788
  [Host]     : .NET Core 5.0.0-alpha1.19404.5 (CoreCLR 5.0.19.40302, CoreFX 5.0.19.38102), 64bit RyuJIT
  Job-FBDBIV : .NET Core 5.0.0-alpha1.19404.5 (CoreCLR 5.0.19.40302, CoreFX 5.0.19.38102), 64bit RyuJIT

Runtime=Core  Server=True  Toolchain=.NET 5.0  RunStrategy=Throughput
Method Mean Error StdDev Op/s Gen 0 Allocated
Build 692.2 ns 15.00 ns 17.27 ns 1,444,691.6 0.0095 408 B

After

BenchmarkDotNet=v0.10.13, OS=Windows 10.0.17134
Intel Core i7-6820HQ CPU 2.70GHz (Skylake), 1 CPU, 8 logical cores and 4 physical cores
.NET Core SDK=5.0.100-alpha1-013788
  [Host]     : .NET Core 5.0.0-alpha1.19404.5 (CoreCLR 5.0.19.40302, CoreFX 5.0.19.38102), 64bit RyuJIT
  Job-TKTSZD : .NET Core 5.0.0-alpha1.19404.5 (CoreCLR 5.0.19.40302, CoreFX 5.0.19.38102), 64bit RyuJIT

Runtime=Core  Server=True  Toolchain=.NET 5.0  RunStrategy=Throughput
Method Mean Error StdDev Op/s Gen 0 Allocated
Build 14.88 ns 0.3769 ns 0.4032 ns 67,220,764.5 0.0006 24 B

@BrennanConroy
Copy link
Member

Maybe I'm missing it, but you seem to be missing some recent changes to this file, could you rebase on master?

@analogrelay analogrelay added the feature-httpclientfactory Includes: HttpClientFactory (some bugs also in Extensions repo) label Oct 1, 2019
@analogrelay analogrelay added this to the 5.0.0-preview1 milestone Oct 1, 2019
@analogrelay analogrelay added area-signalr Includes: SignalR clients and servers and removed feature-httpclientfactory Includes: HttpClientFactory (some bugs also in Extensions repo) labels Oct 1, 2019
@khellang
Copy link
Member Author

khellang commented Oct 1, 2019

Maybe I'm missing it, but you seem to be missing some recent changes to this file, could you rebase on master?

Hmm, looks like I based it of my origin master which is pretty far behind, but GitHub says it's automatically mergeable though...

@khellang khellang changed the title Use compiled expression in TypedClientBuilder Use dynamically compiled factory instead of Activator.CreateInstance in TypedClientBuilder Oct 2, 2019
@khellang
Copy link
Member Author

khellang commented Oct 2, 2019

Alright, got rid of System.Linq.Expressions and rebased 👌

@khellang
Copy link
Member Author

khellang commented Oct 2, 2019

Hmm, I think it might be even cleaner to generate a static Build method on the actual Impl type and create a delegate directly from that.

@halter73
Copy link
Member

halter73 commented Oct 3, 2019

/azp run AspNetCore-ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@khellang
Copy link
Member Author

khellang commented Oct 8, 2019

/azp run AspNetCore-ci

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 14615 in repo aspnet/AspNetCore

@davidfowl
Copy link
Member

/azp run AspNetCore-ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@BrennanConroy BrennanConroy merged commit f31ce2d into dotnet:master Oct 8, 2019
@BrennanConroy
Copy link
Member

Thanks @khellang !

@khellang khellang deleted the typed-client-factory branch October 8, 2019 15:33
@khellang
Copy link
Member Author

khellang commented Oct 8, 2019

Thank you ❤️

@GrabYourPitchforks GrabYourPitchforks mentioned this pull request Feb 19, 2020
Merged
2 tasks
@GrabYourPitchforks GrabYourPitchforks mentioned this pull request Mar 24, 2020
Closed
5 tasks
@GrabYourPitchforks GrabYourPitchforks mentioned this pull request May 11, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@halter73 halter73 halter73 approved these changes

@davidfowl davidfowl davidfowl approved these changes

@BrennanConroy BrennanConroy BrennanConroy approved these changes

@analogrelay analogrelay Awaiting requested review from analogrelay

@mikaelm12 mikaelm12 Awaiting requested review from mikaelm12

Assignees
No one assigned
Labels
area-signalr Includes: SignalR clients and servers
Projects
None yet
Milestone
5.0.0-preview1
Development

Successfully merging this pull request may close these issues.
5 participants
@khellang @BrennanConroy @halter73 @davidfowl @analogrelay