Project

General

Profile

Actions

Feature #6154

closed

Eliminate extending WaitReadable/Writable at runtime

Added by headius (Charles Nutter) about 12 years ago. Updated almost 11 years ago.

Status:
Closed
Target version:
[ruby-core:43325]

Description

The nonblocking IO operations started extending WaitReadable or WaitWritable into the Errno::EAGAIN instance some time during the 1.9 series. This has a rather high cost, since a singleton class must be created and the global method cache must be flushed.

The attached patch instead creates two new classes of the following form, and raises them rather than raising a singleton EAGAIN:

class IO::EAGAINReadable < Errno::EAGAIN
include WaitReadable
end

class IO::EAGAINWritable < Errno::EAGAIN
include WaitWritable
end

The performance of repeatedly doing unsuccessful nonblocking reads improves by about 20%:

BEFORE:

system ~/projects/ruby $ ./ruby2.0.0 -rbenchmark -rsocket -e "sock = TCPSocket.new('localhost', 22); 10.times { puts Benchmark.measure { 100_000.times { begin; sock.read_nonblock(10); rescue IO::WaitReadable; end } } }"
1.210000 0.110000 1.320000 ( 1.328921)
1.220000 0.120000 1.340000 ( 1.326136)
1.220000 0.110000 1.330000 ( 1.334026)
1.230000 0.110000 1.340000 ( 1.349927)
1.310000 0.130000 1.440000 ( 1.426608)
1.210000 0.110000 1.320000 ( 1.333530)
1.220000 0.120000 1.340000 ( 1.330352)
1.230000 0.110000 1.340000 ( 1.350455)
1.220000 0.120000 1.340000 ( 1.327550)
1.220000 0.110000 1.330000 ( 1.337785)

AFTER:

system ~/projects/ruby $ ./ruby2.0.0 -rbenchmark -rsocket -e "sock = TCPSocket.new('localhost', 22); 10.times { puts Benchmark.measure { 100_000.times { begin; sock.read_nonblock(10); rescue IO::WaitReadable; end } } }"
0.980000 0.110000 1.090000 ( 1.092166)
1.010000 0.120000 1.130000 ( 1.129877)
1.090000 0.120000 1.210000 ( 1.202066)
0.960000 0.110000 1.070000 ( 1.076274)
0.970000 0.100000 1.070000 ( 1.078000)
0.970000 0.110000 1.080000 ( 1.078156)
0.970000 0.110000 1.080000 ( 1.078005)
0.970000 0.110000 1.080000 ( 1.078266)
0.980000 0.110000 1.090000 ( 1.093039)
1.000000 0.110000 1.110000 ( 1.112519)

This benchmark does not show the hidden cost of constantly invalidating the global method cache.

I also modified a similar case in OpenSSL, where it previously created an SSLError and extended WaitReadable into it.


Files

eagain_readwrite.diff (5.73 KB) eagain_readwrite.diff headius (Charles Nutter), 03/16/2012 01:02 PM
eagain_readwrite.diff (6.74 KB) eagain_readwrite.diff headius (Charles Nutter), 03/23/2012 03:38 AM
eagain_readwrite.diff (10.3 KB) eagain_readwrite.diff headius (Charles Nutter), 04/03/2013 07:20 AM
eagain_readwrite.diff (10.5 KB) eagain_readwrite.diff headius (Charles Nutter), 04/03/2013 10:55 AM

Updated by headius (Charles Nutter) about 12 years ago

I should have mentioned that JRuby has been running this way for a while (on master) and we have had no reports of incompatibility. The concrete subclass is_a EAGAIN and is_a WaitReadable, so all typical exception-handling patterns work correctly. The only cases that break are cases that check e.class == EAGAIN, which is probably not a good pattern anyway.

Updated by normalperson (Eric Wong) about 12 years ago

Charles Nutter wrote:

I should have mentioned that JRuby has been running this way for a
while (on master) and we have had no reports of incompatibility. The
concrete subclass is_a EAGAIN and is_a WaitReadable, so all typical
exception-handling patterns work correctly.

Good to know. I like this change[1]

The only cases that break are cases that check e.class == EAGAIN,
which is probably not a good pattern anyway.

I also noticed some test failures with your patch. (But I think
the minor incompatibility is acceptable for 2.0.0)

