Monitor#interests could be nil

[ioquatix]

I'd like a way for a monitor to continue to exist, but for it's current interests to be nil. In the context of an async reactor, I think this makes sense - sometimes a task is waiting on IO (monitor lazily initialised) and sometimes other things (e.g. sleep). In the case of sleep, I don't want to be woken up because some IO event happened on a previously monitored task.

One option is to set/clear value, but this feels a bit like a hack. It would be nice to simply say @monitor.intersts = nil

Does this make sense?

[ioquatix]

Here is what the current code path looks like:

		def monitor(interests)
			unless @monitor
				@monitor = @context.register(@io, interests)
			else
				@monitor.interests = interests
			end
				
			@monitor.value = Fiber.current
			
			yield
			
		ensure
			@monitor.value = nil
		end

		def wait_any(interests = :rw)
			monitor(interests) do
				# Async.logger.debug "Fiber #{Fiber.current} yielding..."
				result = Fiber.yield
				
				# Async.logger.debug "Fiber #{Fiber.current} resuming with result #{result}..."
				raise result if result.is_a? Exception
			end
		end

[ioquatix]

In the reactor, if monitor.value is nil, the event is ignored. This doesn't seem particularly efficient though. But also, adding and removing IO from the event loop might also not be that efficient. I'm open for suggestions.

[matthewd]

#124 (review)

[ioquatix]

Awesome!

Just as an aside. I think using a symbol for interests is a bit, uh, inefficient, and even a bit, cumbersome. What about using integer flags?

Additionally, right now, I see rb_intern("r") and so on. Shouldn't this just be done once and stuck into an static ID at the top of the .c file or something?

[matthewd]

To be clear, that's just an endorsement of the "nil should work" theory; since that comment, I haven't made a PR to make it so, and I don't believe @tarcieri has done it directly.

[tarcieri]

Here are tests for clearing the last interest:

https://github.com/socketry/nio4r/blob/master/spec/nio/monitor_spec.rb#L74

That said, I'm not sure it's been exhaustively tested, and Java NIO tends to be full of surprises.

Just as an aside. I think using a symbol for interests is a bit, uh, inefficient, and even a bit, cumbersome. What about using integer flags?

Ick. I think the performance overhead should be minimal (especially if you memoize the symbols), and integer-driven APIs are clunky, especially in Ruby. This suggestion definitely smells of microoptimization in an area that's unlikely to be a bottleneck.

[ioquatix]

I'll come out clearly: I love micro-optimisations, and I also love semantic optimisations. I think this is both.

Symbols are heap allocated, vs small integers which are not. Yes, if we stick with this API, it should definitely be set up initially https://github.com/ioquatix/trenni/blob/9e3cd499ed0d2d4e25a513c569535e4cc3e5e41d/ext/trenni/trenni.c#L18-L41 and used later e.g. https://github.com/ioquatix/trenni/blob/9e3cd499ed0d2d4e25a513c569535e4cc3e5e41d/ext/trenni/markup.c#L171

When benchmarking C code I found that rb_intern in the hot path could make a significant performance impact.

I'm not proposing an Integer driven API, but a set of binary flags for read/write interest, probably the identical values as EV_READ and EV_WRITE so no conversion is necessary.

Using symbols means that we need to have code like

nio4r/lib/nio/monitor.rb

Lines 27 to 102 in 8ceb343

	# Replace the existing interest set with a new one
	#
	# @param interests [:r, :w, :rw] I/O readiness we're interested in (read/write/readwrite)
	#
	# @return [Symbol] new interests
	def interests=(interests)
	raise EOFError, "monitor is closed" if closed?
	raise ArgumentError, "bad interests: #{interests}" unless [:r, :w, :rw].include?(interests)
	@interests = interests
	end
	# Add new interests to the existing interest set
	#
	# @param interests [:r, :w, :rw] new I/O interests (read/write/readwrite)
	#
	# @return [self]
	def add_interest(interest)
	case interest
	when :r
	case @interests
	when :r then @interests = :r
	when :w then @interests = :rw
	when :rw then @interests = :rw
	when nil then @interests = :r
	end
	when :w
	case @interests
	when :r then @interests = :rw
	when :w then @interests = :w
	when :rw then @interests = :rw
	when nil then @interests = :w
	end
	when :rw
	@interests = :rw
	else raise ArgumentError, "bad interests: #{interest}"
	end
	end
	# Remove interests from the existing interest set
	#
	# @param interests [:r, :w, :rw] I/O interests to remove (read/write/readwrite)
	#
	# @return [self]
	def remove_interest(interest)
	case interest
	when :r
	case @interests
	when :r then @interests = nil
	when :w then @interests = :w
	when :rw then @interests = :w
	when nil then @interests = nil
	end
	when :w
	case @interests
	when :r then @interests = :r
	when :w then @interests = nil
	when :rw then @interests = :r
	when nil then @interests = nil
	end
	when :rw
	@interests = nil
	else raise ArgumentError, "bad interests: #{interest}"
	end
	end
	# Is the IO object readable?
	def readable?
	readiness == :r || readiness == :rw
	end
	# Is the IO object writable?
	def writable?
	readiness == :w || readiness == :rw
	end
	alias writeable? writable?

- which is both inefficient and ugly. That code would be simpler and more efficient if we used a bit-field integer.

My suggestion would be something like this:

Internally, interests is an integer with the following interpretation: 0 means no interest, flag EV_READ set means read interested, flag EV_WRITE set means write interested. On assignment, if the input is a symbol, perform the appropriate conversion. For #inspect have something which generates a nice output string.

I'm not trying to give anyone shit for the current design - it works and I see the appeal of it - symbols make sense in lots of situations, but in this case, it feels like an anti-pattern.

[ioquatix]

I also think that a symbol driven API could be almost as fast, provided things are memoized - and perhaps we should initially work on that - it's just that I feel it's simpler using integers.

[tarcieri]

I also think that a symbol driven API could be almost as fast, provided things are memoized - and perhaps we should initially work on that

Sounds good

it's just that I feel it's simpler using integers.

There's nothing simple about invasive, breaking API changes

[ioquatix]

There's nothing simple about invasive, breaking API changes

Sure, but it doesn't have to be invasive or breaking :)

[tarcieri]

I think it'd be pretty weird (and needlessly complicated) to support both

[ioquatix]

I don't think it would be that hard to support both.

I was thinking about this from another POV. Right now we only have two bits, r, w. So, there are only three possibilities, r, w, and rw. But, imagine if there was one or two more - because of the design, we'd have combinatorial explosion. Symbols simply weren't designed to be used this way.

[matthewd]

@tarcieri right: it's possible to clear the last interest via remove_interest... but monitor.interests = nil raises.

So you can't do tmp = monitor.interests; [stuff]; monitor.interests = tmp, for example.

I'm not sure how useful that pattern would be here, but either way, it seems odd for interests= to raise when given a value you can get from interests.

[ioquatix]

@matthewd Do you want to submit a PR to fix this? Your right it's odd.

[ioquatix]

Just for reference, this is how it currently works in async:

https://github.com/socketry/async/blob/6184c298fbc68210161e5c42ae587ce92fec6304/lib/async/wrapper.rb#L33-L46