-
Notifications
You must be signed in to change notification settings - Fork 133
Avoid GetRequiredService in ForwardTo? #124
Comments
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! |
@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. |
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. |
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. Spec: |
v2.2.1 is on nuget.org with this specific issue addressed. I'll do further profiling in a subsequent release. |
@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. |
Hmm, thought it rebuilt the handlers internally.. |
I've unlisted the 2.1.1 package pending investigation. |
Yes, it seems that HttpClient (via I'll profile things to see what can be done wrt allocations. |
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. |
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 |
…ifferent is significant. Fixes #124.
…ifferent is significant. Fixes #124.
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. |
@abergs 2.1.2 on nuget.org. Thanks again for the report. |
Further validation that the problem appears to be in |
Thanks for the report, I'm following up on this. |
Fixed here: dotnet/extensions#2063 |
Treat this issue as an invitation to a discussion rather than a bug.
I'm analyzing the memory usage with dotMemory when running 125 concurrent connections "bombarding" the proxy over 7 min. Total request count around 138k.
The memory pattern is quite bumpy:
I'm seeing this:
Which expands to this:
I'm thinking the call to GetRequiredService is triggering reflection?
The text was updated successfully, but these errors were encountered: