Re: [Mono-dev] MonoListWrapper WIP - code review/feedback

2013-07-20 Thread rfine

Hi Bartosz,

On 2013-07-19 04:56, Bartosz Przygoda wrote:

Hi,

Perfect timing, as only yesterday I was wondering how to elegantly
handle ListT from within the native, as I never worked with anything
generic before.

 A question, any particular reason you've put it in mono clone? I'd
like to fetch the sources, preferably in a form that would allow me
easy sync or even some pull requests, but it's not feasible in this
setup...


Sorry, I'm a bit new to github best-practices and etiquette...

I cloned mono because I wanted to pitch this for inclusion upstream 
once it's done - it seemed natural to me that the mono repo itself would 
include any 'embedder's toolkit' type things, and the internals-futzing 
nature of the wrapper also makes it feel like it ought to be kept close 
to the runtime. So I figured I needed to have the whole repo so I can 
(eventually) do stuff like make sure it's in the right folder, has build 
scripts, etc.


If there's a better way to set things up for this (or someone can 
persuade me that it'll never be accepted upstream), while still allowing 
you to easily use it right now, I'm all ears.


- Richard
___
Mono-devel-list mailing list
Mono-devel-list@lists.ximian.com
http://lists.ximian.com/mailman/listinfo/mono-devel-list


Re: [Mono-dev] MonoListWrapper WIP - code review/feedback

2013-07-20 Thread rfine

On 2013-07-19 11:12, Robert Jordan robe...@gmx.net wrote:


Hi,

On 18.07.2013 19:16, rf...@tbrf.net wrote:

I've been working on a little wrapper library for working with
System.Collections.Generic.ListT instances from native code. The
motivation is to provide a way for Mono embedders to easily design 
APIs
that use ListT instances as output buffers, whilst running as 
quickly
as possible and with minimal allocations. Presently I'm getting 
around a
30x speedup for bulk loading a chunk of data, compared to allocating 
a
new array to return that data in. Particular users I envision for 
this

are game engines like Unity3D.

Any chance I could get a review of what I've done so far? It's just a
couple of files, plus a short readme:

https://github.com/richard-fine/mono/tree/MonoListWrapper/contrib/MonoListWrapper


I'm interested in any there's a better way to do this observations,
suggestions for things I should add, ways to speed up the new-array
cases, bugs you can spot, or any generally un-Mono things about it.


Although I understand your motivation, I find that you're
exposing/using too much internals to make this wrapper
ready for public consumption with an unchanged Mono runtime.

You seem to have exposed mono/class-internals.h, which is
a no-go.


Mmm, I figured this is a downside of it. On the one hand I want to use 
official APIs as much as possible; on the other hand I want to cache 
things to get the best performance possible, which I think sometimes may 
require bypassing the APIs. But let's see... my use of 
mono/class-internals.h comes down to:


1) Using MonoClassField::offset to construct direct pointers to the 
values of fields in the object:


I missed that mono_field_get_offset() exists. Fixed.

2) Using mono_class_get_generic_class(), 
mono_generic_class_get_context(), and the MonoGenericContext structure 
to retrieve the ListT generic type argument:


I can't find any public API for retrieving info about a generic 
instantiation. There's a couple of methods for checking whether 
something *is* inflated, but nothing for retrieving or working with the 
MonoGenericContext that it was inflated *with*. So I'm not sure what to 
change this to. Any suggestions?



Also, poking into ListT internals isn't quite
safe, as there is no written contract between runtime and BCL
regarding ListT.


True enough - the perils of reflection-on-private-members-based 
approaches. I don't think the implementation of ListT is likely to 
change - at least not the few members in it that I'm using - but it 
would be nice not to rely on that.


FWIW, there's actually nothing in the present implementation that 
requires ListT *specifically* - any generic class with int Capacity { 
set; }, int _size, int _version and int _items will work.



It would be safer if you'd copy and rename ListT and provide
it together with your MonoListWrapper implementation.


Sure, can do. WrappableListT or something.


Also, there are public mono_array_* macros that
you can use for accessing MonoArray* elements. You can still
use them unsafely (like taking the address of the first element
and access elements using pointer arithmetic) but at least
it won't poke into too much internals.


Ah, yes, thank you. Fixed to use mono_array_length() and 
mono_array_addr_with_size().


- Richard
___
Mono-devel-list mailing list
Mono-devel-list@lists.ximian.com
http://lists.ximian.com/mailman/listinfo/mono-devel-list


Re: [Mono-dev] MonoListWrapper WIP - code review/feedback

2013-07-20 Thread Robert Jordan

On 20.07.2013 12:03, rf...@tbrf.net wrote:

2) Using mono_class_get_generic_class(),
mono_generic_class_get_context(), and the MonoGenericContext structure
to retrieve the ListT generic type argument:

I can't find any public API for retrieving info about a generic
instantiation. There's a couple of methods for checking whether
something *is* inflated, but nothing for retrieving or working with the
MonoGenericContext that it was inflated *with*. So I'm not sure what to
change this to. Any suggestions?


This is no public API for this, but you can always use the
managed reflection API:

1) get the MonoReflectionType* (the unmanaged representation of
  a System.Type object) of the MonoType* of the class using
  mono_type_get_object

2) invoke Type.GenericArguments() on this MonoReflectionType*.

Unfortunately, you'll hit the next read block after (2). There is
no public way to get a MonoType* from MonoReflectionType*, so you
have to implement a C# helper for (2):

/*
 * gets the MonoTypes* of the generic arguments of the specified Type.
 */
static IntPtr[] GenericArguments(Type type)
{
/* FIXME: check that type is a closed generic type */
var types = t.GetGenericArguments(type);
var handles = new IntPtr[types.Length];
for (int i = 0; i  types.Length; i++)
handles [i] = types[i].TypeHandle.Value;
return handles;
}



But, according to 
https://github.com/richard-fine/mono/blob/MonoListWrapper/contrib/MonoListWrapper/MonoListWrapper.c#L34

it looks like you don't need this stuff at all :)

You can obtain the element class of the array (the elementType of
your wrapper) with mono_class_get_element_class () invoked
on the class of the _items field of the ListT in question.

Robert

___
Mono-devel-list mailing list
Mono-devel-list@lists.ximian.com
http://lists.ximian.com/mailman/listinfo/mono-devel-list