[Mono-dev] [PATCH] Managed Marshal.Copy implemantations

[Kornél Pál]

Hi,

I understand your objections and I agree with most of them.

If there were something like a fast Array.GetElementSize() that could be 
used in Marshal.UnsafeAddrOfPinnedArrayElement as well as in System.Buffer 
methods. System.Buffer is supposed to be used over Array.Copy for better 
performance so I believe that having a managed System.Buffer would affect 
overall performance.

You have written that GetElementSize() should not be added yet. Is there an 
ongoing work that would interfere with this or why should that wait?

Thanks for your help.

Kornél

----- Original Message ----- 
From: "Paolo Molaro" <lupus at ximian.com>
To: <mono-devel-list at lists.ximian.com>
Sent: Thursday, December 20, 2007 4:14 PM
Subject: Re: [Mono-dev] [PATCH] Managed Marshal.Copy implemantations


On 12/20/07 Kornél Pál wrote:
>> It's an extra check because the shared methods will have Array as the
>> type and they could be misused.
>
> They are private so I prefer not to be used anywhere else than extra
> checks. (I added comments before these methods about that.)

The check is cheap and it should go in.

BTW, please send patches as text attachments, not as binaries, thanks.

> Index: mono/mono/metadata/icall-def.h
> ===================================================================
> --- mono/mono/metadata/icall-def.h (revision 91652)
> +++ mono/mono/metadata/icall-def.h (working copy)
> @@ -91,15 +91,17 @@
>  ICALL(ARRAY_2, "Clone",            mono_array_clone)
>  ICALL(ARRAY_3, "CreateInstanceImpl", 
> ves_icall_System_Array_CreateInstanceImpl)
>  ICALL(ARRAY_4, "FastCopy",         ves_icall_System_Array_FastCopy)
> -ICALL(ARRAY_5, "GetGenericValueImpl", 
> ves_icall_System_Array_GetGenericValueImpl)
> -ICALL(ARRAY_6, "GetLength",        ves_icall_System_Array_GetLength)
> -ICALL(ARRAY_7, "GetLowerBound",    ves_icall_System_Array_GetLowerBound)
> -ICALL(ARRAY_8, "GetRank",          ves_icall_System_Array_GetRank)
> -ICALL(ARRAY_9, "GetValue",         ves_icall_System_Array_GetValue)
> -ICALL(ARRAY_10, "GetValueImpl",     ves_icall_System_Array_GetValueImpl)
> -ICALL(ARRAY_11, "SetGenericValueImpl", 
> ves_icall_System_Array_SetGenericValueImpl)
> -ICALL(ARRAY_12, "SetValue",         ves_icall_System_Array_SetValue)
> -ICALL(ARRAY_13, "SetValueImpl",     ves_icall_System_Array_SetValueImpl)
> +ICALL(ARRAY_5, "GetElementSize",   ves_icall_System_Array_GetElementSize)

There is no need to rewrite everything, just add your icall.

> @@ -395,7 +397,7 @@
>  ICALL(OBJ_1, "GetType", ves_icall_System_Object_GetType)
>  ICALL(OBJ_2, "InternalGetHashCode", mono_object_hash)
>  ICALL(OBJ_3, "MemberwiseClone", ves_icall_System_Object_MemberwiseClone)
> -ICALL(OBJ_4, "obj_address", ves_icall_System_Object_obj_address)
> +ICALL(OBJ_4, "UnsafeAddrOfPinnedObject", 
> ves_icall_System_Object_UnsafeAddrOfPinnedObject)

Uneeded renaming, drop it.

> Index: mono/mono/metadata/icall.c
> ===================================================================
> --- mono/mono/metadata/icall.c (revision 91652)
> +++ mono/mono/metadata/icall.c (working copy)
> @@ -713,6 +713,22 @@
>  memcpy (ea, value, esize);
>  }
>
> +static gint
> +ves_icall_System_Array_GetElementSize (MonoObject *this)
> +{
> + MONO_ARCH_SAVE_REGS;
> +
> + return mono_array_element_size (this->vtable->klass);
> +}

This icall is not needed for this change: as I mentioned it's better to
add a size argument to copy_to_unmanaged etc.

> +static gint
> +ves_icall_System_Array_GetOffsetToArrayData (void)
> +{
> + MONO_ARCH_SAVE_REGS;
> +
> + return offsetof (MonoArray, vector);
> +}

This is not the icall that I suggested: you need to return an IntPtr
to the beginning of the elements:

static void*
array_get_array_data_ptr (MonoArray *array)
{
return &array->vector [0];
}

> Index: mono/mono/mini/mini.c
> ===================================================================
> --- mono/mono/mini/mini.c (revision 91652)
> +++ mono/mono/mini/mini.c (working copy)
> @@ -3449,13 +3449,61 @@
>  ins->inst_i0 = args [0];
>  return ins;
>  #endif
> + } else if (strcmp (cmethod->name, "UnsafeAddrOfPinnedObject") == 0) {
> + return args [0];

This is not correct and this icall should not be optimized, since there
are no users.

>  } else if (strcmp (cmethod->name, ".ctor") == 0) {
>   MONO_INST_NEW (cfg, ins, OP_NOP);
>  return ins;
>  } else
>  return NULL;
>  } else if (cmethod->klass == mono_defaults.array_class) {
> - if (cmethod->name [0] != 'g')
> + if (cmethod->name [0] == 'G') {
> + if (strcmp (cmethod->name, "GetElementSize") == 0) {
> + MonoInst *load_vtable, *load_klass;
> +
> + MONO_INST_NEW (cfg, load_vtable, CEE_LDIND_I);
> + if (G_STRUCT_OFFSET (MonoObject, vtable)) {

vtable will always be at offset 0.
Anyway, this icall should not be added for now.

> Index: mcs/class/corlib/System.Runtime.InteropServices/Marshal.cs
> ===================================================================
> --- mcs/class/corlib/System.Runtime.InteropServices/Marshal.cs (revision 
> 91652)
> +++ mcs/class/corlib/System.Runtime.InteropServices/Marshal.cs (working 
> copy)
> @@ -1020,8 +1060,10 @@
>  throw ex;
>  }
>
> - [MethodImplAttribute(MethodImplOptions.InternalCall)]
> - public extern static IntPtr UnsafeAddrOfPinnedArrayElement (Array arr, 
> int index);
> + public static unsafe IntPtr UnsafeAddrOfPinnedArrayElement (Array arr, 
> int index)
> + {
> + return (IntPtr) ((byte*) object.UnsafeAddrOfPinnedObject (arr) + 
> Array.GetOffsetToArrayData () + arr.GetElementSize () * index);
> + }

I would leave this in unmanaged for now.
Thanks!

lupus

-- 
-----------------------------------------------------------------
lupus at debian.org                                     debian/rules
lupus at ximian.com                             Monkeys do it better
_______________________________________________
Mono-devel-list mailing list
Mono-devel-list at lists.ximian.com
http://lists.ximian.com/mailman/listinfo/mono-devel-list