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

perf(async): Only allocate one oneshot per request #194

Merged
merged 1 commit into from
Mar 16, 2019

Conversation

Marwes
Copy link
Collaborator

@Marwes Marwes commented Mar 11, 2019

Changes SharedConnection to collect the values in the worker task
before sending sending them back instead of sending each response value
in it's own oneshot.

This should generally improve performance for pipelined queries since
both allocations and synchronization is reduced. The only downside is
that there is no way to read responses as each one comes in but this
isn't used today.

(I wouldn't put to much stock in the exact numbers as they have pretty high variance, however this PR should always do less work than before).

query/simple_getsetdel_async
                        time:   [190.66 us 196.74 us 203.39 us]a
                        change: [-10.222% -6.5229% -2.8090%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild

query_pipeline/shared_async_implicit_pipeline
                        time:   [4.7329 ms 4.7641 ms 4.8001 ms]
                        thrpt:  [208.33 Kelem/s 209.90 Kelem/s 211.29 Kelem/s]
                 change:
                        time:   [-24.133% -23.438% -22.678%] (p = 0.00 < 0.05)
                        thrpt:  [+29.329% +30.613% +31.809%]
                        Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
  5 (5.00%) high mild
  3 (3.00%) high severe
query_pipeline/shared_async_long_pipeline
                        time:   [2.5665 ms 2.5807 ms 2.5998 ms]
                        thrpt:  [384.64 Kelem/s 387.49 Kelem/s 389.63 Kelem/s]
                 change:
                        time:   [-23.663% -23.095% -22.537%] (p = 0.00 < 0.05)
                        thrpt:  [+29.093% +30.030% +30.999%]
                        Performance has improved.
Found 13 outliers among 100 measurements (13.00%)
  3 (3.00%) low mild
  4 (4.00%) high mild
  6 (6.00%) high severe

@badboy
Copy link
Collaborator

badboy commented Mar 13, 2019

What's the thrpt line in the benchmark output?
(needs a rebase now I guess)

@badboy
Copy link
Collaborator

badboy commented Mar 13, 2019

(I wouldn't put to much stock in these)

To much stock into what?

@Marwes
Copy link
Collaborator Author

Marwes commented Mar 13, 2019

What's the thrpt line in the benchmark output?

It is for Throughput (requests/second).

To much stock into what?

Reworded it as I wouldn't put to much stock in the exact numbers as they have pretty high variance, however this PR should always do less work than before

@badboy
Copy link
Collaborator

badboy commented Mar 13, 2019

👍

Changes `SharedConnection` to collect the values in the worker task
before sending sending them back instead of sending each response value
in it's own `oneshot`.

This should generally improve performance for pipelined queries since
both allocations and synchronization is reduced. The only downside is
that there is no way to read responses as each one comes in but this
isn't used today.

(I wouldn't put to much stock in these)
```
query/simple_getsetdel_async
                        time:   [190.66 us 196.74 us 203.39 us]
                        change: [-10.222% -6.5229% -2.8090%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild

query_pipeline/shared_async_implicit_pipeline
                        time:   [4.7329 ms 4.7641 ms 4.8001 ms]
                        thrpt:  [208.33 Kelem/s 209.90 Kelem/s 211.29 Kelem/s]
                 change:
                        time:   [-24.133% -23.438% -22.678%] (p = 0.00 < 0.05)
                        thrpt:  [+29.329% +30.613% +31.809%]
                        Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
  5 (5.00%) high mild
  3 (3.00%) high severe
query_pipeline/shared_async_long_pipeline
                        time:   [2.5665 ms 2.5807 ms 2.5998 ms]
                        thrpt:  [384.64 Kelem/s 387.49 Kelem/s 389.63 Kelem/s]
                 change:
                        time:   [-23.663% -23.095% -22.537%] (p = 0.00 < 0.05)
                        thrpt:  [+29.093% +30.030% +30.999%]
                        Performance has improved.
Found 13 outliers among 100 measurements (13.00%)
  3 (3.00%) low mild
  4 (4.00%) high mild
  6 (6.00%) high severe
query_pipeline/async_long_pipeline
                        time:   [2.6208 ms 2.6438 ms 2.6718 ms]
                        thrpt:  [374.28 Kelem/s 378.24 Kelem/s 381.57 Kelem/s]
                 change:
                        time:   [-0.8913% -0.0976% +0.6138%] (p = 0.80 > 0.05)
                        thrpt:  [-0.6101% +0.0976% +0.8993%]
                        No change in performance detected.
Found 11 outliers among 100 measurements (11.00%)
  4 (4.00%) high mild
  7 (7.00%) high severe
```
@badboy badboy merged commit 8fd8413 into redis-rs:master Mar 16, 2019
@Marwes Marwes deleted the avoid_oneshot_alloc branch March 16, 2019 23:01
barshaul pushed a commit to barshaul/redis-rs that referenced this pull request Jul 11, 2024
* Java: Add `BZMPOP` command (redis-rs#194)

* Add `BZMPOP` command.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

* Add a test.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

* Address PR comments.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

* Reorder args.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

* Groom glide core.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

* Update glide-core/src/client/value_conversion.rs

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

Co-authored-by: jonathanl-bq <72158117+jonathanl-bq@users.noreply.github.com>

---------

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Co-authored-by: jonathanl-bq <72158117+jonathanl-bq@users.noreply.github.com>

* Address PR comments.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

* Update glide-core/src/client/value_conversion.rs

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

Co-authored-by: Andrew Carbonetto <andrew.carbonetto@improving.com>

* Rename

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

---------

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Co-authored-by: jonathanl-bq <72158117+jonathanl-bq@users.noreply.github.com>
Co-authored-by: Andrew Carbonetto <andrew.carbonetto@improving.com>
barshaul pushed a commit to barshaul/redis-rs that referenced this pull request Oct 9, 2024
…is-rs#194)

* Added new routing option: `SingleNodeRoutingInfo::RandomPrimary`

Signed-off-by: Eran Ifrah <eifrah@amazon.com>
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