Code Review: BlockDispatch7

Ruby IronRuby
1

tfpt review “/shelveset:BlockDispatch7;REDMOND\tomat”

Improves dispatch to blocks.

Previously, DynamicSites were used to adapt call site’s arguments to the
signature of the target block. In block invocation no resolution needs
to be performed, which makes it different from method invocation. The
dynamic behavior is only in the arguments to parameters transformation.
In usual case it is straightforward though. Only if
splatting/unsplatting is used (and in some other special cases) there
are various checks that need to be performed to shuffle the arguments
right. Although dynamic sites could help here in some cases (by caching
by a shape of the target block signatures) the usual cases are rather
slowed down by the overhead. In an optimal case w/o any
splatting/unsplatting and without polymorphic sites kicking in at least
one comparison and 2 delegate calls needs to be done.

This change replaces dynamic site dispatch by a virtual dispatch
optimized for 0-4 parameters. Each block is associated with a block
dispatcher (a subclass of BlockDispatcher abstract class, previously
RubyBlockInfo) that corresponds to its signature. The specialized
dispatchers implement virtual Invoke methods for 0…4 and N parameters
w/ and w/o splatting. The call site uses one of those Invoke methods
(based on its arguments) and calls it. The dispatcher holds on a
delegate that points to the block method. The delegate is called by
Invoke methods with transformed arguments. In the optimal case (e.g.
1.times {|x| puts x}) the cost of block yield is a virtual method
dispatch and a delegate call. Besides no runtime-generated stubs are
needed which improves startup time. Using the dispatchers also enables
to move some previously generated code into RubyOps and therefore
decreases the amount of generated code even more.

Blocks are still IDOs to provide good interop. This change also made the
rules much simpler.

TODO:

There is some work to be done to optimize some paths thru dispatchers.
Will need to run some micro-benchmarks for block dispatch to see where
we should do better.
Also, some parts of the code seem to be good candidates for source code
generation, but I haven’t opted for that for now since it was easier to
write it by hand (there are many exemptions to the “rules” of the block
dispatch, so even if the code looks like it could be generated at the
first glance the generator would actually get more complicated to handle
all such cases). I’ve let this in TODO bucket.

Tomas

2

There doesn’t generally appear to be any consistency about comparing the
block parameter to null. For instance, in RangeOps, only StepFixnum
checks; StepString, StepNumeric and StepObject do not. I know that the
situation predates the current changeset, but it might be a good time to
go through and fix it.

The test for a non-empty range in RangeOps.StepFixnum does not correctly
handle ExcludeEnd.

In method Enumerable.Find, there’s now an extraneous assignment of
“item” to “result”.

In Proc.cs, there are a bunch of newly-added Call methods inside an #if
UNUSED. Are these for future use?

The rethrow inside ThreadOps.CreateThread will take down the process.
Is that what’s desired?

I wouldn’t swear to understanding all of the changes to the compiler,
but things otherwise look good to me. I particularly like the new
pattern for Block.Yield; the “out” parameter will make it much harder to
forget to check for a jump.

3

StepString, StepNumeric, StepFixnum check for the block in the loop.
Note that the exception is not thrown if no item is visited by the
enumerator. But you’re right there are methods that handle the block
incorrectly. We need to write some tests that pass nil to all library
methods that use blocks to cover all cases.

The assignment “result = item” in Enumerable.Find isn’t in fact
redundant. It assigns to the closed-over variable “result”.
“return RuntimeFlowControl.BlockBreak(selfBlock, item);” doesn’t return
from the Find method, it returns from the lambda defined within the
method :slight_smile:

Yes, I’ll revisit the Call methods on the next pass.

I’ve added rethrow to the Thread.CreateThread since it is imo better to
kill the process rather than silently swallow the exception (I hit this
when something wrong happened in the thread and I didn’t know what
because the exception has been swallowed).

Tomas

4

In principle I agree with you, but killing the process on an unhandled
thread exception is an incompatibility with MRI:

irb(main):001:0> Thread.start { raise ‘foo’ }
=> #<Thread:0x38abfe4 dead>
irb(main):002:0>