Avoid GetRequiredService in ForwardTo? about proxykit

[abergs]

I used https://github.com/codesenberg/bombardier to generate the load and fetched a HTML url that weighs around 10kb with 125 concurrent connections for 7m.

Before i run proxykit i just read a string value from the headers with a date, like "deployment-190624-1111".

It ran on my laptop (with other apps running) so the benchmark should not be treated like a sure thing and would probably vary a lot depending on profiling.
However, the reflection caused a lot of queued Finalization so the optimization is probably a good thing. The "GetRequiredService" should probably not be done by the client but somewhere "up the chain" in ProxyKit (not in the hotpath) or cached in a sane manner.

Spec:
Win 10 Enterprise latop
CPU: i7 7820 @ 2,90 GHz 4 cores, 8 logical.
16GB ram.

[rynowak]

Thanks for the report, I'm following up on this.

[abergs]

I cloned the repo and made a (perhaps) ugly test by adding ProxyKitClient as a optional argument in the Forwardto-method.

I instead initialized it once in startup and re-use that instance.

This removed the Finalized objects observed earlier.
Still seeing bumpy memory allocations though.

Edit: It also improved througput with ~12% (4MB/s to 4,6MB/s)

[damianh]

Awesome report!

Can you share your test project on github? I feel the total request count is a bit on the low side 🤔 . I'd like to locally reproduce what you are seeing.

Thanks!

[abergs]

@damianh Do you mean the fork of ProxyKit or "my application" using it? The application is unfortunately not something I can open source... but it's a small .net core app that maps a route, does some header parsing and then forwardsTo.

[damianh]

Post the fork changes to a branch on your repo for a start. I don't need your application but maybe some indication of what it does so I can put the total requests into some context. If you can, please share your proxy config (header parsing etc), what tool you are using to generate the load and maybe some indication of the system it is being run on.

Would like to get to as like-for-like as possible.

[damianh]

v2.2.1 is on nuget.org with this specific issue addressed. I'll do further profiling in a subsequent release.

[saithis]

@damianh Doesn't this now only instantiate HttpClient once and always use this single intance? If I remember correctly, then this has the sideeffect, that DNS ttl is ignored.

Last time I checked, the HttpClientFactory switched the HttpClientHandler (which resolves each DNS only once) for new HttpClients every few minutes so that the DNS ttl is respected, but that doesn't work if the HttpClient is kept as a singleton.

[damianh]

Hmm, thought it rebuilt the handlers internally..
Will check asap and revert if so.

[damianh]

I've unlisted the 2.1.1 package pending investigation.

[damianh]

Yes, it seems that HttpClient (via ProxyKitClient) needs to be considered transient and activated and retrieved on per-request basis. I've reverted #125 and related changes.

I'll profile things to see what can be done wrt allocations.

[saithis]

Maybe you can inject the IHttpClientFactory (which is a singleton) and call CreateClient() when needed to get the HttpClient. If that doesn't solve the allocation problem, then you could probably cache the created HttpClient for a short duration. A setting for the duration would then be useful with duration 0/null equals no caching.

[damianh]

That is what I am currently thinking except it means I can't use a typed client. (May or may not be an issue).

Am gonna profile before first so I have some local data to work with

[damianh]

Did a mini profile and the difference between resolving a typed client and named client is surprisingly large:

Thus I don't think it's GetRequiredService<> per se, but activation of the typed client.

I'm going to move to using named client.

[damianh]

It's not a cache warm up issue either

[damianh]

Regarding the rest of the bumpy memory usage, I suspect it's heavily related to allocation regarding HttpClient usage. I don't know how much I can optimize there from the outside (i.e. object pooling) but will dig in separately.

[damianh]

@abergs 2.1.2 on nuget.org. Thanks again for the report.

[damianh]

Further validation that the problem appears to be in HttpClientFactory

• https://twitter.com/thoemmi/status/1149655040686055425
• https://twitter.com/davidfowl/status/1149648815147741184

[abergs]

Fixed here: dotnet/extensions#2063