fix for perl-redis issue #20 #21

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants

Sysread fixes the Windows issue reported here:
melo#20

Change to sysread
Sysread fixes the Windows issue reported here:
#20

This is just a proposed solution - you may even want to only have windows clients use sysread as well, similar to your ioctl statement, so that other clients can benefit from read() - though I see you're already using syswrite() :

"...so mixing this with reads (other than sysread()), print, write, seek, tell, or eof may cause confusion because the perlio and stdio layers usually buffer data." - http://perldoc.perl.org/functions/syswrite.html

Just wanted to propose a solution with documentation to try and help out, though I understand if you have a better idea for implementation as my understanding of Perl isn't nearly as vast as the other contributors to this project.

Owner

melo commented May 25, 2012

I cannot do that...

I can mix read with syswrite, but not read with sysread. We use read because we want to use <> to read lines, so it needs to go through the perl I/O stack. See the ungetc().

The reason for using read and the I/O stack is performance. Redis uses a lot of "read line until \r\n" and <> absolutely wipes the floor of any sysread-based solution I could come up with.

The fact that this commit didn't make the test suite fail is a very big bug in the test suite :).

I'll keep this pull request around as a reminder of something that did work on windows. I need to look at the I/O parts of Redis.pm again, to come up with something. Worts case, I will drop perl I/O on windows and switch to pure, slower, sysread solution there.

Thanks for tracking this down and proposing this solution.

I thought that would've been the case as well, though on both my work laptop (windows 7, 4gb ram, i5-2540M connected to the redis server via a gigE ssh tunnel) and running it locally on my dev linux server running redis itself I noticed negligible performance hits. These are just two (limited) tests, but it seems to me the bottleneck is something other than read vs sysread right now. Here are my benchmarks run on my linux dev box using the master 'Redis.pm' where notated as 'with read()' and where not notated, it's using sysread: http://dudid.com/readvssysread.pdf

The key is messed up a bit - I forgot to change the key and axis labels. blue is average requests/sec and red is total requests. That's what I get for trying to make a 5 minute pivot chart to better show the differences rather than just the values. Here are the values: http://dudid.com/readvssysread.html

Also - please excuse the crappy html page - I used the html export in excel on my work machine...
Office products are notoriously bad at saving in any format that isn't their own.

Owner

melo commented Jun 18, 2012

I'll be merging this tonight but modified for now just to run on Windows.

I would love to use sysread only everywhere but as I said, at the time I benchmarked it and found it to be slow. I see that your benchmarks tell a different story.

I'll try and find a morning to brush up the sysread version of Redis.pm and benchmark and profile them both. If they are in acceptable distance of each other, I'll switch to sysread only.

Thanks for your work.

iafan commented Aug 30, 2012

Hi guys, is there any ETA on having this merged?

Owner

melo commented Aug 30, 2012

I got caught up with work. I should have free time again next week to do a Redis release, there are several pull requests I want to merge.

melo added a commit that referenced this pull request Sep 4, 2012

Be more flexible when detecting Windows:
We need to know if we are using windows to decide which code to use to set
a socket to non-blocking behaviour in __fh_nonblocking (used by
__try_read_sock).

We had reports of blocking on Windows (see #20 and #21), and the
solution given on #21 is to use replace read() with sysread() in
__try_read_sock(). The fact that this works is a point in its favor,
but after that call, we do a ungetc() to put back what we've read.

According to Perl documentation, we should not mix sysread (unbuffered
I/O) with read/ungetc (buffered I/O) so I don't really like the sysread
solution.

So I'll try this first instead: the logic behind this commit is that for
some reason, the socket is not in non-blocking mode when it reaches the
read() call, and it blocks. This should catch more cases of mswin32 usage,
and it might fix the issue for good. If it does not, I'll quickly relase
a sysread-based release but that uses sysread only on Windows (I've tested
sysread on UNIX systems and it would break as expected).

Signed-off-by: Pedro Melo <melo@simplicidade.org>

melo added a commit that referenced this pull request Sep 4, 2012

Replace read() with sysread() in __try_read_sock() if we are running …
…Win32

See #20 and #21 and also 6a517e64510 for the logic behind this.

Signed-off-by: Pedro Melo <melo@simplicidade.org>
Owner

melo commented Sep 4, 2012

See melo#20 (comment)

Basically, release 1.952 on CPAN has an attempt to fix this in another way, but I have the code ready in 881b451 with the sysread version.

I'll release that one fast if this 1.952 doesn't work.

Owner

melo commented Oct 10, 2012

I've release 1.954 with this fix

@melo melo closed this Oct 10, 2012

@iafan iafan referenced this pull request Jul 9, 2013

Closed

Improve pipelined mode #49

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment