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

Buffer payloads asynchronously when appropriate #2297

Merged
merged 2 commits into from
Apr 13, 2023
Merged

Conversation

mattjohnsonpint
Copy link
Contributor

@mattjohnsonpint mattjohnsonpint commented Apr 12, 2023

During serialization, we buffer payloads of EnvelopeItems into memory so we can calculate lengths to include in headers.

As part of #1965 (released in 3.22.0), I switched to synchronous buffering of these payloads - because serialization of a fully materialized object to a MemoryStream gains no advantage by being async, and doing so adds some minor overhead.

That is still true, however I didn't consider that not all payloads are fully materialized. In particular, a StreamSerializable could be representing an attachment, in which case it would be reading from a FileStream or any other type of Stream (depending on how it was attached). Also, we may need other types of async materialization in the future.

Thus, we should defer to synchronous buffering only when we know the object is fully materialized - which is what our JsonSerializable class represents.

@SeanFeldman
Copy link
Contributor

@mattjohnsonpint, it looks like there's a lot of work taking place with MemoryStream, and this is a hot path. Would there be any benefit in taking advantage of RecyclableMemoryStream?

@vaind vaind merged commit 5a339f5 into main Apr 13, 2023
@vaind vaind deleted the fix/async-buffering branch April 13, 2023 08:16
@mattjohnsonpint
Copy link
Contributor Author

@SeanFeldman - RecyclableMemoryStream looks useful indeed. Created #2299 to track. Thanks!

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