Skip to content

Optimize memory usage by leveraging CommunityToolkit.HighPerformance for buffer pooling #1381

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

Closed
wants to merge 1 commit into from

Conversation

abnercpp
Copy link

@abnercpp abnercpp commented May 5, 2023

This PR proposes leveraging the Microsoft official package CommunityToolkit.HighPerformance (alongside System.Memory and System.Collections.Immutable) to optimize memory usage by removing intermediate allocations where possible. This is only a small change to the project, but such is the nature of optimization: we can work with incremental steps.

Description

This PR aims to optimize memory usage for a few methods of the ImageMagick.MagickImage class, namely:

  • void Trim(Gravity[] edges) now rents a buffer from the shared memory pool when converting edges to their string representations, which means we potentially get rid of intermediate allocations. (Also added a ReadOnlySpan<Gravity> overload for users that want to do their own pooling or stack allocate the parameters.)
  • Methods writing to a stream or a file now potentially get rid of intermediate allocations caused by ToByteArray() by renting a buffer from the shared pool.
  • ToByteArray() now potentially avoids intermediate allocations by writing the data into ArrayPoolBufferWriter<byte> instead of MemoryStream, which leverages the array pool to avoid unnecessary allocations, especially when dynamic resizes are needed.

@CLAassistant
Copy link

CLAassistant commented May 5, 2023

CLA assistant check
All committers have signed the CLA.

@dlemstra
Copy link
Owner

dlemstra commented May 6, 2023

Thanks for creating this pull request but I don't want to add an extra dependency to this library for such a small change. But maybe we could use ArrayPool<byte>.Shared in the .NET Standard 2.1 code and reduce the allocations in WriteAsync? But I think we could also do that by creating a ByteArrayWrapper that uses an implementation that is similar to StreamWrapper to reduce allocations. I will see if I can make this change.

And to be honest the GravityExtensions really feels like an over optimization.

@dlemstra
Copy link
Owner

dlemstra commented May 6, 2023

I am going to close this pull request because I don't want to add the change in this form.

@dlemstra dlemstra closed this May 6, 2023
@abnercpp
Copy link
Author

abnercpp commented May 6, 2023

Thanks for creating this pull request but I don't want to add an extra dependency to this library for such a small change. But maybe we could use ArrayPool<byte>.Shared in the .NET Standard 2.1 code and reduce the allocations in WriteAsync? But I think we could also do that by creating a ByteArrayWrapper that uses an implementation that is similar to StreamWrapper to reduce allocations. I will see if I can make this change.

And to be honest the GravityExtensions really feels like an over optimization.

Thank you for the fast and direct response. I really appreciate the feedback.

I can switch implementations by leveraging System.Memory directly. However that would require a dependency on System.Memory instead of CommunityToolkit.HighPerformance for netstandard2.0. (.NET6+ already comes with System.Memory.) Is that okay with you?

The gravity extensions were more of an attempt to clean up that part of the code than an optimization, but I can undo that with no problem.

@dlemstra
Copy link
Owner

dlemstra commented May 7, 2023

I now have a proof of concept where I use the technique from the StreamWrapper in a new class to reduce the total allocated memory. Will need to make sure all my current test pass with this new class and add some extra tests before I commit these changes. This class will be used in the implementation of the ToByteArray method of the MagickImage class so I don't think we need extra improvements in that area. Maybe we could use an array pool there but because I need to change the size a lot I wonder if we should do this. We could have a look at it after my changes get merged. And thanks again for pointing this issue out.

I don't want to add an extra dependency to the netstandard2.0 code. But I am open for changes that improve parts of the netstandard2.1 code without adding extra dependencies.

p.s. If you are interested in improving performance and allocations then feel free to take a look at my magick-wasm project and see if I can do some smart things there. And I just realized that I will probably also need some help with using ToByteArray in the unit tests instead of writing and reading from a MemoryStream to reduce the allocations in the unit tests.

@abnercpp abnercpp deleted the fork-main branch May 7, 2023 16:56
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