#827 #1679 Improve performance of Request Mapper

[ggnaegi]

Fixes #1679 #827

• 1679

• 827

Proposed Changes

• implementing custom HttpContent, largely inspired by the yarp project. https://github.com/microsoft/reverse-proxy/blob/main/src/ReverseProxy/Forwarder/StreamCopyHttpContent.cs

/// 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: Figure 1: develop branch code

And then I checked the results of this PR 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]

@raman-m this is largely inspired by yarp.

[raman-m]

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

[ggnaegi]

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

[raman-m]

@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]

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

[ggnaegi]

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

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

[ggnaegi]

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

[raman-m]

Will this PR fix #749 ?

[raman-m]

Guys, This PR goes to November release... So, I'll postpone a code review.

[ggnaegi]

Yet another interesting library: https://github.com/microsoft/Microsoft.IO.RecyclableMemoryStream

[raman-m]

Microsoft.IO.RecyclableMemoryStream

Wow! Not bad! 🤩 How did you find this lib? Deep googling? 😁

[ggnaegi]

Microsoft.IO.RecyclableMemoryStream

Wow! Not bad! 🤩 How did you find this lib? Deep googling? 😁

it just appeared out of nowhere, it's magic... Let me think about a possible usage...

[raman-m]

Rebasing of the feature branch is required

[ggnaegi]

@raman-m rebase done!

[raman-m]

@raman-m rebase done!

Once again, please! :smiley: Conflicts! :sos:

[raman-m]

Guys, we're at the finish line... 😃

@ggnaegi I see that we have 28 MB payload test in benchmarks. Some crazy people are using our gateway to download/upload large video files 🤣

Could we add canonic 10 Mb, 100 Mb and 1 Gb payload tests too please?

[ggnaegi]

@raman-m to be honest I'm not very happy with your review, it's not factual. Your mixing new features with what was already implemented. And why no content length, it's because it will have an impact on performance. We could remove md5 too, you're right.

[raman-m]

Gui, I am not happy with Ocelot repo and project. It is very outdated and the project has tons of problems... You see? I am not happy with the quality of PRs of other contributors (not your ones)... This is hard project. But rewriting from scratch is also a bad idea...

Regarding my code review... I will help to resolve my review issues. But can you add more payloads in benchmark tests please as I wrote in prev message?

[ggnaegi]

@raman-m as you wished, it's ready for a new review...

[ggnaegi]

@raman-m I can't push the 1GB payload, it's 100MB maximum

[raman-m]

Ah, sorry... To keep this 100mb payload as a file in git? If Yes that will be not good.

[ggnaegi]

Ah, sorry... To keep this 100mb payload as a file in git? If Yes that will be not good.

You asked for bigger payloads. 100mb is fine, but 1gb is too big for github. So it's not part of the benchmarks now. I hope it's fine for you... Or I could create the payload on the fly, it's maybe better for the end users.

[raman-m]

Yes, We should refactor benchmarks to operate and process in-memory objects rather than file system ones. But this is not critical now... Is it difficult to refactor to switch from file to in-memory or stream objects? If Yes, please ignore this problem now. Adding 100mb file to git is bad for sure...

It seems it is time to create Assets branch in repo to save static files aka blobs... Will do that soon ...

[ggnaegi]

@raman-m So for the 10 100 and 1024Mb payloads, i have provided a method to generate dummy content. As for the json's we will think about a solution later. I have removed those payloads (10, 100 and 1024MB) from the repo.

[raman-m]

I've got another idea... It is totally fine to generate large files during test setup step rather than keep them as static files in Git... But seems you're going to refactor in favor of memory streams...

[ggnaegi]

@raman-m no no, I'm creating the payloads when starting the benchmarks for the first time. No memory stream. It's pushed, you can review!

/// <summary>
    /// Generates a dummy payload of the given size in MB.
    /// Avoiding to maintain a large file in the repository.
    /// </summary>
    /// <param name="size">The file size in MB.</param>
    /// <param name="payloadPath">The path to the payload file.</param>
    /// <returns>The payload as byte array.</returns>
    /// <exception cref="ArgumentNullException">Throwing an exception if the payload path is null.</exception>
    private byte[] GenerateDummyPayload(int size, string payloadPath)
    {
        if (payloadPath == null)
        {
            throw new ArgumentNullException(nameof(payloadPath));
        }

        if (File.Exists(payloadPath))
        {
            return File.ReadAllBytes(payloadPath);
        }

        var newFile = new FileStream(payloadPath, FileMode.CreateNew);
        newFile.Seek(size * 1024L * 1024, SeekOrigin.Begin);
        newFile.WriteByte(0);
        newFile.Close();

        return File.ReadAllBytes(payloadPath);
    }

[raman-m]

Gui, gotchas.rst looks good now. 😃 It is a part of docs updating. This PR changes internal logic of content forwarder, so we should not have many updates in docs. I will think more what we can update in docs... but seems not much... Any ideas on docs updating are welcome!

[RaynaldM]

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]

@RaynaldM commented on Dec 14

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