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.

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

#827 #1679 Improve performance of Request Mapper #1724

Conversation

ggnaegi
Copy link
Member

@ggnaegi ggnaegi commented Oct 9, 2023

Fixes #1679 #827

Proposed Changes

/// Conclusion: by overriding HttpContent.SerializeToStreamAsync,
/// we have full control over pumping bytes to the target stream for all protocols
/// (except Web Sockets, which is handled separately).

@raman-m @RaynaldM I have written a new benchmark, called payload benchmarks (part of PR too). We send various payloads (empty, 25, 50, 100, 1000, 5000, 10000, 20000 json objects), which involves the use of RequestMapper, and evaluate performance and memory usage. I checked that the target client was receiving the payload.

@raman-m @weichaohh As a colleague pointed out, I didn't address the issue of smaller payloads with a length < than the Default Buffer Size. Therefore I have added smaller payloads to the benchmark's payloads. Some are smaller than the Large Object Heap threshold, 85KB. So I modified the copy logic, and made sure that the default buffer size was only used when the promised / announced content length was >= than the Default Buffer Size.

First I have checked the performance of the develop branch code:
image
Figure 1: develop branch code

And then I checked the results of this PR
image
Figure 2:PR code

As you can see, we have a performance gain, but also lower memory usage.
Worst case: 92 MB vs 977 KB for the 20'000 objects payload

@ggnaegi ggnaegi closed this Oct 9, 2023
@ggnaegi ggnaegi reopened this Oct 13, 2023
@ggnaegi
Copy link
Member Author

ggnaegi commented Oct 15, 2023

@raman-m this is largely inspired by yarp.

@raman-m
Copy link
Member

raman-m commented Oct 15, 2023

⚠️ Current build 2178 has failed! This is well-known issue #1706

@raman-m raman-m changed the title Features/1679 request mapper performance issues #1679 Improve performance of Request Mapper Oct 15, 2023
@raman-m raman-m added the proposal Proposal for a new functionality in Ocelot label Oct 15, 2023
src/Ocelot/Request/Mapper/RequestMapper.cs Outdated Show resolved Hide resolved
src/Ocelot/Request/Mapper/RequestMapper.cs Outdated Show resolved Hide resolved
@ggnaegi
Copy link
Member Author

ggnaegi commented Oct 17, 2023

@RaynaldM @raman-m I will provide some benchmarks...

@raman-m
Copy link
Member

raman-m commented Oct 17, 2023

@RaynaldM @raman-m I will provide some benchmarks...

That will be awesome to attach benchmark results of old version code and new one. Thanks!
Screenshots are better to attach.

@raman-m
Copy link
Member

raman-m commented Oct 17, 2023

I'll review this PR after documentation release... OK?

@ggnaegi
Copy link
Member Author

ggnaegi commented Oct 17, 2023

I'll review this PR after documentation release... OK?

Yeah fine, I will check the performance with 1-2 benchmarks first.

@ggnaegi
Copy link
Member Author

ggnaegi commented Oct 23, 2023

@raman-m Ok, code is now review ready.

@raman-m
Copy link
Member

raman-m commented Oct 30, 2023

Will this PR fix #749 ?

@ggnaegi ggnaegi force-pushed the features/1679-request-mapper-performance-issues branch from 244883e to b669552 Compare December 12, 2023 17:17
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready for delivery! ✔️

Thanks Gui for your efforts! 😍
This will be the brilliant feature of November'23 release! 💎

@raman-m raman-m merged commit a17242d into ThreeMammals:develop Dec 13, 2023
2 checks passed
@RaynaldM
Copy link
Collaborator

We've had this PR in production for 5 days (plus a trial run a few days earlier), it's working well, and we haven't had any Out of Memory issues, or even a slightly lower memory footprint from the app.

@raman-m
Copy link
Member

raman-m commented Dec 14, 2023

@RaynaldM commented on Dec 14

🆗 Good to know! 👍
Please, watch the system in production further... OOM and Docker container vs K8s pods restarts also?!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Nov'23 November 2023 release proposal Proposal for a new functionality in Ocelot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RequestMapper performance issue: ToByteArray(request.Body) Low performance even with logging disabled
3 participants