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.

Sign up for GitHub

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

Jump to bottom

Multiplex requests over a single subscription #1069

Merged
merged 14 commits into from
Aug 14, 2023
Merged

Multiplex requests over a single subscription #1069

merged 14 commits into from
Aug 14, 2023

Conversation

caspervonb
Copy link
Collaborator

@caspervonb caspervonb commented Aug 2, 2023 •
edited
Loading

No description provided.

@caspervonb caspervonb force-pushed the multiplex-request-responses branch from 11ac516 to a662f06 Compare August 2, 2023 04:07
@caspervonb
Copy link
Collaborator Author

So we might have to take out inbox on request builder. Or keep legacy requests in for just that scenario.

@caspervonb caspervonb requested a review from Jarema August 2, 2023 05:35
@caspervonb caspervonb marked this pull request as ready for review August 5, 2023 04:58
@caspervonb
Copy link
Collaborator Author

Some benches against main


     Running benches/core_nats.rs (target/release/deps/core_nats-9f9b494bfb99583b)
Gnuplot not found, using plotters backend
Benchmarking request amount/32: Warming up for 3.0000 s
Warning: Unable to complete 10 samples in 5.0s. You may wish to increase target time to 6.5s.
request amount/32       time:   [673.88 ms 682.44 ms 692.60 ms]
                        thrpt:  [14.438 Kelem/s 14.653 Kelem/s 14.840 Kelem/s]
                 change:
                        time:   [-9.2688% -7.6984% -5.9234%] (p = 0.00 < 0.05)
                        thrpt:  [+6.2964% +8.3405% +10.216%]
                        Performance has improved.
Found 2 outliers among 10 measurements (20.00%)
  1 (10.00%) low mild
  1 (10.00%) high severe
Benchmarking request amount/1024: Warming up for 3.0000 s
Warning: Unable to complete 10 samples in 5.0s. You may wish to increase target time to 6.6s.
request amount/1024     time:   [686.72 ms 691.14 ms 695.54 ms]
                        thrpt:  [14.377 Kelem/s 14.469 Kelem/s 14.562 Kelem/s]
                 change:
                        time:   [-9.2669% -8.2935% -7.5109%] (p = 0.00 < 0.05)
                        thrpt:  [+8.1208% +9.0435% +10.213%]
                        Performance has improved.
Found 1 outliers among 10 measurements (10.00%)
  1 (10.00%) low mild
Benchmarking request amount/8192: Warming up for 3.0000 s
Warning: Unable to complete 10 samples in 5.0s. You may wish to increase target time to 7.4s.
request amount/8192     time:   [769.48 ms 780.60 ms 797.06 ms]
                        thrpt:  [12.546 Kelem/s 12.811 Kelem/s 12.996 Kelem/s]
                 change:
                        time:   [-7.0389% -4.8250% -2.2368%] (p = 0.00 < 0.05)
                        thrpt:  [+2.2879% +5.0696% +7.5719%]
                        Performance has improved.
Found 1 outliers among 10 measurements (10.00%)
  1 (10.00%) high severe

     Running unittests src/lib.rs (target/release/deps/nats-2e00c16348cd9698)

@caspervonb caspervonb force-pushed the multiplex-request-responses branch from c5a18c3 to 2be8253 Compare August 5, 2023 19:05
Jarema
Jarema requested changes Aug 9, 2023
View reviewed changes
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.

Do the TODO :)

async-nats/src/lib.rs Outdated Show resolved Hide resolved
@caspervonb caspervonb force-pushed the multiplex-request-responses branch from 4518292 to 618cf14 Compare August 10, 2023 00:05
Jarema
Jarema requested changes Aug 12, 2023
View reviewed changes
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.

Looks good!

One change request.

async-nats/src/lib.rs Outdated
@@ -484,6 +501,26 @@ impl ConnectionHandler {
self.handle_flush().await?;
}
}
} else if let Some(multiplexer) = self.multiplexer.as_mut() {
Copy link
Member

Choose a reason for hiding this comment

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

I would check if sid is as we expect, just to avoid any edge cases.

@caspervonb caspervonb force-pushed the multiplex-request-responses branch from f2e247c to a0b7a58 Compare August 14, 2023 13:42
Jarema
Jarema approved these changes Aug 14, 2023
View reviewed changes
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 474cb14 into nats-io:main Aug 14, 2023
@Jarema Jarema mentioned this pull request Aug 14, 2023
Open
16 tasks
@Jarema Jarema mentioned this pull request Sep 21, 2023
Merged
@caspervonb caspervonb mentioned this pull request Oct 5, 2023
Closed
@paolobarbolini paolobarbolini mentioned this pull request Oct 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jarema Jarema Jarema approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@caspervonb @Jarema