Changing how structures are created

[kdschlosser]

I have been spending a pretty decent amount of time going over the generated code for the binding and I believe we can improve how the structures are created to do 2 things.

Currently this is what needs to be done to create a structure...

import lvgl as lv

point = lv.point_t({'x': 100, 'y': 100})

If you notice a dictionary needs to be passed to the constructor when setting the fields. Firstly this is not very pythonic but it also adds a decent amount of additional memory use in several different ways. The first being from loading the source code. More characters in a source file means more memory consumption. Then you also have the mechanics of creating the dictionary which uses more memory as well.

Currently the way things are processed is like this...

structure make_new function gets called.
the arguments are checked to see if there is a dictionary is passed.
if a dictionary is passed then the dictionary is mapped and then iterated over.
the function that sets the attributes (field) gets called passing the key (field name) and value (field value)

How micropython handles positional arguments and keyword arguments is actually set up in a manner that would work perfect for eliminating the need to pass a dictionary as the first argument.

The "__init__" function (construction) in C code looks like the following.

static mp_obj_t make_new_lv_struct(const mp_obj_type_t *type, size_t n_args, size_t n_kw, const mp_obj_t *args)

type is the wrapper around the structure that is wanting to be created
n_args is the number of positional arguments that are passed to the constructor.
n_kw is the number of keyword arguments that have been passed to the constructor
args is an array of objects.

what the args parameter holds are the arguments that have been passed to the constructor. It holds both the positional as well as the keyword arguments. It is a single dimension array.

Here is an example of how the data is stored in that array.

if we use this code to construct the structure. (notice I am not using a dictionary)

import lvgl as lv

point = lv.point_t(x=100, y=100)

The values of the parameters for the make_new C function would be

type: mp_lv_point_t_type
n_args: 0
n_kw: 2
args: { qstr key, mp_obj_t value, qstr key, mp_obj_t value }

All the information that we need is in that array when passing keyword parameters to the constructors. passing a dictionary as a positional argument doesn't need to be done.

this is the current constructor that is used for structures.

static mp_obj_t make_new_lv_struct(
    const mp_obj_type_t *type,
    size_t n_args,
    size_t n_kw,
    const mp_obj_t *args)
{
    if ((!MP_OBJ_IS_TYPE(type, &mp_type_type)) || MP_OBJ_TYPE_GET_SLOT_OR_NULL(type, make_new) != &make_new_lv_struct)
        nlr_raise(
            mp_obj_new_exception_msg(
                &mp_type_SyntaxError, MP_ERROR_TEXT("Argument is not a struct type!")));
    size_t size = get_lv_struct_size(type);
    mp_arg_check_num(n_args, n_kw, 0, 1, false);
    mp_lv_struct_t *self = m_new_obj(mp_lv_struct_t);
    mp_lv_struct_t *other = (n_args > 0) && (!mp_obj_is_int(args[0])) ? mp_to_lv_struct(cast(args[0], type)): NULL;
    size_t count = (n_args > 0) && (mp_obj_is_int(args[0]))? mp_obj_get_int(args[0]): 1;
    *self = (mp_lv_struct_t){
        .base = {type},
        .data = (size == 0 || (other && other->data == NULL))? NULL: m_malloc(size * count)
    };
    if (self->data) {
        if (other) {
            memcpy(self->data, other->data, size * count);
        } else {
            memset(self->data, 0, size * count);
        }
    }
    return MP_OBJ_FROM_PTR(self);
}

the call to cast is where the dictionary that has been passed gets used...

static mp_obj_t cast(mp_obj_t mp_obj, const mp_obj_type_t *mp_type)
{
    mp_obj_t res = NULL;
    if (mp_obj == mp_const_none && MP_OBJ_TYPE_GET_SLOT_OR_NULL(mp_type, make_new) == &make_new_lv_struct) {
        res = MP_OBJ_FROM_PTR(&mp_lv_null_obj);
    } else if (MP_OBJ_IS_OBJ(mp_obj)) {
        res = get_native_obj(mp_obj);
        if (res){
            const mp_obj_type_t *res_type = ((mp_obj_base_t*)res)->type;
            if (res_type != mp_type){
                if (res_type == &mp_type_dict &&
                    MP_OBJ_TYPE_GET_SLOT_OR_NULL(mp_type, make_new) == &make_new_lv_struct)
                        res = dict_to_struct(res, mp_type);
                else res = NULL;
            }
        }
    }
    if (res == NULL) nlr_raise(
        mp_obj_new_exception_msg_varg(
            &mp_type_SyntaxError, MP_ERROR_TEXT("Can't convert %s to %s!"), mp_obj_get_type_str(mp_obj), qstr_str(mp_type->name)));
    return res;
}

the call to dict_to_struct is where the fields gets set.

