Feature #4531: [PATCH 0/7] use poll() instead of select() in certain cases

• Status: Closed
• Assignee: kosaki (Motohiro KOSAKI)
• Target version: 2.0.0

[normalperson (Eric Wong)]

=begin
ref: [ruby-core:35527]

This adds a new C API function with the following prototype:

rb_io_poll_fd(int fd, short events, int timeout);

It is emulated using select() for platforms that we do not support
poll() for. It is much easier to use than rb_thread_select() and
rb_thread_fd_select() for the common case in C extensions[1].

For Linux (and eventually any other platforms where poll() works for all
select()-able files), we actually implement rb_io_poll_fd() using the
poll() system call which means it is faster for high numbered file
descriptors and does not put malloc pressure on the garbage collector.

Lastly, since IO.select() is commonly used with a single IO object
in my experience, we will try to use rb_io_poll_fd() in that case.

There is also a new testcase for io/wait since I needed to verify my
changes to ext/io/wait.c were correct.

No failures were introduced to test-all and test-rubyspec targets with
either the select() or poll()-based implementation of rb_io_poll_fd()
on my platform (Linux x86_64)

[1] see patches for changes I made in ext/socket/init.c, ext/io/wait.c,
and ext/readline/readline.c:
$ git diff --stat origin/trunk -- ext
ext/io/wait/wait.c | 34 +++-------------------
ext/readline/readline.c | 6 +---
ext/socket/init.c | 72 ++++++++---------------------------------------
3 files changed, 18 insertions(+), 94 deletions(-)

=end

[kosaki (Motohiro KOSAKI)]

=begin

ref: [ruby-core:35527]

This adds a new C API function with the following prototype:

  rb_io_poll_fd(int fd, short events, int timeout);

It is emulated using select() for platforms that we do not support
poll() for.  It is much easier to use than rb_thread_select() and
rb_thread_fd_select() for the common case in C extensions[1].

For Linux (and eventually any other platforms where poll() works for all
select()-able files), we actually implement rb_io_poll_fd() using the
poll() system call which means it is faster for high numbered file
descriptors and does not put malloc pressure on the garbage collector.

Meta review comment. All performance patches should have mesured
performance number.

And, if you really want to care C10K scalability issue, why don't you use
epoll?

Lastly, since IO.select() is commonly used with a single IO object
in my experience, we will try to use rb_io_poll_fd() in that case.

There is also a new testcase for io/wait since I needed to verify my
changes to ext/io/wait.c were correct.

No failures were introduced to test-all and test-rubyspec targets with
either the select() or poll()-based implementation of rb_io_poll_fd()
on my platform (Linux x86_64)
=end

[normalperson (Eric Wong)]

• File io_select_using_poll_test.rb io_select_using_poll_test.rb added

=begin
Added a contrived test case, using a high RLIMIT_NOFILE (e.g. ulimit -n 16384) is recommended to make the results more apparent.
=end

[normalperson (Eric Wong)]

=begin
KOSAKI Motohiro [e-mail] wrote:

ref: [ruby-core:35527]

This adds a new C API function with the following prototype:

  rb_io_poll_fd(int fd, short events, int timeout);

It is emulated using select() for platforms that we do not support
poll() for.  It is much easier to use than rb_thread_select() and
rb_thread_fd_select() for the common case in C extensions[1].

For Linux (and eventually any other platforms where poll() works for all
select()-able files), we actually implement rb_io_poll_fd() using the
poll() system call which means it is faster for high numbered file
descriptors and does not put malloc pressure on the garbage collector.

Meta review comment. All performance patches should have mesured
performance number.

Based on a contrived test case:
http://redmine.ruby-lang.org/attachments/1562/io_select_using_poll_test.rb
I get the following results:

---- before NOFILE=1024 ---
max fd: 1024 (results not apparent with <= 1024 max fd)
last IO: #<IO:fd 1022>
2.050000 1.440000 3.490000 ( 3.476264)

GC.count: 386

---- after NOFILE=1024 ---
max fd: 1024 (results not apparent with <= 1024 max fd)
last IO: #<IO:fd 1022>
2.430000 0.320000 2.750000 ( 2.734643)

GC.count: 343

---- before NOFILE=16384 ---
max fd: 16384 (results not apparent with <= 1024 max fd)
last IO: #<IO:fd 16382>
3.610000 5.680000 9.290000 ( 9.266017)

GC.count: 643

---- after NOFILE=16384 ---
max fd: 16384 (results not apparent with <= 1024 max fd)
last IO: #<IO:fd 16382>
2.970000 0.450000 3.420000 ( 3.415426)

GC.count: 394

I don't have real world results/test case but I think small bits
like this count for the future.

