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

Support all binary payload APIs with only generics #143

Merged
merged 9 commits into from
Oct 6, 2023

Conversation

mtmk
Copy link
Collaborator

@mtmk mtmk commented Oct 5, 2023

With this proposal we reduce the API footprint and make the default behaviour work as expected when sending byte types and strings:

await using var sub = await SubscribeAsync<T>(string subject);

await PublishAsync<T>(string subject, T data);

await PublishAsync(string subject); // send empty message / sentinel

The default serializer is smart enough to do the right thing handling known types avoiding surprises, especially when publishing.

// Send as binary data
await PublishAsync(string subject, new byte[] {1,2,3});
await PublishAsync(string subject, new Memory(bytes));
await PublishAsync(string subject, myMemory); // IMemoryOwner

// JSON serialized
await PublishAsync(string subject, complexObject);

The idea is to reduce the code duplication between generic calls and non-generic calls using binary data as payload and make the defaults work for the vast majority of the cases while having the flexibility of defining a custom serializer.

Related to #137

@mtmk mtmk requested a review from caleblloyd October 5, 2023 00:19
@mtmk mtmk changed the title Support bytes apis via generics Support bytes APIs using generics only Oct 5, 2023
@mtmk mtmk mentioned this pull request Oct 5, 2023
@mtmk mtmk changed the title Support bytes APIs using generics only Support all binary payload APIs Oct 5, 2023
* Added switcher so we can add more benchmarks
* Added a benchmark comparing pub wait-until-sent option
@mtmk mtmk removed the request for review from caleblloyd October 5, 2023 11:59
@mtmk mtmk marked this pull request as draft October 5, 2023 11:59
@mtmk mtmk changed the title Support all binary payload APIs [Proposal] Support all binary payload APIs Oct 5, 2023
String serialized is causing excess allocations.
@mtmk
Copy link
Collaborator Author

mtmk commented Oct 5, 2023

Performance considerations for publish:

Without this change:

| Method             | Iter | Mean        | Error         | StdDev      | Gen0   | Allocated |
|------------------- |----- |------------:|--------------:|------------:|-------:|----------:|
| WaitUntilSentTrue  | 64   |  2,995.8 us |   1,481.06 us |    81.18 us |      - |    6987 B |
| WaitUntilSentFalse | 64   |    156.2 us |      21.80 us |     1.19 us |      - |     591 B |
| WaitUntilSentTrue  | 512  | 26,541.7 us | 115,608.79 us | 6,336.91 us |      - |   53614 B |
| WaitUntilSentFalse | 512  |    439.3 us |      63.39 us |     3.47 us | 0.4883 |    9575 B |
| WaitUntilSentTrue  | 1024 | 45,792.0 us |  29,602.46 us | 1,622.61 us |      - |  106941 B |
| WaitUntilSentFalse | 1024 |    711.4 us |      64.15 us |     3.52 us | 2.9297 |   46649 B |

With this change:

| Method             | Iter | Mean        | Error        | StdDev      | Gen0   | Allocated |
|------------------- |----- |------------:|-------------:|------------:|-------:|----------:|
| WaitUntilSentTrue  | 64   |  2,894.7 us |  1,268.27 us |    69.52 us |      - |    6990 B |
| WaitUntilSentFalse | 64   |    161.6 us |     23.09 us |     1.27 us |      - |     520 B |
| WaitUntilSentTrue  | 512  | 21,691.3 us | 11,121.19 us |   609.59 us |      - |   53606 B |
| WaitUntilSentFalse | 512  |    438.5 us |     70.65 us |     3.87 us | 0.4883 |   10168 B |
| WaitUntilSentTrue  | 1024 | 44,436.2 us | 37,207.27 us | 2,039.46 us |      - |  106921 B |
| WaitUntilSentFalse | 1024 |    738.8 us |    111.61 us |     6.12 us | 2.9297 |   44885 B |

@mtmk mtmk changed the title [Proposal] Support all binary payload APIs [Proposal] Support all binary payload APIs with only generics Oct 5, 2023
mtmk added 4 commits October 5, 2023 15:54
This is to make the purpose of this method more obvious.
Complicates the API. Documentation should be enough.
@mtmk mtmk changed the title [Proposal] Support all binary payload APIs with only generics Support all binary payload APIs with only generics Oct 5, 2023
@mtmk mtmk marked this pull request as ready for review October 5, 2023 20:59
Copy link
Collaborator

@caleblloyd caleblloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent! I really like how it simplifies the codebase.

Left one comment, but don't let that hold up merging this PR


await foreach (var msg in sub.Msgs.ReadAllAsync())
{
var data = Encoding.UTF8.GetString(msg.Data.ToArray());
var data = Encoding.UTF8.GetString(msg.Data!);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the null-forgiving operator needed here? Is SubscribeAsync<T> eventually returning T? instead? Would be nice if we could avoid that

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default values in general yield empty/sentinel payloads that's why there are T?s in a lot of places. Do you think we shouldn't allow nulls? How would we handle sending and receiving empty payloads?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I guess there is no way to avoid it for reference types when default may be retruned. Seeing as System.Text.JSON even does it

@mtmk mtmk merged commit 1220541 into main Oct 6, 2023
9 checks passed
@mtmk mtmk deleted the support-bytes-apis-via-generics branch October 6, 2023 08:13
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.

2 participants