search

ThreeMammals / Ocelot

.NET API Gateway
https://www.nuget.org/packages/Ocelot
MIT License
8.31k stars 1.63k forks source link

#356 #695 #1924 Custom `HttpMessageInvoker` pooling #1824

Closed ggnaegi closed 7 months ago

ggnaegi commented 9 months ago

Closes #356 #695 #1924

After some checks, it seems impossible to use HttpClientFactory in the solution. There are several reasons for this:

I conclude that we must implement our own pool mechanism... Thus, I propose a first draft of a solution using solutions borrowed from StackExchange and Microsoft. It will obviously have to be tested in detail, but it serves as a basis for discussion.

Proposed Changes

I will provide Acceptance and Unit tests later. You should @raman-m, @RaynaldM have a look at this draft. Thanks

payloadName Mean Max Op/s Allocated
1KBPayload.json 591.3 us 648.3 us 1,691.1792 39.78 KB
16KBPayload.json 642.4 us 664.7 us 1,556.7119 82.36 KB
32KBPayload.json 650.1 us 675.2 us 1,538.2294 192.68 KB
64KBPayload.json 827.9 us 841.7 us 1,207.8715 350.25 KB
128KBPayload.json 997.5 us 1,018.1 us 1,002.5522 668.68 KB
256KBPayload.json 1,424.4 us 1,461.5 us 702.0456 1303.03 KB
512KBPayload.json 2,323.8 us 2,466.2 us 430.3273 2568.97 KB
2048KBPayload.json 8,299.8 us 8,570.5 us 120.4855 10171.14 KB
8192KBPayload.json 24,469.6 us 25,469.2 us 40.867 40619.62 KB
10MBPayload.dat 29,709.8 us 32,654.6 us 33.6589 42708.54 KB
15360KBPayload.json 54,608.4 us 70,385.4 us 18.3122 47989.27 KB
30720KBPayload.json 101,482.4 us 114,551.1 us 9.8539 95953.63 KB
100MBPayload.dat 381,869.0 us 419,428.2 us 2.6187 362479 KB
1024MBPayload.dat 3,300,482.0 us 3,444,481.6 us 0.303 5193381.84 KB

Table 1: Performance with current http client

payloadName Mean Max Op/s Allocated
1KBPayload.json 579.6 us 588.2 us 1,725.2972 39.26 KB
16KBPayload.json 619.0 us 666.1 us 1,615.4381 66.49 KB
32KBPayload.json 661.8 us 670.4 us 1,511.0611 160.79 KB
64KBPayload.json 856.2 us 888.3 us 1,167.9673 286.51 KB
128KBPayload.json 886.4 us 901.6 us 1,128.1035 540.97 KB
256KBPayload.json 1,478.0 us 1,503.4 us 676.5674 1047.99 KB
512KBPayload.json 2,539.9 us 2,570.8 us 393.713 2061.79 KB
2048KBPayload.json 8,232.4 us 9,159.4 us 121.4709 8128.46 KB
8192KBPayload.json 25,601.3 us 26,334.2 us 39.0605 32306.69 KB
10MBPayload.dat 26,567.3 us 27,575.2 us 37.6403 32300.54 KB
15360KBPayload.json 29,534.4 us 31,930.2 us 33.8588 32361.59 KB
30720KBPayload.json 100,626.1 us 103,409.9 us 9.9378 64746.37 KB
100MBPayload.dat 330,750.7 us 351,597.2 us 3.0234 258272.69 KB
1024MBPayload.dat 3,441,790.4 us 3,559,791.2 us 0.2905 4124999.17 KB

Table 2: Performance with the custom message invoker pool

raman-m commented 9 months ago

@ggnaegi Wow! So great thing... 😍 You are real coding locomotive, Gui! 😉

FYI... Delivery after #1724 , so these both PRs cannot be in the same release because of Prod testing.

raman-m commented 9 months ago

Guys, Let's return to this awesome PR after Nov'23 release.. 🆗 ❔

ggnaegi commented 9 months ago

@raman-m @RaynaldM I'm writing the test cases at the minute. No worries guys! I realized we can still simplify the logic, since the Downstreamroute is an immutable, no need to use the timeout as key parameter, the object reference is good enough. We might have problems in the future if allowing dynamic configuration though (1: empty the pool, then 2: load the new configuration).

