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: Optimize the command encoding #165

Merged
merged 2 commits into from
Mar 26, 2019
Merged

perf: Optimize the command encoding #165

merged 2 commits into from
Mar 26, 2019

Conversation

Marwes
Copy link
Collaborator

@Marwes Marwes commented Sep 7, 2018

Not as important as IO latency but can still be a decent throughput improvement.

(Lots of variance in the benchmarks but there is still some clear improvements)

encode/pipeline         time:   [727.07 us 731.89 us 737.21 us]
                        change: [-59.635% -58.716% -57.668%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
  2 (2.00%) high mild
  4 (4.00%) high severe
encode/pipeline_nested  time:   [246.61 us 249.05 us 251.81 us]
                        change: [-71.597% -70.950% -70.333%] (p = 0.00 < 0.05)
                        Performance has improved.
encode/integer          time:   [693.79 us 698.95 us 703.98 us]
                        change: [-63.633% -63.125% -62.651%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
  4 (4.00%) high mild
encode/small            time:   [1.1458 us 1.1580 us 1.1701 us]
                        change: [-40.826% -39.741% -38.664%] (p = 0.00 < 0.05)
                        Performance has improved.

I could probably refactor to remove these breaking changes but for the moment...

BREAKING CHANGE

ToRedisArgs has been changed to take take an instance of RedisWrite instead of Vec<Vec<u8>>. Use the write_arg method instead of Vec::push.

impl Iterator is used which requires 1.26

iter now takes self by value instead of cloning self inside the method.

@Marwes
Copy link
Collaborator Author

Marwes commented Sep 7, 2018

cc #81

@Marwes Marwes changed the title [WIP] perf: Optimize the command encoding perf: Optimize the command encoding Sep 12, 2018
@Marwes
Copy link
Collaborator Author

Marwes commented Sep 12, 2018

If #167 is merged a breaking release is needed anyways so perhaps it is fine to use impl Iterator then as well.

@Marwes
Copy link
Collaborator Author

Marwes commented Sep 25, 2018

ping @badboy

@badboy
Copy link
Collaborator

badboy commented Sep 27, 2018

I won't get to review it this week

@Marwes
Copy link
Collaborator Author

Marwes commented Feb 18, 2019

I rebased and squashed this since there was some files accidentally included and to fix the conflicts.

@badboy
Copy link
Collaborator

badboy commented Feb 18, 2019

I rebased and squashed this since there was some files accidentally included and to fix the conflicts.

I was just about to ask :D

@badboy
Copy link
Collaborator

badboy commented Feb 18, 2019

Test failures due to formatting (because I merged #167). I'm fine with merging as is and fixing afterwards.

src/cmd.rs Show resolved Hide resolved
@badboy
Copy link
Collaborator

badboy commented Mar 16, 2019

Ah damn, of course this has merge conflicts now and thus will need a rebase (and resolving those conflicts).

But otherwise I would be fine with merging it now.

Markus Westerlind added 2 commits March 17, 2019 00:02
Changes the way commands are encoded to reduce the number of separate
allocations made during encoding.
@Marwes
Copy link
Collaborator Author

Marwes commented Mar 19, 2019

Rebased!

@badboy badboy merged commit f0dfd5b into redis-rs:master Mar 26, 2019
@Marwes Marwes deleted the opt branch March 26, 2019 22:30
barshaul pushed a commit to barshaul/redis-rs that referenced this pull request Jul 11, 2024
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.

None yet

2 participants