And, if you really want to care C10K scalability issue, why don't you use
epoll?

I assume you mean Ruby programming in general and not making MRI itself
use epoll().

I find synchronous programming easier especially when dealing with
existing libraries (I use Ruby to make programming life easier :)

For me, thousands of native threads (NPTL + 64-bit) often makes more
sense than epoll() + non-blocking I/O.

Concurrent connections isn't always an issue, either, but also sometimes
have many open file handles to support other tasks[1].

I mainly think select() is a horrible interface and having extension
authors to deal with HAVE_RB_FD_INIT and remembering rb_fd_term() is
dangerous. Small bits of pressure from Rubyists can hopefully steer
other OSes towards better poll() support.

Thank you for your time!

[1] DB connection pool, large memcached pool, regular files,
various message queues, etc...

--
Eric Wong
=end

[kosaki (Motohiro KOSAKI)]

• Assignee set to kosaki (Motohiro KOSAKI)

=begin

=end

[kosaki (Motohiro KOSAKI)]

=begin
2011/3/29 Eric Wong [e-mail]:

KOSAKI Motohiro [e-mail] wrote:

ref: [ruby-core:35527]

This adds a new C API function with the following prototype:

  rb_io_poll_fd(int fd, short events, int timeout);

It is emulated using select() for platforms that we do not support
poll() for.  It is much easier to use than rb_thread_select() and
rb_thread_fd_select() for the common case in C extensions[1].

For Linux (and eventually any other platforms where poll() works for all
select()-able files), we actually implement rb_io_poll_fd() using the
poll() system call which means it is faster for high numbered file
descriptors and does not put malloc pressure on the garbage collector.

Meta review comment. All performance patches should have mesured
performance number.

Based on a contrived test case:
 http://redmine.ruby-lang.org/attachments/1562/io_select_using_poll_test.rb
I get the following results:

