Feature #6154: Eliminate extending WaitReadable/Writable at runtime

• Status: Closed
• Assignee: headius (Charles Nutter)
• Target version: 2.6

[headius (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:

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.

[headius (Charles Nutter)]

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.

[normalperson (Eric Wong)]

Charles Nutter [e-mail] 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 :)

[headius (Charles Nutter)]

On Fri, Mar 16, 2012 at 7:24 PM, Eric Wong [e-mail] 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

[headius (Charles Nutter)]

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" [e-mail] wrote:

2012/3/16 Charles Nutter [e-mail]:

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

[nahi (Hiroshi Nakamura)]

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.

[headius (Charles Nutter)]

Ok, will do.

[headius (Charles Nutter)]

• File eagain_readwrite.diff eagain_readwrite.diff added

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

[mame (Yusuke Endoh)]

• 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 [e-mail]

[headius (Charles Nutter)]

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?

[headius (Charles Nutter)]

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.

[mame (Yusuke Endoh)]

• 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 [e-mail]

[normalperson (Eric Wong)]

"mame (Yusuke Endoh)" [e-mail] 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.

[headius (Charles Nutter)]

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.

[drbrain (Eric Hodel)]

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

[headius (Charles Nutter)]

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

[headius (Charles Nutter)]

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

[headius (Charles Nutter)]

• File eagain_readwrite.diff eagain_readwrite.diff added

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.

=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

[headius (Charles Nutter)]

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).

[headius (Charles Nutter)]

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

[normalperson (Eric Wong)]

"headius (Charles Nutter)" [e-mail] 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!

[headius (Charles Nutter)]

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.

[headius (Charles Nutter)]

• File eagain_readwrite.diff eagain_readwrite.diff added

Updated patch with new exception variables made static.

[headius (Charles Nutter)]

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)

[headius (Charles Nutter)]

• 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.