Skip to content

make headers buffer non-nullable #778

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

caleblloyd
Copy link
Collaborator

Small optimization to stop wrapping headers buffer in Nullable

I think this may technically be a breaking change in its current state due to changing the signature in NatsSubBase.ReceiveAsync. But that method is not part of INatsSub. @mtmk do you know of any intended uses of NatsSubBase.ReceiveAsync outside of other projects in the solution?

@caleblloyd caleblloyd requested a review from mtmk March 13, 2025 05:14
Copy link
Member

@mtmk mtmk left a comment

Choose a reason for hiding this comment

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

I was thinking of it as making a distinction between headers existence but it has to have the top version line (protocol wise version line might not be there - in that case it would only signal it's a HPUB message rather than PUB, which is not important once the message is parsed off the wire) so checking the length is sufficient. technically as you mentioned it is a breaking change and i am increasingly more aware we should not make breaking changes even though i very much doubt anyone is using it.

_inbox.ReceivedAsync(subject, replyTo, headersBuffer, payloadBuffer, _connection);

// Not used. Dummy implementation to keep base happy.
protected override ValueTask ReceiveInternalAsync(string subject, string? replyTo, ReadOnlySequence<byte>? headersBuffer, ReadOnlySequence<byte> payloadBuffer)
protected override ValueTask ReceiveInternalAsync(string subject, string? replyTo, ReadOnlySequence<byte> headersBuffer, ReadOnlySequence<byte> payloadBuffer)
Copy link
Member

@mtmk mtmk Mar 13, 2025

Choose a reason for hiding this comment

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

should we create an overload instead to make it non-breaking?
edit: hmm that would break it even more it's an abstract class. can you think of a way to not make it breaking change?
edit2:wrong file. i meant the abstract method on NatsSubBase

@caleblloyd
Copy link
Collaborator Author

Going to mark this as draft, I'd like to do 2 things:

  1. Write a microbench to make sure this actually results in a performance improvement. Since Nullable<T> isn't a readonly struct there may be defensive copies, but I need to see whether or not the compiler optimizes it
  2. If the benchmark shows an improvement, add remarks to the public methods in NatsSubBase that do not appear in INatsSub about compatibility guarantees. These methods are likely rarely used by external users of the library, although it is still nice to have them public to give power users the option of leveraging them. I was thinking we could do something similar to EF Core

@caleblloyd caleblloyd marked this pull request as draft March 13, 2025 13:34
@to11mtm
Copy link
Collaborator

to11mtm commented Mar 22, 2025

Going to mark this as draft, I'd like to do 2 things:

1. Write a microbench to make sure this actually results in a performance improvement.  Since `Nullable<T>` isn't a readonly struct there may be defensive copies, but I need to see whether or not the compiler optimizes it

2. If the benchmark shows an improvement, add remarks to the public methods in `NatsSubBase` that do not appear in `INatsSub` about compatibility guarantees.  These methods are likely rarely used by external users of the library, although it is still nice to have them public to give power users the option of leveraging them.  I was thinking we could do something [similar to EF Core](https://github.com/dotnet/efcore/blob/b0da78396a068811a1c5467e5124253038e382b1/src/EFCore.Design/Query/Internal/QueryLocator.cs#L20-L25)

As far as point two, FWIW, In Akka.NET they use an InternalApiAttribute alongside PublicApiGenerator to both document public APIs as well as make it easy to 'fast check' public API changes. Contributors are expected to run the tests if they are changing any APIs; that will re-generate the files and makes it easy in PR to catch breaking changes; CI also goes ahead and runs the API tests, and if results don't match the PR build fails (a failsafe in case someone forgot to rerun the API tests.) Mind you, none of this -masks- stuff with the attribute, however the attribute gets printed in our public API spec and the attribute is at least as visible in an IDE as XMLDoc... (that said, having 'INTERNAL API' at the start of XMLDoc is the convention we have, just to cover bases...)

Point 1 is far more interesting...

This isn't quite apples to apples but shows how even a -similar- comparison causes some extra instructions and more register/stack pressure. Basically the nullable is 32 bytes instead of 24, however the increased pressure can at times have a large impact...

So, I'm for it TBH

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