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

Add subject type #952

Merged
merged 11 commits into from
Oct 4, 2023
Merged

Add subject type #952

merged 11 commits into from
Oct 4, 2023

Conversation

caspervonb
Copy link
Collaborator

@caspervonb caspervonb commented May 1, 2023

Adds a bytes based immutable string type for subjects.
Goal is to have the same interface as String, minus the mutability.

  • Make it compile
  • Make tests compile
  • Make examples compile
  • Update documentation and make it compile
  • Take out redundant allocations
  • Forward Bytes directly where applicable

Closes #950

@Jarema
Copy link
Member

Jarema commented May 3, 2023

We should validate subjects when publishing differently than when subscribing.

When subscribing, we only validate for a valid subject.
When publishing, we can't use wildcards.

Not sure when the check should happen yet.

@caspervonb
Copy link
Collaborator Author

caspervonb commented May 4, 2023

We should validate subjects when publishing differently than when subscribing.

Aye, this is just a String like interface without the mutability. utf8 is guaranteed because str guarantees it.

Thinking about prior art a bit, think validating at the call site is fine. E.g think Path.

@n1ghtmare n1ghtmare self-assigned this Jul 21, 2023
@n1ghtmare n1ghtmare force-pushed the add-subject-type branch 2 times, most recently from 9af9ebf to 86f0d81 Compare July 21, 2023 19:10
@caspervonb caspervonb marked this pull request as ready for review July 26, 2023 14:26
@caspervonb
Copy link
Collaborator Author

caspervonb commented Aug 3, 2023

Seeing a slight performance bump from main

async-nats: publish messages amount/32                                                                           
                        time:   [52.064 µs 52.322 µs 52.550 µs]
                        thrpt:  [1.9029 Melem/s 1.9113 Melem/s 1.9207 Melem/s]
                 change:
                        time:   [-3.7262% -0.2156% +2.1908%] (p = 0.91 > 0.05)
                        thrpt:  [-2.1438% +0.2161% +3.8704%]
                        No change in performance detected.
Found 8 outliers among 30 measurements (26.67%)
  1 (3.33%) low severe
  4 (13.33%) low mild
  2 (6.67%) high mild
  1 (3.33%) high severe
async-nats: publish messages amount/1024                                                                           
                        time:   [101.89 µs 102.36 µs 103.02 µs]
                        thrpt:  [970.68 Kelem/s 976.95 Kelem/s 981.45 Kelem/s]
                 change:
                        time:   [-0.5065% +0.0175% +0.5152%] (p = 0.95 > 0.05)
                        thrpt:  [-0.5126% -0.0175% +0.5090%]
                        No change in performance detected.
async-nats: publish messages amount/8192                                                                            
                        time:   [684.27 µs 690.45 µs 699.05 µs]
                        thrpt:  [143.05 Kelem/s 144.83 Kelem/s 146.14 Kelem/s]
                 change:
                        time:   [-8.6033% -5.2521% -2.8634%] (p = 0.00 < 0.05)
                        thrpt:  [+2.9478% +5.5432% +9.4131%]
                        Performance has improved.

Waiting on Xcode to finish updating so I can run instruments to check the heap profile on this.

@caspervonb caspervonb requested a review from Jarema August 3, 2023 06:58
@caspervonb
Copy link
Collaborator Author

caspervonb commented Aug 5, 2023

Instrumented allocations (left is this branch, right is main)
TL;DR main ends up having 6GB of transient allocations we don't have with this type.
image

@Jarema
Copy link
Member

Jarema commented Aug 10, 2023

@caspervonb reminder - resolve the conflicts please.

Copy link
Collaborator

@n1ghtmare n1ghtmare left a comment

Choose a reason for hiding this comment

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

LGTM! Tiniest of tiny nits...

@@ -445,7 +447,7 @@ impl ConnectionHandler {
length,
} => {
if let Some(subscription) = self.subscriptions.get_mut(&sid) {
let message = Message {
let message: Message = Message {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let message: Message = Message {
let message = Message {

This change might not be needed (most probably added by me, during rebase...).

@MattesWhite
Copy link
Contributor

We should validate subjects when publishing differently than when subscribing.

When subscribing, we only validate for a valid subject. When publishing, we can't use wildcards.

Not sure when the check should happen yet.

Just wanted to through in a reminder that #596 exists 😄. So the only thing I really would like to see when a Subject type is introduced is the possibility to iterate over tokens of a subject. This makes it way more comfortable to check subjects of a messages from a wildcard subscription for what was actually sent, e.g.:

  • Subscription to my-app.*.data
  • Receive message from my-app.1234.data
  • Use Subject::tokens().nth(1) to be able to parse the app's ID

All in all, a nice implementation 👍 I really like the use of Bytes to prevent allocations.

@MattesWhite MattesWhite mentioned this pull request Aug 30, 2023
@caspervonb caspervonb assigned caspervonb and unassigned n1ghtmare Sep 25, 2023
Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

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

LGTM!

@Jarema Jarema merged commit 76d16e7 into nats-io:main Oct 4, 2023
10 checks passed
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.

Use Bytes with a string interface for subjects to avoid allocations.
4 participants