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

ActualConnection: wrap BufReader around TcpStream #61

Merged
merged 1 commit into from
Oct 7, 2015

Conversation

astro
Copy link
Contributor

@astro astro commented Aug 7, 2015

Hi

I am just starting with Rust, and tried to receive messages with this library's experimental Pubsub support. The parser reads byte by byte, performing a lot of syscalls. What's your opinion on using BufReader to reduce the performance impact?

Safety not guaranteed. It still fails for me sometimes with Invalid byte in response

avoids doing one read syscall per byte read
@astro
Copy link
Contributor Author

astro commented Aug 8, 2015

It still fails for me sometimes with Invalid byte in response

Scratch that. redis kept disconnecting me because the network in between couldn't keep up.

@mitsuhiko
Copy link
Contributor

The reason I did not use buf reader is because it did not seem like a good idea to read past what the responses indicate is safe to read. But I pulled your changes from #63 in.

@astro
Copy link
Contributor Author

astro commented Aug 19, 2015

Are you concerned about blocking behaviour? I believe if not enough data is available the read from BufStream will return with what's available just like TcpStream.

In my case that's not relevant because all I'm doing with redis is receiving pubsub notifications at a high rate. This change lowers my CPU usage by a lot.

@xitep
Copy link

xitep commented Aug 30, 2015

today i gave redis-rs a try and while i generally like its api i wondered why it is that slow. after some basic profiling i basically came to the same conclusion as @astro. reading the tcp stream char-by-char is definitely a show-stopper (for me) :( at least astro's patch gives my program a considerable speed-up.

@xitep xitep mentioned this pull request Sep 1, 2015
@astro
Copy link
Contributor Author

astro commented Oct 1, 2015

FYI: my workload changed from just PubSub to interactive commands as well. I'm running with this patch and it's working fine.

mitsuhiko added a commit that referenced this pull request Oct 7, 2015
ActualConnection: wrap BufReader around TcpStream
@mitsuhiko mitsuhiko merged commit 3a19bc1 into redis-rs:master Oct 7, 2015
@mitsuhiko
Copy link
Contributor

Let's see how this goes.

barshaul pushed a commit to barshaul/redis-rs that referenced this pull request Nov 12, 2023
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

3 participants