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

WIP: 282-Create InFlightNatsMsg to improve channel buffer usage. #290

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

to11mtm
Copy link
Contributor

@to11mtm to11mtm commented Dec 16, 2023

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 inside NatsMsg<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 make NatsJSMsg<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.

@to11mtm
Copy link
Contributor Author

to11mtm commented Dec 17, 2023

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 ReadAsync where we have to wait on the internal reader; At the moment the custom ChannelReader doesn't have any pooling for it's async operations. I tried to add pooling however... I'm definitely doing something wrong there since alloc numbers didn't change. 😅 Not sure if it's my lack of full understanding of how our pooling works or something I'm doing wrong in the implementation... or if something more thought out is required.

BenchmarkDotNet v0.13.8, Windows 10 (10.0.19045.3803/22H2/2022Update)
Intel Core i7-8750H CPU 2.20GHz (Coffee Lake), 1 CPU, 12 logical and 6 physical cores
.NET SDK 8.0.100
  [Host]   : .NET 6.0.16 (6.0.1623.17311), X64 RyuJIT AVX2
  .NET 8.0 : .NET 8.0.0 (8.0.23.53103), X64 RyuJIT AVX2

Job=.NET 8.0  Runtime=.NET 8.0  
Method Mean Error StdDev Gen0 Allocated
RunNatsMsgChannel_Sync 61.41 us 0.623 us 0.552 us - -
RunInFlightNatsMsgChannel_Sync 55.17 us 0.225 us 0.199 us - -
RunNatsMsgChannel_Sync_Pulse 59.99 us 0.315 us 0.263 us - -
RunInFlightNatsMsgChannel_Sync_Pulse 54.54 us 0.256 us 0.239 us - -
RunNatsMsgChannel_Async_Pulse 87.39 us 0.311 us 0.276 us - -
RunInFlightNatsMsgChannel_Async_Pulse 82.23 us 0.368 us 0.326 us - -
RunNatsMsgChannel_Async_Pulse_Unhappy 178.55 us 1.279 us 1.196 us 31.2500 147456 B
RunInFlightNatsMsgChannel_Async_Pulse_Unhappy 784.90 us 8.744 us 8.179 us 80.0781 377081 B
RunNatsMsgChannel_Async_Unhappy 119.70 us 1.230 us 1.091 us 0.1221 887 B
RunInFlightNatsMsgChannel_Async_Unhappy 100.23 us 1.514 us 1.416 us 0.2441 1308 B
RunNatsMsgChannel_Async_Happy 117.07 us 0.640 us 0.599 us - 408 B
RunInFlightNatsMsgChannel_Async_Happy 92.05 us 1.206 us 1.069 us - 408 B

@to11mtm
Copy link
Contributor Author

to11mtm commented Dec 17, 2023

Fixed the pooling issues, however I'm still seeing a huge perf loss in the 'nightmare' case (even though memory usage is now better than the old Reader.) Need to profile to see what's going on.

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:

Method Mean Error StdDev Gen0 Allocated
RunNatsMsgChannel_Sync 61.62 us 0.624 us 0.553 us - -
RunInFlightNatsMsgChannel_Sync 57.67 us 0.394 us 0.368 us - -
RunNatsMsgChannel_Sync_Pulse 60.45 us 0.202 us 0.158 us - -
RunInFlightNatsMsgChannel_Sync_Pulse 56.52 us 0.275 us 0.258 us - -
RunNatsMsgChannel_Async_Pulse 88.67 us 0.274 us 0.229 us - -
RunInFlightNatsMsgChannel_Async_Pulse 94.57 us 0.498 us 0.441 us - -
RunNatsMsgChannel_Async_Pulse_Unhappy 192.28 us 1.355 us 1.201 us 31.2500 147456 B
RunInFlightNatsMsgChannel_Async_Pulse_Unhappy 245.43 us 4.011 us 3.752 us 27.8320 131072 B
RunNatsMsgChannel_Async_Unhappy 123.47 us 1.597 us 1.494 us - 900 B
RunInFlightNatsMsgChannel_Async_Unhappy 128.66 us 1.880 us 1.666 us - 833 B
RunNatsMsgChannel_Async_Happy 117.61 us 0.932 us 0.872 us - 408 B
RunInFlightNatsMsgChannel_Async_Happy 121.40 us 0.817 us 0.764 us - 408 B

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.

None yet

1 participant