[Mono-dev] IMT patch

[Paolo Molaro]

On 05/31/07 Massimiliano Mantione wrote:
> Initially it looked "mildly unstable": when rebuilding the whole Mono
> I got a couple of NRE in the mcs symbol writer while compiling some
> library; just typing make again worked, so the problem was very hard
> to reproduce.

Does it run the corlib tests for you? It fails here:
.Unhandled Exception:
System.InvalidCastException: Value is not a convertible object:
NUnit.Core.NormalTestCase to NUnit.Core.ITest  at (wrapper
xdomain-invoke) EventCollector:TestFinished (NUnit.Core.TestCaseResult)
  at NUnit.Core.RemoteTestRunner.NUnit.Core.EventListener.TestFinished
  (NUnit.Core.TestCaseResult result) [0x00000] in
  /opt/lupus/svn/mcs/nunit20/core/RemoteTestRunner.cs:533

This is likely related to handling of collisions for transparent proxies
vtables: the bitmap used to flag collisions in the imt table needs to be
stored in the tproxy data structures and checked there instead of in the
MonoClass in that case.

>   BTW, do you think some of the print statements can stay there anyway?

They may be useful while porting the code to other architectures, but
after that most or all should be removed. Note that all the #if commands
should be aligned on column 0.

> - Move "mono_convert_imt_slot_to_vtable_slot" elsewhere (mini.c?).

mini-trampolines.c is likely better.

> - Does setting "MonoClass.imt_collisions_bitmap" need locking?
>   If yes, it should be the loader lock, but does acquiring it with the
>   domain lock held (we are building the MonoVTable) risk a deadlock?

You should add an assert so that the size of the imt table is never set
to a value bigger than the bitmap field.

> - Add cumulative size of thunks to mono stats.

The IMT table should also be avoided for types that don't implement
interfaces. It is also likely a good idea to limit the size to the
highest used slot.

> - Also add general "performance" of the hash function to the stats?
>   I mean, collision ratio and things like that...

A counter for number of collisions, the max number of chained methods
and the mean number of chained methods in collisions.

> - Maybe rewrite the thunks to do binary search instead of linear.

Yes, it should do a binary search, at least for n > 3 or 4.

> Do you think this should be committed before the other archs are ported,
> or should we wait for the port to be complete and commit without the
> current code path?

Let's finish the other todos and make tests work first.

> Index: mono/metadata/object.c
> ===================================================================
> --- mono/metadata/object.c	(revision 78299)
> +++ mono/metadata/object.c	(working copy)
[...]
> +guint32
> +mono_compute_imt_slot (MonoMethod *method) {

A better name is mono_method_get_imt_slot (). Also, since this needs to
be called only on the abstract methods of an interface it's useful to
add the proper asserts here. This may also be one of the causes of the
bugs that show up running the nunit tests (think about explicit iface
implementations: the method name is different from the name of the
method in the iface, so it will result in the wrong imt slot).

> +	MonoMethodSignature *sig = mono_method_signature (method);
> +	int hashes_count = sig->param_count + 4;
> +	guint32 *hashes = alloca (hashes_count * sizeof (guint32));

We should try to avoid the use of alloca in the runtime, especially
where it is trivially avoidable like here.

> Index: mono/metadata/object-internals.h
> ===================================================================
> --- mono/metadata/object-internals.h	(revision 78299)
> +++ mono/metadata/object-internals.h	(working copy)
> @@ -1130,5 +1130,22 @@
>  MonoObject*
>  mono_nullable_box (guint8 *buf, MonoClass *klass) MONO_INTERNAL;
>  
> +#define MONO_IMT_SIZE 4
> +typedef struct _MonoImtBuilderEntry {
> +	MonoMethod *method;
> +	int vtable_slot;
> +	struct _MonoImtBuilderEntry *next;
> +	int children;
> +} MonoImtBuilderEntry;
> +typedef struct _MonoImtBuilderEntry** MonoImtBuilder;
> +
> +typedef void (*MonoImtSlotInitializer) (gpointer **slot, MonoVTable *vtable, MonoDomain *domain, MonoImtBuilderEntry *imt_builder_entry);

Why don't we just return the slot value from the function instead of
passing a pointer?

> Index: mono/mini/mini-x86.c
> ===================================================================
> --- mono/mini/mini-x86.c	(revision 78299)
> +++ mono/mini/mini-x86.c	(working copy)
> @@ -4085,6 +4085,85 @@
>  	}
>  }
>  
> +#define DEBUG_IMT 0
> +#define MONO_ARCH_IMT_REG X86_EDX
> +void
> +mono_arch_emit_imt_argument (MonoCompile *cfg, MonoCallInst *call)
> +{
> +	MonoInst *inst;
> +	MONO_INST_NEW (cfg, inst, OP_PCONST);
> +	//MONO_INST_NEW (cfg, inst, CEE_LDIND_I);

Remove this commented line.

> +	inst->inst_p0 = call->method;
> +	inst->dreg = mono_regstate_next_int (cfg->rs);
> +	mono_bblock_add_inst (cfg->cbb, inst);
> +
> +	mono_call_inst_add_outarg_reg (cfg, call, inst->dreg, MONO_ARCH_IMT_REG, FALSE);
> +}

mono_arch_emit_imt_argument() is likely going to be the same for most of the
architectures, so it should be in common code.

> +
> +//[1 + 4] x86_alu_reg_imm (code, X86_CMP, ins->sreg1, ins->inst_imm);
> +//[1 + 1] x86_branch8(inst,cond,imm,is_signed)
> +//        x86_patch(ins,target)
> +//[1 + 5] x86_jump_mem(inst,mem)
> +#define IMT_THUNK_SLOT_HANDLER_LENGTH 14
> +#define IMT_THUNK_END_HANDLER_LENGTH 1
> +void
> +mono_arch_initialize_imt_slot (gpointer **slot, MonoVTable *vtable, MonoDomain *domain, MonoImtBuilderEntry *imt_builder_entry) {

Most of this function is arch-indep code: only the chaining codegen
should be implemented by the different architectures, to minimize code
duplication.

> +/*
> + * FIXME:
> + * The following function is arch-independent, and should go in another
> + * file to be reused (as object.c, with prototype in object-internals.h)

mini-trampolines.c is more appropriate, since this function is
jit-specific.

Thanks!

lupus

-- 
-----------------------------------------------------------------
lupus at debian.org                                     debian/rules
lupus at ximian.com                             Monkeys do it better