static mp_obj_t dict_to_struct(mp_obj_t dict, const mp_obj_type_t *type)
{
    mp_obj_t mp_struct = make_new_lv_struct(type, 0, 0, NULL);
    mp_obj_t native_dict = cast(dict, &mp_type_dict);
    mp_map_t *map = mp_obj_dict_get_map(native_dict);
    if (map == NULL) return mp_const_none;
    for (uint i = 0; i < map->alloc; i++) {
        mp_obj_t key = map->table[i].key;
        mp_obj_t value = map->table[i].value;
        if (key != MP_OBJ_NULL) {
            mp_obj_t dest[] = {MP_OBJ_SENTINEL, value};
            MP_OBJ_TYPE_GET_SLOT(type, attr)(mp_struct, mp_obj_str_get_qstr(key), dest);
            if (dest[0]) nlr_raise(
                mp_obj_new_exception_msg_varg(
                    &mp_type_SyntaxError, MP_ERROR_TEXT("Cannot set field %s on struct %s!"), qstr_str(mp_obj_str_get_qstr(key)), qstr_str(type->name)));
        }
    }
    return mp_struct;
}

Now that is simply an obnoxious amount of code that has to run in order to set the fields at the time a structure is constructed.

I propose something along these lines as a replacement.

static mp_obj_t make_new_lv_struct(
    const mp_obj_type_t *type,
    size_t n_args,
    size_t n_kw,
    const mp_obj_t *args)
{
    if ((!MP_OBJ_IS_TYPE(type, &mp_type_type)) || MP_OBJ_TYPE_GET_SLOT_OR_NULL(type, make_new) != &make_new_lv_struct)
        nlr_raise(
            mp_obj_new_exception_msg(
                &mp_type_SyntaxError, MP_ERROR_TEXT("Argument is not a struct type!")));
    
    size_t size = get_lv_struct_size(type);
    
    // checks for both keyword and positional arguments. we tell it that 
    // anywhere from 0 to 99 parameters can be passed because this constructor 
    // gets used for all structures the actual number of fields is unknown
    mp_arg_check_num(n_args, n_kw, 0, 99, true);
                
    // I am not sure whast the original handling os passing a positional integer 
    // to the constructor was intended to do. I think it was intended to create 
    // an array of structures but it was never completed perhaps? 
    // I have altered the code so an array is what gets returned if an integer 
    // is supplied as a positional argument.
    size_t count = 0;
    if (n_args > 0) {
         if (!mp_obj_is_int(args[0])) {
             nlr_raise(mp_obj_new_exception_msg(&mp_type_SyntaxError, MP_ERROR_TEXT("Only integers can be passed as positional arguments when creating a structure")));
             return mp_const_none;
         }
         count = (size_t)mp_obj_get_int_truncated(args[0]);
    }
      
    if (count > 0) {
        mp_obj_t res = mp_obj_new_list(0, NULL);
        for (size_t i=0;i<count;i++) {
            mp_obj_list_append(res, make_new_lv_struct(type, 0, 0, NULL));
        }
        return res;
    }
    
    mp_lv_struct_t *self = m_new_obj(mp_lv_struct_t);
    
    *self = (mp_lv_struct_t){
        .base = {type},
        .data = (size == 0) ? NULL: m_malloc(size)
    };
    
    if (self->data) {
        if (n_kw > 0) {
            for (size_t i = 0; i < n_kw; i+=2) {
               mp_obj_t dest[] = {MP_OBJ_SENTINEL, args[i + 1]};
                MP_OBJ_TYPE_GET_SLOT(type, attr)(self, MP_OBJ_QSTR_VALUE(args[i]), dest);
                if (dest[0]) {
                    nlr_raise(mp_obj_new_exception_msg_varg(&mp_type_SyntaxError, MP_ERROR_TEXT("Cannot set field %s on struct %s!"), qstr_str(mp_obj_str_get_qstr(key)), qstr_str(type->name)));
                }
            }
        }
    }
    
    return MP_OBJ_FROM_PTR(self);
}

That will take keywords for setting the fields instead of a dictionary when constructing the structure. Optionally if a positional integer is passed instead of keywords it will create a list (array) of empty structures instead. we can also have to use both keywords and a positional integer to create an array of structures that all have their fields set to the same thing. Not sure if that would be useful or not.

The point being is is would reduce the code footprint which would lessen memory consumption and I also believe it is more efficient code in terms of stack use and also speed.

The code above is pseudo code and it has not been tested. The idea is sound and it should work but other changes elsewhere might need to be made to get it to work properly. I believe that this should be considered due to the mechanics of how python works and passing a dict literal as a positional argument is really not the most ideal thing to do. This kind of a change is also something that a user would be able to easily adapt their code by doing a simple replacement

point = lv.point_t({'x': 100, 'y': 100})

regex replacement of _t\(\{ to _t(**{

so you end up with this..

point = lv.point_t(**{'x': 100, 'y': 100})

and that would allow the code to run properly so the user could change to using keywords without the dict literal whenever they want to.