Skip to content
This repository has been archived by the owner on Dec 7, 2018. It is now read-only.

Nonblock without exception #166

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

HoneyryderChuck
Copy link

Related to #139 .

Should boost performance, as exception handling is a quite expensive VM operation.

Left support for ruby 2.0.0, although the ball is on someone else to drop it.

Tiago Cardoso added 3 commits January 8, 2016 18:24
…one doesn't need to handle Wait* Exception (Closes celluloid#139)
…to nonblock_without_exception

Conflicts:
	lib/celluloid/io/stream.rb
	spec/celluloid/io/actor_spec.rb
	spec/celluloid/io/dns_resolver_spec.rb
	spec/celluloid/io/mailbox_spec.rb
	spec/celluloid/io/ssl_server_spec.rb
	spec/celluloid/io/ssl_socket_spec.rb
	spec/celluloid/io/tcp_server_spec.rb
	spec/celluloid/io/tcp_socket_spec.rb
	spec/celluloid/io/udp_socket_spec.rb
	spec/celluloid/io/unix_server_spec.rb
	spec/celluloid/io/unix_socket_spec.rb
	spec/celluloid/io_spec.rb
@digitalextremist
Copy link
Member

@TiagoCardoso1983 are there benchmarks available? I'm curious to see some

… and using it to encapsulate nonblock reads and writes inside the streams
@HoneyryderChuck
Copy link
Author

@digitalextremist not for celluloid-io, but maybe @tarcieri has some, as the project README claims some numbers comparing it to eventmachine and node.js.

@HoneyryderChuck
Copy link
Author

And then I looked at the benchmarks directory.... :)

with ruby 2.1.6

# master
Calculating -------------------------------------
               spawn       154 i/100ms
               calls       415 i/100ms
         async calls       165 i/100ms
-------------------------------------------------
               spawn     1607.1 (±4.4%) i/s -       8162 in   5.088789s
               calls     4123.4 (±14.7%) i/s -      20335 in   5.027527s
         async calls     8537.5 (±17.2%) i/s -      41415 in   5.012115s

#nonblock without exceptions
Calculating -------------------------------------                       
               spawn       155 i/100ms                                  
               calls       446 i/100ms                                  
         async calls       173 i/100ms                                  
-------------------------------------------------                       
               spawn     1650.4 (±7.1%) i/s -       8215 in   5.004736s 
               calls     4059.4 (±9.4%) i/s -      20516 in   5.094898s 
         async calls     8781.6 (±14.8%) i/s -      42904 in   5.001161s

There is improvement, although I was expecting more.

@HoneyryderChuck
Copy link
Author

Then I looked at the benchmark file, and it's not doing any IO. Maybe we better wait for @tarcieri to answer.

@tarcieri
Copy link
Member

tarcieri commented Jan 8, 2016

I think the performance implications are going to vary widely depending on which Ruby VM you're on. Exceptional async I/O used to create a new exception object and mix IO::WaitReadable/WaitWritable into the instance on MRI 1.9.3/2.0, which busts the method cache. This was fixed in later versions, but these are also the versions that support this feature.

I don't have hard numbers.

@HoneyryderChuck
Copy link
Author

actually I was wrong, celluloid-io doesn't have any benchmarks, reel does. @digitalextremist , it's your house, right?

@digitalextremist
Copy link
Member

The Reel benchmarks are outdated badly if not irrelevant, unfortunately 👎

@ioquatix
Copy link
Contributor

ioquatix commented Jan 8, 2016

The traditional expectation that exception handling is expensive is true in many compiled languages since everything else is really fast, but in Ruby that may not hold true.

@HoneyryderChuck
Copy link
Author

it is true, and by the reasons stated before. check performance difference between raise/resque and throw/catch, for example. This can be indeed a huge performance boost for IO scenarios.

@tarcieri
Copy link
Member

tarcieri commented Jan 9, 2016

@ioquatix the situation was actually a lot more dire than that in MRI for quite awhile, as the exceptions generated for EAGAIN events in the async I/O APIs mixed a module into the exception instance. In addition to creating a new metaclass each time an exception is thrown (allocating even more memory and increasing GC pressure that much more) this also busts the method cache, and in the versions of MRI that did this MRI only has a single global method cache. This results in the entire application running much slower because the class hierarchy changes each time one of these exceptions is thrown. It's pretty much the same thing I was describing in this blog post:

https://tonyarcieri.com/dci-in-ruby-is-completely-broken

