-
Notifications
You must be signed in to change notification settings - Fork 55
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
WIP: 282-Create InFlightNatsMsg to improve channel buffer usage. #290
base: main
Are you sure you want to change the base?
WIP: 282-Create InFlightNatsMsg to improve channel buffer usage. #290
Conversation
Numbers so far are promising... With one exception there is a 5-20% boost on read/write to the channels. The case where we are suffering badly is
|
Very very very close. Just have to improve the 25% dip on unhappy case and I feel like the other regressions will drop off as concerns:
|
Here is a Draft PR based on our preliminary discussions in #282 as well as observations I've made, at least as far as how to keep this from being a breaking change or involving too much hard to maintain code (Putting a DU-esque
object
insideNatsMsg<T>
results in a -lot- of painful code, especially if fields get added in the future.)This does, however, result in a possibly (minorly) breaking change I was not initially aware of; the constructor for
NatsJSMsg<T>
was made public, and one of the optimizations that was made there was to use_context.Connection
(since that way, we can get away with not converting to a message and also makeNatsJSMsg<T>
smaller).I would not expect this to break anything internally; however if end users have tests that depend on the behavior of a
NatsJSMsg
's connection being set to something that isn't what the consumer has... it may cause them problems.We can always just not take the improvement on
NatsJSMsg
for now if this is too worrisome.I'll also note, we -can- also DU
NatsMsg<T>
as well if we wanted, however for a record struct of that style it is a bit more problematic.