ggnaegi commented 9 months ago

@raman-m @RaynaldM I'm writing the test cases at the minute. No worries guys! I realized we can still simplify the logic, since the Downstreamroute is an immutable, no need to use the timeout as key parameter, the object reference is good enough. We might have problems in the future if allowing dynamic configuration though (1: empty the pool, then 2: load the new configuration).

By the way, why not use the Record type instead?

RaynaldM commented 9 months ago

@raman-m @RaynaldM I'm writing the test cases at the minute. No worries guys! I realized we can still simplify the logic, since the Downstreamroute is an immutable, no need to use the timeout as key parameter, the object reference is good enough. We might have problems in the future if allowing dynamic configuration though (1: empty the pool, then 2: load the new configuration).

By the way, why not use the Record type instead?

It is a good idea

raman-m commented 9 months ago

:+1:

ggnaegi commented 9 months ago

Hello @raman-m and @RaynaldM I'm providing some benchmarks, it seems the memory leak has been addressed

ggnaegi commented 8 months ago

@raman-m Interesting: https://learn.microsoft.com/en-us/dotnet/core/runtime-config/garbage-collector#large-object-heap-threshold

raman-m commented 8 months ago

Please, resolve the conflict! 👇

raman-m commented 8 months ago

@ggnaegi As you may know, I've attached #695 ... Is it right? Do you think that #695 will also be fixed by this PR?

The 695 quote:

I couldn't find any configuration setting to enable request or response streaming.

What about this?

raman-m commented 8 months ago

Ready for code review @RaynaldM FYI

ggnaegi commented 8 months ago

@raman-m I think, it's the request mapper implementation that solved the issue #695 (we were copying the full request body to a memory stream). Now, with the http client pool we avoid using the http client full buffering, but as others wrote, the performance improvement is minimal. There was another flaw, reading body content to generate cache keys, but this issue has been addressed in https://github.com/ThreeMammals/Ocelot/pull/1849

raman-m commented 8 months ago

Ready for delivery! ✔️

ggnaegi commented 8 months ago

@raman-m @RaynaldM I would like to add a few test cases, ok?

raman-m commented 8 months ago

@ggnaegi commented on Jan 9

Need to review once again? I will review before the merge definitely! 😉 Let us know when ready to review plz!

P.S.

Pray 🙏 to GitHub Bot which can remove current approvals when you push more commits! 😃

ggnaegi commented 7 months ago

@raman-m could we push this to the november release? I think it's high priority.

ggnaegi commented 7 months ago

@raman-m so, I have added a memory usage acceptance test in ContentTests. I have tried it with the current develop, and we can see a memory increase of about the payload size. In this PR, it is kept within a range of +/- 10 Mb

@raman-m but we should be careful and foresee some field tests when merged to develop, since the refactoring is quite aggressive....

raman-m commented 7 months ago

@raman-m but we should be careful and foresee some field tests when merged to develop, since the refactoring is quite aggressive....

@ggnaegi Yes, it is. Sorry, I don't understand... What are "some field tests"?

Merging was done by you a couple of hours ago. I'll just press the button, there will be no merge conflicts with develop branch too. So, what's the rest of work?

ggnaegi commented 7 months ago

@raman-m but we should be careful and foresee some field tests when merged to develop, since the refactoring is quite aggressive....

@ggnaegi Yes, it is. Sorry, I don't understand... What are "some field tests"?

Merging from develop was done by you a couple of hours ago. I'll just press the button, there will be no merge conflicts.

@raman-m I mean, wait on feedbacks from @RaynaldM and others before moving to release and create the packages ;-)

raman-m commented 7 months ago

Ha, making packages... No! I see you are so hot to release 🤣 There is the rest of 2 other features in the milestone. Ok, make sense to wait for Ray's feedback...

raman-m commented 7 months ago

Ready to merge! ✔️

@ggnaegi @RaynaldM Let me know if you agree with merging.

IamMartinPeek commented 7 months ago

Hey guys, just asking. When will this be released ? Would need that for my project 👍

ggnaegi commented 7 months ago

Hey guys, just asking. When will this be released ? Would need that for my project 👍

Hello, we have validated the behavior on production environments. It's looking good. we will finalize the release process in the next few days.

raman-m commented 7 months ago

@IamMartinPeek Soon... It will be released, I hope, on Monday, Feb 12...