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

RequestMapper performance issue: ToByteArray(request.Body) #1679

Closed
weichaohh opened this issue Jul 11, 2023 · 7 comments · Fixed by #1724
Closed

RequestMapper performance issue: ToByteArray(request.Body) #1679

weichaohh opened this issue Jul 11, 2023 · 7 comments · Fixed by #1724
Assignees
@ggnaegi
Labels
bug Identified as a potential bug high High priority merged Issue has been merged to dev and is waiting for the next release Nov'23 November 2023 release proposal Proposal for a new functionality in Ocelot
Milestone
Nov-December'23

Comments

@weichaohh
Copy link

weichaohh commented Jul 11, 2023 •
edited by raman-m
Loading

Expected Behavior / New Feature

File: RequestMapper.cs
Method: Task<HttpContent> MapContent(HttpRequest request)
Line:

var content = new ByteArrayContent(await ToByteArray(request.Body));

There are performance issues.
Maybe Can change to use ArrayPool like YARP, This can reduce GC byte[].
@raman-m raman-m added bug Identified as a potential bug medium effort Likely a few days of development effort proposal Proposal for a new functionality in Ocelot labels Jul 11, 2023
@raman-m
Copy link
Member

raman-m commented Jul 11, 2023

Hi weichao!
Thanks for your interest in Ocelot gateway!

Could you correct your English text description please? It is hard to read it!

In general I understand the issue, but we need to work on description and user scenario to accept the issue.

Do you have some draft solution with a fix?

@raman-m raman-m added question Initially seen a question could become a new feature or bug or closed ;) waiting Waiting for answer to question or feedback from issue raiser labels Jul 11, 2023
@raman-m raman-m changed the title ToByteArray(request.Body) performance RequestMapper performance issue: ToByteArray(request.Body) Jul 11, 2023
@weichaohh
Copy link
Author

weichaohh commented Jul 12, 2023 •
edited by raman-m
Loading

Hi
Thanks for your reply,
Sorry, my english leve very low.

I have a large number of iot,they post real time data to the platform. every time convert client request httpcontent to destination must use memStream.ToArray() to allocate new new byte[]. this will increase the burden of GC.

I found that YARP → StreamCopier.cs use ArrayPool<byte>.Shared to copy stream, that can buffer byte[], so reduce GC. I think this is a good choice

@raman-m
Copy link
Member

raman-m commented Jul 14, 2023

Yes, this is a good choice!
Are you going to create a PR to fix this problem?
Have you provided some performance tests and/or comparison of YARP and Ocelot?

@weichaohh weichaohh removed their assignment Jul 16, 2023
@ggnaegi ggnaegi mentioned this issue Oct 9, 2023
Merged
@raman-m raman-m added accepted Bug or feature would be accepted as a PR or is being worked on and removed question Initially seen a question could become a new feature or bug or closed ;) medium effort Likely a few days of development effort waiting Waiting for answer to question or feedback from issue raiser labels Oct 15, 2023
@raman-m
Copy link
Member

raman-m commented Oct 15, 2023

The issue has been accepted due to opened PR #1724

@raman-m
Copy link
Member

raman-m commented Oct 18, 2023 •
edited
Loading

@ggnaegi Gui, the author doesn't want to fix and closed the issue 😄 What will we do?
So, because we're close to finishing and you're working on #1724 , I've decided to reopen this issue. OK?

@raman-m raman-m reopened this Oct 18, 2023
@ggnaegi
Copy link
Member

ggnaegi commented Oct 20, 2023

Ok @raman-m could you assign me? Thanks!

@raman-m
Copy link
Member

raman-m commented Oct 20, 2023

Ok @raman-m could you assign me? Thanks!

You're assigned.

@raman-m raman-m added high High priority Nov'23 November 2023 release labels Nov 9, 2023
@raman-m raman-m added this to the November'23 milestone Nov 18, 2023
raman-m added a commit that referenced this issue Dec 13, 2023
@ggnaegi @raman-m
#827 #1679 Improve performance of Request Mapper (#1724)
a17242d 
* using ArrayPool<byte> instead of memorystream CopyToAsync

* Maybe this could be a solution. It should be checked. Adding StreamHttpContent (yarp)

* little bit of cleanup here

* Avoiding ToLower() in IsSupportedHeader, using StringComparer.OrdinalIgnoreCase

* for smaller payloads, avoid allocating a buffer that is larger than the announced content length

* adding some unit tests for stream http content tests

* typo

* GivenThereIsAPossiblyBrokenServiceRunningOn

* Some code refactorings after code review. There are still some todos, but providing some more improvements, removing the exception handling from RequestMapper. It's handled in the middleware now, we will need to analyze this in detail.

* Some code cleanup

* removing some commented code bits

* adding more information in InvalidOperationException

* changing some methods signatures in request mapper, making them static.

* code review

* Update gotchas.rst

* Update notsupported.rst

* Update gotchas.rst

Add "Maximum request body size" notes

* With this fix, the system is able to handle content with size > 2 Gb

* adding new benchmarks, generating the payloads on the fly, from 1KB to 1024MB

* Review PayloadBenchmarks

* valid JSON

* should make sure the payloads directory exists

* Payloads folder name should match test method name

---------

Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>
@raman-m raman-m added merged Issue has been merged to dev and is waiting for the next release and removed accepted Bug or feature would be accepted as a PR or is being worked on labels Dec 13, 2023
@raman-m raman-m mentioned this issue Feb 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ggnaegi ggnaegi

Labels
bug Identified as a potential bug high High priority merged Issue has been merged to dev and is waiting for the next release Nov'23 November 2023 release proposal Proposal for a new functionality in Ocelot
Projects
None yet
Milestone
Nov-December'23
Development

Successfully merging a pull request may close this issue.

#827 #1679 Improve performance of Request Mapper ggnaegi/Ocelot
3 participants
@weichaohh @raman-m @ggnaegi