test_read_nonblock(OpenSSL::TestPair) [$top_srcdir/test/openssl/test_pair.rb:145]:
[OpenSSL::SSL::SSLError] exception expected, not
Class: OpenSSL::SSL::SSLErrorReaable
Message: <"read would block">
---Backtrace---
$top_srcdir/trunk/.ext/common/openssl/buffering.rb:174:in sysread_nonblock' $top_srcdir/trunk/.ext/common/openssl/buffering.rb:174:in read_nonblock'
$top_srcdir/test/openssl/test_pair.rb:147:in `block (2 levels) in test_read_nonblock'

test_dgram_pair(TestSocket_UNIXSocket) [$top_srcdir/test/socket/test_unix.rb:348]:
[Errno::EAGAIN] exception expected, not
Class: IO::EAGAINReadable
Message: <"Resource temporarily unavailable - recvfrom(2) would block">
---Backtrace---
$top_srcdir/test/socket/test_unix.rb:348:in recv_nonblock' $top_srcdir/test/socket/test_unix.rb:348:in block in test_dgram_pair'

[1] I'd like an exception-avoiding API more, though :)

Updated by headius (Charles Nutter) about 12 years ago

On Fri, Mar 16, 2012 at 7:24 PM, Eric Wong wrote:

The only cases that break are cases that check e.class == EAGAIN,
which is probably not a good pattern anyway.

I also noticed some test failures with your patch.  (But I think
the minor incompatibility is acceptable for 2.0.0)

test_read_nonblock(OpenSSL::TestPair) [$top_srcdir/test/openssl/test_pair.rb:145]:
[OpenSSL::SSL::SSLError] exception expected, not
Class: OpenSSL::SSL::SSLErrorReaable
...
test_dgram_pair(TestSocket_UNIXSocket) [$top_srcdir/test/socket/test_unix.rb:348]:
[Errno::EAGAIN] exception expected, not
Class: IO::EAGAINReadable

Yes, these are expected failures in my eyes, and not indicative of
typical exception-handling or nonblocking-IO use cases.

And I agree 100% about adding an exception-free nonblocking API. This
plasters over the problems 1.9.x introduced, but doesn't solve the
root cause.

  • Charlie

Updated by headius (Charles Nutter) about 12 years ago

I thought about that...it would still be better to add EWOULDBLOCKReadable,
etc, than to pay the .extend toll every time.
On Mar 17, 2012 10:28 AM, "Tanaka Akira" wrote:

2012/3/16 Charles Nutter :

The nonblocking IO operations started extending WaitReadable or
WaitWritable into the Errno::EAGAIN instance some time during the 1.9
series. This has a rather high cost, since a singleton class must be
created and the global method cache must be flushed.

The attached patch instead creates two new classes of the following
form, and raises them rather than raising a singleton EAGAIN:

EWOULDBLOCK is a different from EAGAIN on some platforms.
(HP-UX, for example.)

I think your patch breaks applications which rescue only EWOULDBLOCK,
on such platforms.

Tanaka Akira

Updated by nahi (Hiroshi Nakamura) about 12 years ago

Charles, do you create a patch to introduce 4 constants (+2 for
EWOULDBLOCK) ? We discussed this issue today and we concluded that
that's better than anonymous class caching (to avoid Marshalling
issue.)

I think I can take care of openssl change.

Updated by headius (Charles Nutter) about 12 years ago

Updated patch with the following changes:

  • rb_eEWOULDBLOCKReadable and rb_eEWOULDBLOCKWritable added
  • EWOULDBLOCK versions are set to EAGAIN versions if EAGAIN == EWOULDBLOCK
  • renamed function rb_readwrite_sys_fail to match error.c names better
  • reverted OSSL changes so nahi can do them right
  • fixed the one test (test/socket/test_unix.rb) that expected an exact exception

Notes:

  • assert_raise should probably use === instead of == to better match rescue logic
  • more tests for this logic are needed(!!!)
  • I have not tested RubySpec yet
  • I can't test on HP/UX or other systems where EAGAIN != EWOULDBLOCK
Actions #8

Updated by mame (Yusuke Endoh) almost 12 years ago

  • Tracker changed from Bug to Feature
  • Status changed from Open to Assigned
  • Assignee set to matz (Yukihiro Matsumoto)

I think it is suitable to handle this issue in the feature tracker.

--
Yusuke Endoh

Updated by headius (Charles Nutter) over 11 years ago

Seven months and no activity. This is not a breaking change and it could improve performance of nonblocking IO operations a lot. Any reason not to incorporate it?

Updated by headius (Charles Nutter) over 11 years ago

I have made the additional OpenSSL changes in the following JRuby commit: https://github.com/jruby/jruby/commit/8b022c896ea0d76f876e458229f4f150893295b9

The new exceptions are SSLErrorReadable and SSLErrorWritable. I am not married to these names, but unfortunately I do not wish to continue perpetuating the performance impact of extending these exceptions every time.

The current behavior of MRI to extend WaitReadable and WaitWritable every time is a bug. It needs to be fixed.

Updated by mame (Yusuke Endoh) over 11 years ago

  • Target version set to 2.6

I postpone this ticket to next minor. Very sorry.
If Eric Wong were a committer, I would leave this to him.

--
Yusuke Endoh

Updated by normalperson (Eric Wong) over 11 years ago

"mame (Yusuke Endoh)" wrote:

I postpone this ticket to next minor. Very sorry.
If Eric Wong were a committer, I would leave this to him.

Looks like Charles Nutter will be a committer soon and can handle this.

Updated by headius (Charles Nutter) about 11 years ago

mame or matz: Is this change approved? If so, I can proceed with a patch that includes what I've already posted plus OpenSSL changes.

Updated by drbrain (Eric Hodel) about 11 years ago

Since a committer nominee was given permission to commit I think you should commit it

Updated by headius (Charles Nutter) almost 11 years ago

Working on this today. I will post an updated patch for review before committing.

Updated by headius (Charles Nutter) almost 11 years ago

  • Category set to core
  • Assignee changed from matz (Yukihiro Matsumoto) to headius (Charles Nutter)
  • % Done changed from 0 to 70

Updated by headius (Charles Nutter) almost 11 years ago

Ok, here's the updated patch. Here's a list of changes from the last time I updated:

  • Names have been changed to EAGAINWaitReadable, etc, from EAGAINReadable. It seemed clearer to indicate the full name of the module, especially in the SSL case where it would have been SSLErrorReadable. Weird.

  • OpenSSL has been patched to have SSLErrorWaitReadable and SSLErrorWaitWritable. Note that I am not married to any of these names, if there's better suggestions.

  • Socket has been patched to use rb_readwrite_sys_fail for EINPROGRESS + WaitWritable. I added EINPROGRESSWaitWritable and EINPROGRESSWaitReadable because it uses rb_readwrite_sys_fail and I didn't think that method should only handle the writable version

  • rb_readwrite_sys_fail has been expanded to include EINPROGRESS versions of WaitReadable and WaitWritable. Note this is added as a public API, to give users a path away from rb_mod_sys_fail*.

  • Logic that switches based on ERRNO modified to use switch/case

  • Tests that checked for exact error types of EAGAIN have been modified to check for the WaitReadable/Writable subclasses.

make test-all passes. Please review and comment.

Note that I'd also like to lobby for this change to be applied to 1.9.3, perhaps without the public API. Lots of people are going to be on 1.9.3 for a long time before moving to 2.0.x, and they'd like to have this improvement.

Updated by Anonymous almost 11 years ago

=begin
Crazy idea, but could you avoid introducing a new class by caching the iclass?

So instead of something like:

/* when exception is raised */
VALUE exc = ossl_exc_new(eSSLError, "read would block");
rb_extend_object(exc, rb_mWaitWritable);
rb_exc_raise(exc);

You might do something like this:

VALUE cached_ssl_error_wait_writable_iclass;

/* once off setup during vm boot */
VALUE exc = ossl_exc_new(eSSLError, "read would block");
rb_extend_object(exc, rb_mWaitWritable);
cached_ssl_error_wait_writable_iclass = RBASIC(exc)->klass;

/* when exception is raised */
VALUE exc = ossl_exc_new(eSSLError, "read would block");
RBASIC(exc)->klass = cached_ssl_error_wait_writable_iclass;
rb_exc_raise(exc);

The benefit of this is that you don't have to change the behaviour that's visible from Ruby land.
=end

Updated by headius (Charles Nutter) almost 11 years ago

charliesome: Yeah, I was thinking about that as I created this patch.

It's possible, but it introduces a rather strange oddity: you'll have multiple exceptions floating around that look like singletons but are actually the same singleton class. If anyone adds methods to them it will add methods to all of them, but there's no way to know you're doing it. I believe this would mean you can't create proper isolated singleton instances/classes of these exception objects.

Are there other places in MRI where this pattern is used? If it's not being done elsewhere, I'd be reluctant to do it here.

The only behavior you have to change from Ruby land would be not testing for EAGAIN == exception. EAGAIN#===, WaitReadable#===, kind_of? and so on work just as they did before. I had to change the tests only because assert_raise does an == check internally (and it arguably should do === or kind_of? since that's closer to what rescue does).

Updated by headius (Charles Nutter) almost 11 years ago

And by EAGAIN == exception I meant EAGAIN == exception.class, of course.

Updated by normalperson (Eric Wong) almost 11 years ago

"headius (Charles Nutter)" wrote:

make test-all passes. Please review and comment.

--- io.c (revision 40073)
+++ io.c (working copy)
@@ -133,6 +133,15 @@

+VALUE rb_eEAGAINWaitReadable;
+VALUE rb_eEAGAINWaitWritable;
+VALUE rb_eEWOULDBLOCKWaitReadable;
+VALUE rb_eEWOULDBLOCKWaitWritable;
+VALUE rb_eEINPROGRESSWaitWritable;
+VALUE rb_eEINPROGRESSWaitReadable;

New globals introduced don't appear to be used outside of io.c,
so they can be made static (unless you intend to reference them
outside of io.c in the future)

--- ext/openssl/ossl_ssl.c (revision 40073)
+++ ext/openssl/ossl_ssl.c (working copy)
@@ -26,6 +26,8 @@

VALUE mSSL;
VALUE eSSLError;
+VALUE eSSLErrorWaitReadable;
+VALUE eSSLErrorWaitWritable;
VALUE cSSLContext;
VALUE cSSLSocket;

Likewise for these globals.

Otherwise, your patch looks good. Thanks!

Updated by headius (Charles Nutter) almost 11 years ago

normalperson (Eric Wong) wrote:

+VALUE rb_eEAGAINWaitReadable;
+VALUE rb_eEAGAINWaitWritable;
+VALUE rb_eEWOULDBLOCKWaitReadable;
+VALUE rb_eEWOULDBLOCKWaitWritable;
+VALUE rb_eEINPROGRESSWaitWritable;
+VALUE rb_eEINPROGRESSWaitReadable;

New globals introduced don't appear to be used outside of io.c,
so they can be made static (unless you intend to reference them
outside of io.c in the future)

Good call, will fix.

Updated by headius (Charles Nutter) almost 11 years ago

Updated patch with new exception variables made static.

Updated by headius (Charles Nutter) almost 11 years ago

I am prepared to commit this fix if there's no other commentary.

A quick benchmark to show it in action: https://gist.github.com/headius/5315475

$ ./ruby -I lib:ext:. nonblock_bench.rb 
  0.940000   0.170000   1.110000 (  1.113261)
  0.940000   0.170000   1.110000 (  1.104994)
  0.940000   0.160000   1.100000 (  1.108880)
  0.950000   0.170000   1.120000 (  1.107059)
  0.970000   0.170000   1.140000 (  1.148363)

$ ./ruby -I lib:ext:.:ext/socket nonblock_bench.rb 
  0.660000   0.140000   0.800000 (  0.808189)
  0.680000   0.150000   0.830000 (  0.814997)
  0.670000   0.140000   0.810000 (  0.811901)
  0.670000   0.140000   0.810000 (  0.804447)
  0.670000   0.140000   0.810000 (  0.817231)
Actions #25

Updated by headius (Charles Nutter) almost 11 years ago

  • Status changed from Assigned to Closed
  • % Done changed from 70 to 100

This issue was solved with changeset r40195.
Charles, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


Fix #6154 by introducing new EAGAIN/EWOULDBLOCK/EINPROGRESS
subclasses that include WaitReadable or WaitWritable rather than
extending them into the exception object each time.

  • error.c: Capture EGAIN, EWOULDBLOCK, EINPROGRESS exceptions and
    export them for use in WaitReadable/Writable exceptions.
  • io.c: Create versions of EAGAIN, EWOULDBLOCK, EINPROGRESS that
    include WaitReadable and WaitWritable. Add rb_readwrite_sys_fail
    for nonblocking failures using those exceptions. Use that
    function in io_getpartial and io_write_nonblock instead of
    rb_mod_sys_fail
  • ext/openssl/ossl_ssl.c: Add new SSLError subclasses that include
    WaitReadable and WaitWritable. Use those classes for
    write_would_block and read_would_block instead of rb_mod_sys_fail.
  • ext/socket/ancdata.c: Use rb_readwrite_sys_fail instead of
    rb_mod_sys_fail in bsock_sendmsg_internal and
    bsock_recvmsg_internal.
  • ext/socket/init.c: Use rb_readwrite_sys_fail instead of
    rb_mod_sys_fail in rsock_s_recvfrom_nonblock and
    rsock_s_connect_nonblock.
  • ext/socket/socket.c: Use rb_readwrite_sys_fail instead of
    rb_mod_sys_fail in sock_connect_nonblock.
  • include/ruby/ruby.h: Export rb_readwrite_sys_fail for use instead
    of rb_mod_sys_fail. Introduce new constants RB_IO_WAIT_READABLE and
    RB_IO_WAIT_WRITABLE for first arg to rb_readwrite_sys_fail.
Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0