---- before NOFILE24 ---
max fd: 1024 (results not apparent with <
=end

[normalperson (Eric Wong)]

=begin
KOSAKI Motohiro [e-mail] wrote:

Thanks again for your reply and support :>

2011/3/29 Eric Wong [e-mail]:

I mainly think select() is a horrible interface and having extension
authors to deal with HAVE_RB_FD_INIT and remembering rb_fd_term() is
dangerous.  Small bits of pressure from Rubyists can hopefully steer
other OSes towards better poll() support.

I'm sorry. I haven't catch this. can you please explain more detail?

rb_fd_init() uses malloc() to create fdsets instead of traditional stack
allocation. This also requires using rb_fd_term() to free() memory, so
rb_ensure() must be used[1]. Often this is for waiting on a single fd,
so the rb_io_poll_fd() interface is better for extension authors all
around even if it internally uses select().

As for poll() support, Evan Phoenix pointed out poll() doesn't work on
OS X with ttys/pipes (ref: [ruby-core:35529]). I don't know about other
kernels. Ideally, poll() should work everywhere select() does (and
select() should be deprecated). I would like to encourage other OSes to
get where Linux is (or get more people using Linux :).

On a related note:

Ruby also still needlessly checks for read/writability with select()[2]
even though though we have support for blocking regions. I think
that is safe to remove if we check return values with
rb_io_wait_{read,writ}able(). One part of
http://redmine.ruby-lang.org/issues/4535 does exactly this but I have
another patch to kill more unnecessary select() calls that aren't
bugs, just extra code/syscalls.

[1] though from reading through do_select() in thread.c, I don't think
do_select() can raise..

[2] via rb_thread_wait_fd()/rb_thread_fd_writable()

--
Eric Wong
=end

[normalperson (Eric Wong)]

=begin
Eric Wong [e-mail] wrote:

On a related note:

Ruby also still needlessly checks for read/writability with select()[2]
even though though we have support for blocking regions. I think
that is safe to remove if we check return values with
rb_io_wait_{read,writ}able(). One part of
http://redmine.ruby-lang.org/issues/4535 does exactly this but I have
another patch to kill more unnecessary select() calls that aren't
bugs, just extra code/syscalls.

Just opened this one: http://redmine.ruby-lang.org/issues/4538

--
Eric Wong
=end

[kosaki (Motohiro KOSAKI)]

=begin
commnet for patch 0001.

begin
require 'io/wait'
rescue LoadError
end

shold be

begin
require 'io/wait'
rescue LoadError

skip this test.

return
end

If we can't load io/wait, why do we need to run the tests for it?
Anyway, I'll commit it.
=end

[normalperson (Eric Wong)]

=begin
Motohiro KOSAKI [e-mail] wrote:

Issue #4531 has been updated by Motohiro KOSAKI.

commnet for patch 0001.

begin
require 'io/wait'
rescue LoadError
end

shold be

begin
require 'io/wait'
rescue LoadError

skip this test.

return
end

If we can't load io/wait, why do we need to run the tests for it?
Anyway, I'll commit it.

I used a condition at the end of the class definition:

end if IO.method_defined?(:wait)

I copied the style from test/io/nonblock/test_flush.rb

--
Eric Wong
=end

[kosaki (Motohiro KOSAKI)]

=begin
I also commited io_select_using_poll_test.rb (but modified a little) at r31390.
=end

[normalperson (Eric Wong)]

KOSAKI Motohiro [e-mail] wrote:

I'd suggest to make generic single fd waiting abstract function, like below.
rb_wait_for_single_fd(int fd, int events, struct timeval *tv);

Also, RB_POLLIN should be avoided.

Huh? What would I use for events instead?

Thanks for taking your time to review this!

--
Eric Wong

[kosaki (Motohiro KOSAKI)]

Also, RB_POLLIN should be avoided.

Huh? What would I use for events instead?

Thanks for taking your time to review this!

I meant RB_POLLIN is not backend independ name.

thanks.

[normalperson (Eric Wong)]

• File rb_wait_for_single_fd.mbox rb_wait_for_single_fd.mbox added

I've updated the series with your suggestions and rebased against trunk r31412 and uploaded a new
mbox with the 6 following patches:

1. rb_wait_for_single_fd: initial implementation
2. io/wait: switch to rb_wait_for_single_fd()
3. ext/socket/init.c: simplify wait_connectable() using rb_wait_for_single_fd
4. readline: use rb_wait_for_single_fd() instead of rb_thread_fd_select()
5. io.c (IO.select): use rb_wait_for_single_fd for single IO case
6. rb_wait_for_single_fd: use poll() on Linux

Also available in my git repo:
git pull git://bogomips.org/ruby rb_wait_for_single_fd

[kosaki (Motohiro KOSAKI)]

Please do NOT send untested patch. :-/

% ./ruby-single-wait benchmark/bm_io_select3.rb
max fd: 100000 (results not apparent with <= 1024 max fd)
benchmark/bm_io_select3.rb:15: [BUG] Segmentation fault
ruby 1.9.3dev (2011-05-03 trunk 31412) [x86_64-linux]

-- Control frame information -----------------------------------------------
c:0007 p:---- s:0021 b:0021 l:000020 d:000020 CFUNC :select
c:0006 p:0019 s:0016 b:0016 l:000738 d:000015 BLOCK benchmark/bm_io_select3.rb:15
c:0005 p:---- s:0014 b:0014 l:000013 d:000013 FINISH
c:0004 p:---- s:0012 b:0012 l:000011 d:000011 CFUNC :times
c:0003 p:0094 s:0009 b:0009 l:000738 d:001e00 EVAL benchmark/bm_io_select3.rb:14
c:0002 p:---- s:0004 b:0004 l:000003 d:000003 FINISH
c:0001 p:0000 s:0002 b:0002 l:000738 d:000738 TOP

-- Ruby level backtrace information ----------------------------------------
benchmark/bm_io_select3.rb:14:in <main>' benchmark/bm_io_select3.rb:14:in times'
benchmark/bm_io_select3.rb:15:in block in <main>' benchmark/bm_io_select3.rb:15:in select'

-- C level backtrace information -------------------------------------------
./ruby-single-wait() [0x528508] vm_dump.c:797
./ruby-single-wait() [0x572226] error.c:249
./ruby-single-wait(rb_bug+0xb1) [0x573631] error.c:266
./ruby-single-wait() [0x4b8420] signal.c:624
/lib64/libpthread.so.0() [0x33bf20eeb0]
/lib64/libc.so.6() [0x33bea884bb]
./ruby-single-wait(rb_thread_fd_select+0x340) [0x5307e0] thread.c:2393
./ruby-single-wait() [0x42e00c] io.c:7298
./ruby-single-wait(rb_ensure+0xab) [0x41758b] eval.c:745
./ruby-single-wait() [0x434d27] io.c:7713
./ruby-single-wait() [0x524a2d] vm_insnhelper.c:403
./ruby-single-wait() [0x519224] insns.def:1012
./ruby-single-wait() [0x51eb8e] vm.c:1163
./ruby-single-wait() [0x51f8e5] vm.c:574
./ruby-single-wait(rb_yield+0x44) [0x525504] vm.c:604
./ruby-single-wait() [0x446861] numeric.c:3225
./ruby-single-wait() [0x524a2d] vm_insnhelper.c:403
./ruby-single-wait() [0x519224] insns.def:1012
./ruby-single-wait() [0x51eb8e] vm.c:1163
./ruby-single-wait(rb_iseq_eval_main+0xbf) [0x525e3f] vm.c:1404
./ruby-single-wait() [0x4147d2] eval.c:215
./ruby-single-wait(ruby_run_node+0x34) [0x416da4] eval.c:262
./ruby-single-wait() [0x414379] main.c:38
/lib64/libc.so.6(__libc_start_main+0xfd) [0x33bea1ee5d]
./ruby-single-wait() [0x414269]

-- Other runtime information -----------------------------------------------

• Loaded script: benchmark/bm_io_select3.rb

• Loaded features:

  0 enumerator.so
  1 /home/kosaki/ruby/lib/ruby/1.9.1/x86_64-linux/enc/encdb.so
  2 /home/kosaki/ruby/lib/ruby/1.9.1/x86_64-linux/enc/trans/transdb.so
  3 /home/kosaki/ruby/lib/ruby/1.9.1/rubygems/defaults.rb
  4 /home/kosaki/ruby/lib/ruby/1.9.1/tsort.rb
  5 /home/kosaki/ruby/lib/ruby/1.9.1/rubygems/dependency_list.rb
  6 /home/kosaki/ruby/lib/ruby/1.9.1/x86_64-linux/rbconfig.rb
  7 /home/kosaki/ruby/lib/ruby/1.9.1/rubygems/exceptions.rb
  8 /home/kosaki/ruby/lib/ruby/1.9.1/rubygems/custom_require.rb
  9 /home/kosaki/ruby/lib/ruby/1.9.1/rubygems.rb

• Process memory map:

00400000-00625000 r-xp 00000000 fd:03 2626950 /home/kosaki/linux/ruby/ruby-single-wait
00824000-00827000 rw-p 00224000 fd:03 2626950 /home/kosaki/linux/ruby/ruby-single-wait
00827000-0083f000 rw-p 00000000 00:00 0
014b6000-032bf000 rw-p 00000000 00:00 0 [heap]
33be200000-33be21f000 r-xp 00000000 fd:00 4980738 /lib64/ld-2.13.so
33be41e000-33be41f000 r--p 0001e000 fd:00 4980738 /lib64/ld-2.13.so
33be41f000-33be420000 rw-p 0001f000 fd:00 4980738 /lib64/ld-2.13.so
33be420000-33be421000 rw-p 00000000 00:00 0
33bea00000-33beb91000 r-xp 00000000 fd:00 4980744 /lib64/libc-2.13.so
33beb91000-33bed91000 ---p 00191000 fd:00 4980744 /lib64/libc-2.13.so
33bed91000-33bed95000 r--p 00191000 fd:00 4980744 /lib64/libc-2.13.so
33bed95000-33bed96000 rw-p 00195000 fd:00 4980744 /lib64/libc-2.13.so
33bed96000-33bed9c000 rw-p 00000000 00:00 0
33bee00000-33bee02000 r-xp 00000000 fd:00 4980748 /lib64/libdl-2.13.so
33bee02000-33bf002000 ---p 00002000 fd:00 4980748 /lib64/libdl-2.13.so
33bf002000-33bf003000 r--p 00002000 fd:00 4980748 /lib64/libdl-2.13.so
33bf003000-33bf004000 rw-p 00003000 fd:00 4980748 /lib64/libdl-2.13.so
33bf200000-33bf217000 r-xp 00000000 fd:00 4980758 /lib64/libpthread-2.13.so
33bf217000-33bf416000 ---p 00017000 fd:00 4980758 /lib64/libpthread-2.13.so
33bf416000-33bf417000 r--p 00016000 fd:00 4980758 /lib64/libpthread-2.13.so
33bf417000-33bf418000 rw-p 00017000 fd:00 4980758 /lib64/libpthread-2.13.so
33bf418000-33bf41c000 rw-p 00000000 00:00 0
33bf600000-33bf684000 r-xp 00000000 fd:00 4980756 /lib64/libm-2.13.so
33bf684000-33bf883000 ---p 00084000 fd:00 4980756 /lib64/libm-2.13.so
33bf883000-33bf884000 r--p 00083000 fd:00 4980756 /lib64/libm-2.13.so
33bf884000-33bf885000 rw-p 00084000 fd:00 4980756 /lib64/libm-2.13.so
33bfa00000-33bfa15000 r-xp 00000000 fd:00 4980798 /lib64/libgcc_s-4.5.1-20100924.so.1
33bfa15000-33bfc14000 ---p 00015000 fd:00 4980798 /lib64/libgcc_s-4.5.1-20100924.so.1
33bfc14000-33bfc15000 rw-p 00014000 fd:00 4980798 /lib64/libgcc_s-4.5.1-20100924.so.1
33c0200000-33c0207000 r-xp 00000000 fd:00 4980764 /lib64/librt-2.13.so
33c0207000-33c0406000 ---p 00007000 fd:00 4980764 /lib64/librt-2.13.so
33c0406000-33c0407000 r--p 00006000 fd:00 4980764 /lib64/librt-2.13.so
33c0407000-33c0408000 rw-p 00007000 fd:00 4980764 /lib64/librt-2.13.so
33caa00000-33caa07000 r-xp 00000000 fd:00 4980814 /lib64/libcrypt-2.13.so
33caa07000-33cac07000 ---p 00007000 fd:00 4980814 /lib64/libcrypt-2.13.so
33cac07000-33cac08000 r--p 00007000 fd:00 4980814 /lib64/libcrypt-2.13.so
33cac08000-33cac09000 rw-p 00008000 fd:00 4980814 /lib64/libcrypt-2.13.so
33cac09000-33cac37000 rw-p 00000000 00:00 0
33cae00000-33cae5d000 r-xp 00000000 fd:00 4980811 /lib64/libfreebl3.so
33cae5d000-33cb05c000 ---p 0005d000 fd:00 4980811 /lib64/libfreebl3.so
33cb05c000-33cb05e000 rw-p 0005c000 fd:00 4980811 /lib64/libfreebl3.so
33cb05e000-33cb062000 rw-p 00000000 00:00 0
7f6892403000-7f68924e3000 rw-p 00000000 00:00 0
7f6892509000-7f689250b000 r-xp 00000000 fd:03 1725292 /home/kosaki/ruby/lib/ruby/1.9.1/x86_64-linux/enc/trans/transdb.so
7f689250b000-7f689270a000 ---p 00002000 fd:03 1725292 /home/kosaki/ruby/lib/ruby/1.9.1/x86_64-linux/enc/trans/transdb.so
7f689270a000-7f689270b000 rw-p 00001000 fd:03 1725292 /home/kosaki/ruby/lib/ruby/1.9.1/x86_64-linux/enc/trans/transdb.so
7f689270b000-7f689270d000 r-xp 00000000 fd:03 1725319 /home/kosaki/ruby/lib/ruby/1.9.1/x86_64-linux/enc/encdb.so
7f689270d000-7f689290c000 ---p 00002000 fd:03 1725319 /home/kosaki/ruby/lib/ruby/1.9.1/x86_64-linux/enc/encdb.so
7f689290c000-7f689290d000 rw-p 00001000 fd:03 1725319 /home/kosaki/ruby/lib/ruby/1.9.1/x86_64-linux/enc/encdb.so
7f689290d000-7f689290e000 ---p 00000000 00:00 0
7f689290e000-7f6892a12000 rw-p 00000000 00:00 0
7f6892a12000-7f68988a2000 r--p 00000000 fd:00 277164 /usr/lib/locale/locale-archive
7f68988a2000-7f68988a7000 rw-p 00000000 00:00 0
7f68988c1000-7f68988c3000 rw-p 00000000 00:00 0
7fffc64c8000-7fffc64e9000 rw-p 00000000 00:00 0 [stack]
7fffc652e000-7fffc652f000 r-xp 00000000 00:00 0 [vdso]
ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0 [vsyscall]

[NOTE]
You may have encountered a bug in the Ruby interpreter or extension libraries.
Bug reports are welcome.
For details: http://www.ruby-lang.org/bugreport.html

zsh: abort (core dumped) ./ruby-single-wait benchmark/bm_io_select3.rb

[normalperson (Eric Wong)]

Motohiro KOSAKI [e-mail] wrote:

Issue #4531 has been updated by Motohiro KOSAKI.

Please do NOT send untested patch. :-/

% ./ruby-single-wait benchmark/bm_io_select3.rb
max fd: 100000 (results not apparent with <= 1024 max fd)
benchmark/bm_io_select3.rb:15: [BUG] Segmentation fault
ruby 1.9.3dev (2011-05-03 trunk 31412) [x86_64-linux]

I can't reproduce on my end, same architecture and base revision, too...
I just rebased cleanly off r31415 and still can't reproduce.

-- C level backtrace information -------------------------------------------
./ruby-single-wait() [0x528508] vm_dump.c:797
./ruby-single-wait() [0x572226] error.c:249
./ruby-single-wait(rb_bug+0xb1) [0x573631] error.c:266
./ruby-single-wait() [0x4b8420] signal.c:624
/lib64/libpthread.so.0() [0x33bf20eeb0]
/lib64/libc.so.6() [0x33bea884bb]
./ruby-single-wait(rb_thread_fd_select+0x340) [0x5307e0] thread.c:2393

My patches don't touch the rb_thread_fd_select() code. thread.c:2393
is in rb_fd_copy(), not rb_thread_fd_select(), and is the following:

memcpy(dst->fdset, src, size);

Shouldn't that be:

memcpy(dst->fdset, src->fdset, size);

since r31395?

[kosaki (Motohiro KOSAKI)]

My patches don't touch the rb_thread_fd_select() code.  thread.c:2393
is in rb_fd_copy(), not rb_thread_fd_select(), and is the following:

  memcpy(dst->fdset, src, size);

Shouldn't that be:

  memcpy(dst->fdset, src->fdset, size);

since r31395?

Right. Thanks for good catching.
I'll test your patch again.

[kosaki (Motohiro KOSAKI)]

Your patch improve single fd case. but slightly decrease multiple fd case.
The most important usecase of IO.select is web server application (ie multiple fd case).
It's no good trade-off.

name before ruby 1.9.3dev (2011-05-03 trunk 31415) [x86_64-linux]
io_select 2.282 1.596
io_select2 56.223 3.097
io_select3 13.470 13.628

[kosaki (Motohiro KOSAKI)]

---

int
rb_wait_for_single_fd(int fd, int events, struct timeval *tv)
{
(snip)
BLOCKING_REGION({
result = poll(&fds, 1, timeout);
if (result < 0) lerrno = errno;
}, ubf_select, GET_THREAD());

if (result > 0) {
    /* remain compatible with select(2)-based implementation */
    result = (int)(fds.revents & fds.events);
    return result == 0 ? events : result;
}

---

It's really unclear comment. select_internal() doesn't have such quirk.
Can you please clarify this?

---

static VALUE
select_internal(VALUE read, VALUE write, VALUE except, struct timeval *tp, rb_fdset_t fds)
{
(snip)
n = rb_thread_fd_select(max, rp, wp, ep, tp);
if (n < 0) {
rb_sys_fail(0);
}
if (!pending && n == 0) return Qnil; / returns nil on timeout */

[kosaki (Motohiro KOSAKI)]

I've commited following five patches.

0001-rb_wait_for_single_fd-initial-implementation.patch
0002-io-wait-switch-to-rb_wait_for_single_fd.patch
0003-ext-socket-init.c-simplify-wait_connectable-using-rb.patch
0004-readline-use-rb_wait_for_single_fd-instead-of-rb_thr.patch
0006-rb_wait_for_single_fd-use-poll-on-Linux.patch

But I haven't commited below one patch.

0005-io.c-IO.select-use-rb_wait_for_single_fd-for-single-.patch

I'm waiting performance fix or explanation why should we commit it.

[kosaki (Motohiro KOSAKI)]

+int
+rb_wait_for_single_fd(int fd, int events, struct timeval *tv)
+{

• struct pollfd fds;
• int result, lerrno;
• double start;
• int timeout = tv ? tv->tv_sec * 1000 + (tv->tv_usec + 500) / 1000 : -1;

no overflow check?

[normalperson (Eric Wong)]

Motohiro KOSAKI [e-mail] wrote:

I've commited following five patches.

0001-rb_wait_for_single_fd-initial-implementation.patch
0002-io-wait-switch-to-rb_wait_for_single_fd.patch
0003-ext-socket-init.c-simplify-wait_connectable-using-rb.patch
0004-readline-use-rb_wait_for_single_fd-instead-of-rb_thr.patch
0006-rb_wait_for_single_fd-use-poll-on-Linux.patch

Thanks, I'm preparing some comments + tests in response to your
comments on 0006.

But I haven't commited below one patch.

0005-io.c-IO.select-use-rb_wait_for_single_fd-for-single-.patch

I'm waiting performance fix or explanation why should we commit it.

I'm OK with rejecting 0005. It's too ugly in retrospect...

Instead the io/wait extension should be expanded to be able to check for
writability, too. Maybe by adding a new method:

IO#wait_writable(timeout = 0)

--
Eric Wong

[normalperson (Eric Wong)]

• File rb_wait_for_single_fd-followup.mbox rb_wait_for_single_fd-followup.mbox added

3 more safety patches + tests based on your comments in this thread:

1. rb_wait_for_single_fd: explain return value for poll() users
2. rb_wait_for_single_fd: have poll()-using impl set EBADF
3. rb_wait_for_single_fd: check for invalid timeval

Also available at:

git pull git://bogomips.org/ruby rb_wait_for_single_fd-followup

[kosaki (Motohiro KOSAKI)]

diff --git a/ext/-test-/wait_for_single_fd/wait_for_single_fd.c
b/ext/-test-/wait_for_single_fd/wait_for_single_fd.c
index d406724..6efd1af 100644
--- a/ext/-test-/wait_for_single_fd/wait_for_single_fd.c
+++ b/ext/-test-/wait_for_single_fd/wait_for_single_fd.c
@@ -25,6 +25,7 @@ Init_wait_for_single_fd(void)
rb_define_const(rb_cObject, "RB_WAITFD_IN", INT2NUM(RB_WAITFD_IN));
rb_define_const(rb_cObject, "RB_WAITFD_OUT", INT2NUM(RB_WAITFD_OUT));
rb_define_const(rb_cObject, "RB_WAITFD_PRI", INT2NUM(RB_WAITFD_PRI));

• rb_define_const(rb_cObject, "INT_MAX", INT2NUM(INT_MAX));
  rb_define_singleton_method(rb_cIO, "wait_for_single_fd",
  wait_for_single_fd, 3);

Strongly disagree. Any language change should be passed matz review.

@@ -2723,7 +2736,14 @@ rb_wait_for_single_fd(int fd, int events,
struct timeval *tv)
struct pollfd fds;
int result, lerrno;
double start;

• int timeout = tv ? tv->tv_sec * 1000 + (tv->tv_usec + 500) / 1000 : -1;

• int timeout = -1;
• if (tv) {

•   timeout = timeval2msec(tv);
  

•   if (timeout == -1) {
  

•       return -1;
  

•   }
  

• }

No good solusion. backend change should be transparent from caller.
I recommend following two way.

1. use ppoll(2) if available. and use INT_MAX if unavailable. or

2. fallback select(2)

3. is safe because linux has ppol(2).

   if (result > 0) {

•   /* remain compatible with select(2)-based implementation */
  

•   /*
  

•    * Remain compatible with the select(2)-based implementation:
  

•    * 1) mask out poll()-only revents such as POLLHUP/POLLERR
  

•    * 2) In case revents only consists of masked-out events, return all
  

•    *    requested events in the result and force the caller to make an
  

•    *    extra syscall (e.g. read/write/send/recv) to get the error.
  

•    */
    result = (int)(fds.revents & fds.events);
    return result == 0 ? events : result;
  

  }

I don't understand this. Why does this behavior help to compatible?
When do we use it?

2. rb_wait_for_single_fd: have poll()-using impl set EBADF
   is good fix. but a testcase of it introduce ruby level new method.
   It's unacceptable. I'll commit only code fix.

[normalperson (Eric Wong)]

KOSAKI Motohiro [e-mail] wrote:

diff --git a/ext/-test-/wait_for_single_fd/wait_for_single_fd.c
b/ext/-test-/wait_for_single_fd/wait_for_single_fd.c
index d406724..6efd1af 100644
--- a/ext/-test-/wait_for_single_fd/wait_for_single_fd.c
+++ b/ext/-test-/wait_for_single_fd/wait_for_single_fd.c
@@ -25,6 +25,7 @@ Init_wait_for_single_fd(void)
rb_define_const(rb_cObject, "RB_WAITFD_IN", INT2NUM(RB_WAITFD_IN));
rb_define_const(rb_cObject, "RB_WAITFD_OUT", INT2NUM(RB_WAITFD_OUT));
rb_define_const(rb_cObject, "RB_WAITFD_PRI", INT2NUM(RB_WAITFD_PRI));

• rb_define_const(rb_cObject, "INT_MAX", INT2NUM(INT_MAX));
  rb_define_singleton_method(rb_cIO, "wait_for_single_fd",
  wait_for_single_fd, 3);

Strongly disagree. Any language change should be passed matz review.

Huh? ext/-test-/* is only loaded during tests and never installed.
No users see anything in ext/-test-/*

1. use ppoll(2) if available. and use INT_MAX if unavailable. or

2. fallback select(2)

3. is safe because linux has ppol(2).

OK, good point about ppoll(), I forgot that exists. I'll work on that
later or tomorrow.

 if (result > 0) {

•   /* remain compatible with select(2)-based implementation */
  

•   /*
  

•    * Remain compatible with the select(2)-based implementation:
  

•    * 1) mask out poll()-only revents such as POLLHUP/POLLERR
  

•    * 2) In case revents only consists of masked-out events, return all
  

•    *    requested events in the result and force the caller to make an
  

•    *    extra syscall (e.g. read/write/send/recv) to get the error.
  

•    */
    result = (int)(fds.revents & fds.events);
    return result == 0 ? events : result;
  

  }

I don't understand this. Why does this behavior help to compatible?
When do we use it?

We need to ensure rb_wait_for_single_fd(fd, events, timeval) returns
only a subset of its +events+ argument because that's all select() is
capable of.

If poll() returns POLLHUP/POLLERR, we should not expose those flags to
callers of rb_wait_for_single_fd() since it would then behave
differently if poll() or select() were used.

 int events = RB_WAITFD_IN | RB_WAITFD_OUT;
 int revents = rb_wait_for_single_fd(fd, events, NULL);
 /* poll() itself may return POLLERR, but we prevent it from being in
  * revents since select() can't return that */
 if (revents & RB_WAITFD_IN) {
/* since we don't know POLLERR, we fall back to fail here */
if (read(fd, ...) < 0)
    rb_sys_fail(0);
 }
 if (revents & RB_WAITFD_OUT) {
/* since we don't know POLLERR, we fall back to fail here */
if (write(fd, ...) < 0)
    rb_sys_fail(0);
 }
 /* user code shouldn't care about anything else since it only
  * requested RB_WAITFD_IN|RB_WAITFD_OUT */

--
Eric Wong

[kosaki (Motohiro KOSAKI)]

Hi

2011/5/4 Eric Wong [e-mail]:

KOSAKI Motohiro [e-mail] wrote:

diff --git a/ext/-test-/wait_for_single_fd/wait_for_single_fd.c
b/ext/-test-/wait_for_single_fd/wait_for_single_fd.c
index d406724..6efd1af 100644
--- a/ext/-test-/wait_for_single_fd/wait_for_single_fd.c
+++ b/ext/-test-/wait_for_single_fd/wait_for_single_fd.c
@@ -25,6 +25,7 @@ Init_wait_for_single_fd(void)
     rb_define_const(rb_cObject, "RB_WAITFD_IN", INT2NUM(RB_WAITFD_IN));
     rb_define_const(rb_cObject, "RB_WAITFD_OUT", INT2NUM(RB_WAITFD_OUT));
     rb_define_const(rb_cObject, "RB_WAITFD_PRI", INT2NUM(RB_WAITFD_PRI));

•    rb_define_const(rb_cObject, "INT_MAX", INT2NUM(INT_MAX));
       rb_define_singleton_method(rb_cIO, "wait_for_single_fd",
                                  wait_for_single_fd, 3);

Strongly disagree. Any language change should be passed matz review.

Huh?  ext/-test-/* is only loaded during tests and never installed.
No users see anything in ext/-test-/*

Oops, my bad. I misunderstood your diff. ok, I'll commit it.

1. use ppoll(2) if available. and use INT_MAX if unavailable. or

2. fallback select(2)

3. is safe because linux has ppol(2).

OK, good point about ppoll(), I forgot that exists.  I'll work on that
later or tomorrow.

ok.

     if (result > 0) {

•       /* remain compatible with select(2)-based implementation */

•       /*
•        * Remain compatible with the select(2)-based implementation:
•        * 1) mask out poll()-only revents such as POLLHUP/POLLERR
•        * 2) In case revents only consists of masked-out events, return all
•        *    requested events in the result and force the caller to make an
•        *    extra syscall (e.g. read/write/send/recv) to get the error.
•        */
          result

[normalperson (Eric Wong)]

• File 0001-rb_wait_for_single_fd-use-ppoll-instead-of-poll.patch 0001-rb_wait_for_single_fd-use-ppoll-instead-of-poll.patch added

I've uploaded a new patch to use ppoll() instead of poll()

Also pullable from git://bogomips.org/ruby rb_wait_for_single_fd-ppoll

[normalperson (Eric Wong)]

• File 0001-add-io-wait-benchmarks.patch 0001-add-io-wait-benchmarks.patch added

also adding benchmarks for IO#wait since those are improved by poll/ppoll

[normalperson (Eric Wong)]

Eric Wong [e-mail] wrote:

Instead the io/wait extension should be expanded to be able to check for
writability, too. Maybe by adding a new method:

IO#wait_writable(timeout = 0)

Should've been:

IO#wait_writable(timeout = nil)

actually. I created a separate issue/patch for it:

http://redmine.ruby-lang.org/issues/4647

--
Eric Wong

[kosaki (Motohiro KOSAKI)]

• Status changed from Open to Closed

[kosaki (Motohiro KOSAKI)]

Eric Wong [e-mail] wrote:

Instead the io/wait extension should be expanded to be able to check for
writability, too.  Maybe by adding a new method:

      IO#wait_writable(timeout

[kosaki (Motohiro KOSAKI)]

Issue #4531 has been updated by Eric Wong.

File 0001-rb_wait_for_single_fd-use-ppoll-instead-of-poll.patch added

I've uploaded a new patch to use ppoll() instead of poll()

Also pullable from git://bogomips.org/ruby rb_wait_for_single_fd-ppoll

I've commited modified patch based on it at r31450.