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

Switch MemoryStream to Microsoft.IO.RecyclableMemoryStream. #6093

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

adamradocz
Copy link

@adamradocz adamradocz commented Nov 2, 2024

Closes #6087

Initial commit switching from MemorySteam to Microsoft.IO.RecyclableMemoryStream.
What do you think about the implementation in general? If OK, I have added a few comments in the code that should be addressed.


This change is Reviewable

@bobhauser
Copy link
Contributor

bobhauser commented Nov 13, 2024

It feels to me that this might also be accomplished using an IMemoryStreamManager interface (perhaps just specifying the public methods in RecyclableMemoryStreamManager) and injecting the implementation. The default implementation could very well create an instance using the defaults you specify in your helper class. I think making the stream manager injectable would be useful for a number of reasons

  • The parent process hosting Elsa might already have a RecyclableMemoryStreamManager which they might want to reuse, and an IMemoryStreamManager implementation might be created to reference it.
  • The default settings you specify might not be appropriate in all instances
  • Perhaps the usage of memory streams in a particular application is very occasional, and it might be better to just use bare MemoryStreams to avoid losing the allocated (and cached) memory forever
  • It might be desired to attach to the RecyclableMemoryStreamManager events, for instance to log when a stream is requested to expand beyond the maximum length

@adamradocz
Copy link
Author

  • IMemoryStreamManager
  1. I can't see the IMemoryStreamManager in the code. Can you provide a link, please? Also, if you use DI, you can't use the cache in static classes.
  2. Configuration via options could be used for sure.

@sfmskywalker
Copy link
Member

I think @bobhauser is proposing to introduce a new interface called IMemoryStreamManager rather than using static helper classes. I haven't immersed myself into this PR yet, but do you think that might be a good idea @adamradocz ?

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.

[CHORE] Use Microsoft.IO.RecyclableMemoryStream
3 participants