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

Use RecyclableMemoryStream instead of MemoryStream #133

Merged
merged 3 commits into from
Feb 14, 2024

Conversation

Odonno
Copy link
Contributor

@Odonno Odonno commented Aug 8, 2023

What is done on this PR:

I have a benchmark in place for a project I am working on. Here are the result benchmarks.

Before:

Method Mean Error StdDev Gen0 Gen1 Allocated
HttpWithClientFactory 1,036.2 μs 8.33 μs 7.79 μs 15.6250 3.9063 253.87 KB
WsText 4,626.6 μs 49.26 μs 43.67 μs 39.0625 15.6250 2675.36 KB
WsBinary NA NA NA NA NA NA

After:

Method Mean Error StdDev Gen0 Gen1 Allocated
HttpWithClientFactory 1,033.3 us 4.92 us 4.37 us 15.6250 3.9063 253.67 KB
WsText 3,212.6 us 60.18 us 53.34 us 27.3438 7.8125 1166.44 KB
WsBinary NA NA NA NA NA NA

I put the HttpWithClientFactory in reference because it should have a very close performance impact to WsText. CPU time and memory allocation are reduced by 80%-120%. Of course, it depends on the use case. For some scenarii, there are absolutely no impact (small batch, few/no data consumption).

@Odonno Odonno force-pushed the feat/binary-memory branch from 3db795e to 6f9ebbd Compare August 9, 2023 19:04
@Odonno Odonno changed the title Export binary memory on ResponseMessage Use RecyclableMemoryStream instead of MemoryStream Aug 9, 2023
@Odonno Odonno force-pushed the feat/binary-memory branch 3 times, most recently from fcc0b85 to 29a419c Compare August 12, 2023 14:55
@Odonno
Copy link
Contributor Author

Odonno commented Aug 12, 2023

Some notes:

  • ResponseMessage now has a MemoryStream property
  • Dispose of the Stream need to be handled manually for this so we can access the stream
  • A new property on the client: IsStreamDisposedAutomatically, allows to handle the dispose manually to have access to inner MemoryStream...
  • ...default value is true, stream will be disposed automatically/immediately and so we do not provide access to the MemoryStream (as it is disposed before access)
  • refactor the code around ReceiveClient to fully use the RecyclableMemoryStream & use recommended Memory<byte> instead of ArraySegment<byte> (except for .NET Standard 2.0) => I did not perceive any significant impact in terms of performance/memory allocation doing that, and the code is way cleaner

I made some final touches and now it is good for review.

@Odonno Odonno force-pushed the feat/binary-memory branch from 29a419c to 781367b Compare August 12, 2023 16:32
@Odonno
Copy link
Contributor Author

Odonno commented Aug 12, 2023

Just a final final touch:

  • Add SendTextAsBinary and SendTextAsBinaryAsync methods to send TEXT message without having to serialize again from string to byte[]. On benchmark, it reduces memory allocation by half.

@Marfusios
Copy link
Owner

Thanks @Odonno for a nicely prepared PR, I'm going to review it

@Odonno
Copy link
Contributor Author

Odonno commented Sep 18, 2023

Hi @Marfusios

Thank you for your effort. I resolved the conflicts after your changes. Mostly nullable reference types. Not sure if I am 100% compliant on this topic. Will make some check later to see if it works as expected.

@Odonno
Copy link
Contributor Author

Odonno commented Sep 24, 2023

Since you upgraded to .NET Standard 2.1, I removed pragma conditions for .NET Standard 2.0.

@Odonno
Copy link
Contributor Author

Odonno commented Oct 3, 2023

Hi @Marfusios

When do you think you can review this PR?

@kresseandreas51
Copy link

kresseandreas51 commented Oct 5, 2023 via email

@Marfusios
Copy link
Owner

Hey, finally got to it, thanks for the PR, looks good to me from here, will test it locally after the merge and do some slight polishing

@Marfusios Marfusios merged commit caba38b into Marfusios:master Feb 14, 2024
1 check passed
@Odonno Odonno deleted the feat/binary-memory branch February 14, 2024 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants