Don't use MULTI when not needed

[allison]

Most (perhaps all) of the current core PMCs don't really use multiple dispatch. They declare MULTIs as an expensive way of performing a simple if/else on the type of the second argument. Convert these to actual if/else or switch/case statements within a single-dispatched VTABLE entry.

Some of the converted VTABLE entries should have a default case that extracts a value from the second argument and acts on it (such as, division within a Float PMC might simply ask for the 'get_number' value of the second argument). Other converted VTABLE entries should have a default case of multiple dispatch (if the second argument isn't one of the specific anticipated types). Some converted VTABLE entries might use value extraction as a first default for core PMC types (which are relatively predictable) and multiple dispatch as the default-default to handle non-core types.

The VTABLE entries in default.pmc should still perform multiple dispatch, to allow MMD overrides from non-core types.

[chromatic]

I have my doubts about this idea; it makes PMCs more difficult to write and much more difficult to subclass. In the medium term, making MULTI and VTABLE calls less expensive will be better overall. (In the long term, making VTABLE calls multidispatch themselves will actually reduce the amount of code in the system.)

For now I'd rather see the PMC parser and code generator improved so that we can add an optimization to collapse "unnecessary" MULTIs into if/else statements. We can undo that optimization as needed. This would allow us to keep the source code of PMCs reasonably clear in terms of intended semantics while taking advantage of short term optimizations and waiting for longer term optimizations.

[allison]

It's actually the correct semantics for the existing MULTIs in core PMCs, because a simple type ID selection is what the old C-level MMD system used, so it's what they were written for. (Some of the MMD "bugs" are caused by PMCs that expect switch semantics but are now getting full multiple dispatch.)

Switch semantics are safe for core types, because they're interacting between a pre-defined set of PMCs. Outside the core, it's less applicable, unless you're working within a tightly grouped set of PMCs (like, perhaps, all the PMCs for one HLL).

It's impossible for the PMC parser to know when it's safe to use switch semantics instead of full multiple dispatch semantics. It has to be a hand-written optimization.

[cotto]

switch Integer from is_equal MULTI to is_equal VTABLE

[cotto]

Attached is an example patch that switches Integer to use a simple is_equal VTABLE function with a switch. I have two questions about it:

First, is what should happen in general or am I missing something/doing something wrong? All tests pass except one.

Second, the failing test is the last one in t/pmc/array.t. It happens because Array's is_equal VTABLE function uses Parrot_mmd_multi_dispatch_from_c_args to call is_equal. Since Integer doesn't have an is_equal MULTI anymore, this dispatches to the wrong place (maybe Undef's is_equal MULTI?). What code needs to be fixed in this case?

[allison]

Yes, this patch is correct. The answer is that Array's is_equal VTABLE function should be calling the is_equal vtable function instead of directly calling Parrot_mmd_multi_dispatch_from_c_args. That way, each PMC is free to decide whether to use multiple dispatch or switch semantics.

[Infinoid]

The tt452_reduce_mmd branch has been created to work on this. bacek++ has been getting some pretty amazing performance improvements as a result of his work in this branch, but that work has also caused some failing tests. In particular, removing MULTI from the Integer PMC has caused these failures:

t/pmc/multidispatch.t .. 1/48 
#   Failed test 'Integer_divide_Integer  10 / 3 = 1003'
#   at t/pmc/multidispatch.t line 28.
#          got: '3.33333333333333
# '
#     expected: '1003
# '

#   Failed test '1+1=3'
#   at t/pmc/multidispatch.t line 60.
#          got: '2
# '
#     expected: '3
# '

#   Failed test 'PASM divide - override builtin 10 / 3 = 42'
#   at t/pmc/multidispatch.t line 92.
#          got: '3.33333333333333
# '
#     expected: '42
# '

#   Failed test 'PASM MMD divide - loaded sub'
#   at t/pmc/multidispatch.t line 223.
#          got: '3.33333333333333
# '
#     expected: '42
# '
t/pmc/multidispatch.t .. 46/48 # Looks like you failed 4 tests of 48.
t/pmc/multidispatch.t .. Dubious, test returned 4 (wstat 1024, 0x400)
Failed 4/48 subtests

Those tests rely on the Integer PMC being MULTI (and using MMD to override some methods), and it's not clear whether the tests are wrong or whether the Integer PMC needs to keep using MULTI.

So with this in mind, is there a general way to determine which PMCs should be using MULTI and which shouldn't?

Mark

[bacek]

Ok, tt452_reduce_mmd branch is ready for merge into trunk. I wrote message into parrot-dev mailing list and didn't recive any objections. I'll merge it soonish.

[bacek]

Replying to chromatic:

For now I'd rather see the PMC parser and code generator improved so that we can add an optimization to collapse "unnecessary" MULTIs into if/else statements. We can undo that optimization as needed. This would allow us to keep the source code of PMCs reasonably clear in terms of intended semantics while taking advantage of short term optimizations and waiting for longer term optimizations.

Collapsing VABLE creation implemented in r39253. Now we can undo that optimisation if needed.

Only one unexpected failure for dynpmc/foo dispatch. I'll fix it later today by generating proper VTABLE function for dynpmc.

-- Bacek

[doughera]

Replying to bacek:

Collapsing VABLE creation implemented in r39253. Now we can undo that optimisation if needed.

Unfortunately, that's not going to work. The following bits from PMCEmitter.pm

  $case = <<"CASE";
        default:
            if (type < enum_class_core_max)
                return $func(INTERP, SELF, $parameters);

may be appropriate for functions like Parrot_Integer_add, which return a value, but are not appropriate for functions like Parrot_Integer_i_add, which are prototyped to return void.

[bacek]

Replying to doughera:

may be appropriate for functions like Parrot_Integer_add, which return a value, but are not appropriate for functions like Parrot_Integer_i_add, which are prototyped to return void.

Fixed in r39312. Thanks!

-- Bacek

[allison]

Thanks bacek, that's a decent short-term optimization.

The remaining element of the original ticket is to review the core PMCs and see where multiple dispatch can be removed entirely. It's a little more work than a simple search-and-replace on MULTI, because our current PMCs are terrible about respecting the encapsulation boundaries of vtable functions, but it's an important code-cleanup.

[bacek]

Replying to allison:

Thanks bacek, that's a decent short-term optimization. The remaining element of the original ticket is to review the core PMCs and see where multiple dispatch can be removed entirely. It's a little more work than a simple search-and-replace on MULTI, because our current PMCs are terrible about respecting the encapsulation boundaries of vtable functions, but it's an important code-cleanup.

We probably have to consider totally opposite direction - switch everything to MULTIs instead. Explanations in #713.

-- Bacek

[jkeenan]

Can we get an update on the status of this ticket?

Thank you very much.

kid51

[plobsing]

It is not clear what exactly is deprecated here. MULTI use, but only when it is not needed? That's terribly vague.

Rejecting due to lack of specificity.