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

[rfine at tbrf.net]

On 2013-07-19 11:12, Robert Jordan <robertj at gmx.net> wrote:
> 
> Hi,
> 
> On 18.07.2013 19:16, rfine at tbrf.net wrote:
>> I've been working on a little wrapper library for working with
>> System.Collections.Generic.List<T> instances from native code. The
>> motivation is to provide a way for Mono embedders to easily design 
>> APIs
>> that use List<T> 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 List<T> 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 List<T> internals isn't quite
> safe, as there is no "written" contract between runtime and BCL
> regarding List<T>.

True enough - the perils of reflection-on-private-members-based 
approaches. I don't think the implementation of List<T> 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 List<T> *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 List<T> and provide
> it together with your MonoListWrapper implementation.

Sure, can do. WrappableList<T> 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