I believe in 2.2 they added special exception classes (IO::EAGAINWaitReadable and IO::EAGAINWaitWritable) which already have IO::WaitReadable and IO::WaitWritable mixed in so that no longer happens (this is exactly what JRuby and Rubinius were already doing). Using exception: false at least avoids allocating objects for the exception and collecting the stack trace (as many more objects).

@ioquatix
Copy link
Contributor

ioquatix commented Jan 9, 2016

@tarcieri sounds like the whole thing is over engineered up the wazoo, your blog was informative :)

@HoneyryderChuck
Copy link
Author

When handled in UDPSocket class, will probably solve #156

@HoneyryderChuck
Copy link
Author

Just checked the documentation, it seems that the same usage of exceptions doesn't apply there...

@HoneyryderChuck
Copy link
Author

Wrong again, apparently support was added in ruby 2.3.0: http://docs.ruby-lang.org/en/2.3.0/UDPSocket.html

@HoneyryderChuck
Copy link
Author

is this good to merge? or is the build an artifact coming from celluloid behaving bad?

@tarcieri
Copy link
Member

I don't think so. It doesn't look complete. Nowhere are you actually passing :exception => false to a read_nonblock or write_nonblock call...

…ket, as the arguments are directly passed and one should leverage it himself
@HoneyryderChuck
Copy link
Author

ups, completely overlooked.

…rm dns queries (this one is only compatible with ruby 2.3.0)
@@ -42,7 +42,10 @@ def resolve(hostname)

query = build_query(hostname)
@socket.send query.encode, 0, @server.to_s, DNS_PORT
data, _ = @socket.recvfrom(MAX_PACKET_SIZE)
data, _ = RUBY_VERSION >= "2.3" ?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, this definitely seems like the wrong place to put this check. In the DNS resolver? It should be using a Celluloid::IO::UDPSocket, and these details should be pushed down there.

@HoneyryderChuck
Copy link
Author

better?

@@ -80,6 +71,20 @@ def syswrite(string)
total_written
end

# TODO: remove after ending ruby 2.0.0 support
if RUBY_VERSION >= "2.1"
def read_nonblock(*args, **options)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the semantics of this method on Ruby >= "2.1" making it exceptionless by default and unilaterally (i.e. even if exception: true were passed)

I'd rather we preserve Ruby core semantics as much as possible.

How about an internal __read_nonblock method which is always exceptional on Ruby < 2.1 and always exceptionless on 2.1+?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll get back at this tomorrow. But isn't that what this method is doing already? No redefinition is happening under ruby < 2.1, it just delegates to the socket.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are redefining it under Ruby 2.1+ such that:

  • The exception: ... parameter is completely ignored
  • The default is switched from exceptional to exceptionless

@HoneyryderChuck
Copy link
Author

better? It seems that the default for ruby 2.1+ is still exceptionless. Isn't it the purpose? Because I don't see otherwise the condition to be used. Should I test if I'm inside an actor and then switch to exceptionless mode?

@tarcieri
Copy link
Member

FWIW, we've received reports of problems in http.rb's timeout implementation which this PR is copypasta'd from, so I think there's probably some more work needed here:

httprb/http#298 (comment)

@HoneyryderChuck
Copy link
Author

@tarcieri are you sure that it's about the exception handling? The problem in http may arise also from the io/wait API usage in favour of IO.select, as one favours poll() syscall over select(), and maybe the interval handling is different (?). possible guess.

@tarcieri
Copy link
Member

@TiagoCardoso1983 we haven't figured out the exact cause yet

@HoneyryderChuck
Copy link
Author

@tarcieri I just checked you reverted to using IO.select in http.rb . So, what is the gist? Unusable with timeouts? Funky with fd's other than network sockets? I had complaints on another project where using io/wait calls for IO.pipe fds has a different behaviour than using them with IO.select.

@tarcieri
Copy link
Member

@TiagoCardoso1983 see this issue:

httprb/http#298

Switching from IO.select to io/wait has an indeterminate difference with respect to timeout behavior. They are not semantically equivalent.

I've been meaning to dig deeper into this issue but haven't yet. The result was a complete breakage of timeout handling.

@HoneyryderChuck
Copy link
Author

I see. See net-ssh/net-ssh#303 (comment) for what I meant with API incompatibility. As I was investigating, io/wait uses poll API in Linux and falls back to select in MacOS and Windows, but I suspect that the calls semantics are different. A bit unfortunate IMO.

@tarcieri
Copy link
Member

I'd like to pull an strace-style syscall profile against the two APIs to see what actually differs. Just haven't had the time yet.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants