Feature #6154

Eliminate extending WaitReadable/Writable at runtime

Added by Charles Nutter about 2 years ago. Updated about 1 year ago.

[ruby-core:43325]
Status:Closed
Priority:Normal
Assignee:Charles Nutter
Category:core
Target version:next minor

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 { 100000.times { begin; sock.readnonblock(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 { 100000.times { begin; sock.readnonblock(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.

eagain_readwrite.diff Magnifier (5.73 KB) Charles Nutter, 03/16/2012 01:02 PM

eagain_readwrite.diff Magnifier (6.74 KB) Charles Nutter, 03/23/2012 03:38 AM

eagain_readwrite.diff Magnifier (10.3 KB) Charles Nutter, 04/03/2013 07:20 AM

eagain_readwrite.diff Magnifier (10.5 KB) Charles Nutter, 04/03/2013 10:55 AM

Associated revisions

Revision 40195
Added by Charles Nutter about 1 year ago

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 rbreadwritesysfail for nonblocking failures using those exceptions. Use that function in iogetpartial and iowritenonblock instead of rbmodsys_fail
  • ext/openssl/osslssl.c: Add new SSLError subclasses that include WaitReadable and WaitWritable. Use those classes for writewouldblock and readwouldblock instead of rbmodsysfail.
  • ext/socket/ancdata.c: Use rbreadwritesysfail instead of rbmodsysfail in bsocksendmsginternal and bsockrecvmsginternal.
  • ext/socket/init.c: Use rbreadwritesysfail instead of rbmodsysfail in rsocksrecvfromnonblock and rsocksconnectnonblock.
  • ext/socket/socket.c: Use rbreadwritesysfail instead of rbmodsysfail in sockconnectnonblock.
  • include/ruby/ruby.h: Export rbreadwritesysfail for use instead of rbmodsysfail. Introduce new constants RBIOWAITREADABLE and RBIOWAITWRITABLE for first arg to rbreadwritesys_fail.

History

#1 Updated by Charles Nutter about 2 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 isa EAGAIN and isa 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.

#2 Updated by Eric Wong about 2 years ago

Charles Nutter headius@headius.com 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 isa EAGAIN and isa 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)

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


testdgrampair(TestSocketUNIXSocket) [$topsrcdir/test/socket/testunix.rb:348]:
[Errno::EAGAIN] exception expected, not
Class: IO::EAGAINReadable
Message: <"Resource temporarily unavailable - recvfrom(2) would block">
---Backtrace---
$top
srcdir/test/socket/testunix.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 :)

#3 Updated by Charles Nutter about 2 years ago

On Fri, Mar 16, 2012 at 7:24 PM, Eric Wong normalperson@yhbt.net 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)

testreadnonblock(OpenSSL::TestPair) [$topsrcdir/test/openssl/testpair.rb:145]:
[OpenSSL::SSL::SSLError] exception expected, not
Class: OpenSSL::SSL::SSLErrorReaable
...
testdgrampair(TestSocketUNIXSocket) [$topsrcdir/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

#4 Updated by Charles Nutter about 2 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" akr@fsij.org wrote:

2012/3/16 Charles Nutter headius@headius.com:

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

#5 Updated by Hiroshi Nakamura about 2 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.

#6 Updated by Charles Nutter about 2 years ago

Ok, will do.

#7 Updated by Charles Nutter about 2 years ago

Updated patch with the following changes:

  • rbeEWOULDBLOCKReadable and rbeEWOULDBLOCKWritable added
  • EWOULDBLOCK versions are set to EAGAIN versions if EAGAIN == EWOULDBLOCK
  • renamed function rbreadwritesys_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

#8 Updated by Yusuke Endoh about 2 years ago

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

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

Yusuke Endoh mame@tsg.ne.jp

#9 Updated by Charles Nutter over 1 year 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?

#10 Updated by Charles Nutter over 1 year 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.

#11 Updated by Yusuke Endoh over 1 year ago

  • Target version set to next minor

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

Yusuke Endoh mame@tsg.ne.jp

#12 Updated by Eric Wong over 1 year ago

"mame (Yusuke Endoh)" mame@tsg.ne.jp 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.

#13 Updated by Charles Nutter about 1 year 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.

#14 Updated by Eric Hodel about 1 year ago

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

#15 Updated by Charles Nutter about 1 year ago

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

#16 Updated by Charles Nutter about 1 year ago

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

#17 Updated by Charles Nutter about 1 year 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 rbreadwritesysfail for EINPROGRESS + WaitWritable. I added EINPROGRESSWaitWritable and EINPROGRESSWaitReadable because it uses rbreadwritesysfail and I didn't think that method should only handle the writable version

  • rbreadwritesysfail 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 rbmodsysfail*.

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

#18 Updated by Charlie Somerville about 1 year 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

#19 Updated by Charles Nutter about 1 year 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#===, kindof? and so on work just as they did before. I had to change the tests only because assertraise does an == check internally (and it arguably should do === or kind_of? since that's closer to what rescue does).

#20 Updated by Charles Nutter about 1 year ago

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

#21 Updated by Eric Wong about 1 year ago

"headius (Charles Nutter)" headius@headius.com wrote:

make test-all passes. Please review and comment.

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

+VALUE rbeEAGAINWaitReadable;
+VALUE rb
eEAGAINWaitWritable;
+VALUE rbeEWOULDBLOCKWaitReadable;
+VALUE rb
eEWOULDBLOCKWaitWritable;
+VALUE rbeEINPROGRESSWaitWritable;
+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/osslssl.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!

#22 Updated by Charles Nutter about 1 year ago

normalperson (Eric Wong) wrote:

+VALUE rbeEAGAINWaitReadable;
+VALUE rb
eEAGAINWaitWritable;
+VALUE rbeEWOULDBLOCKWaitReadable;
+VALUE rb
eEWOULDBLOCKWaitWritable;
+VALUE rbeEINPROGRESSWaitWritable;
+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.

#23 Updated by Charles Nutter about 1 year ago

Updated patch with new exception variables made static.

#24 Updated by Charles Nutter about 1 year 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)

#25 Updated by Charles Nutter about 1 year 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 rbreadwritesysfail for nonblocking failures using those exceptions. Use that function in iogetpartial and iowritenonblock instead of rbmodsys_fail
  • ext/openssl/osslssl.c: Add new SSLError subclasses that include WaitReadable and WaitWritable. Use those classes for writewouldblock and readwouldblock instead of rbmodsysfail.
  • ext/socket/ancdata.c: Use rbreadwritesysfail instead of rbmodsysfail in bsocksendmsginternal and bsockrecvmsginternal.
  • ext/socket/init.c: Use rbreadwritesysfail instead of rbmodsysfail in rsocksrecvfromnonblock and rsocksconnectnonblock.
  • ext/socket/socket.c: Use rbreadwritesysfail instead of rbmodsysfail in sockconnectnonblock.
  • include/ruby/ruby.h: Export rbreadwritesysfail for use instead of rbmodsysfail. Introduce new constants RBIOWAITREADABLE and RBIOWAITWRITABLE for first arg to rbreadwritesys_fail.

Also available in: Atom PDF