Juerg,

That is is interesting proposal. As an alternative to letting users hook up arbitrary Go function for comparison, I have also thought about the possibility of providing a small set of static C functions usable for comparison. A flexible compound key comparison function like this could fit well into that idea.

Howard,

Sorry I did not find the issues mentioned in previous searches.

I understand the concern about such a hot code path. I'm not sure that Go would see acceptable performance.

But, Go is not an interpreted language (though there is glue). And while I'm not positive about the performance of Go in this area you seem to dismiss comparison functions in any other language. Is it unreasonable to think that comparison functions written in other compiled languages like Rust, Nim, or ML variants would also be impractically slow?

I also believe you have misunderstood the practical problems of passing Go function pointers to C. But to be fair, I think the wording of that quoted paragraph could be better.

> Sorry but there is no other Go function for the mdb_cmp() function to call, the only one it knows about is the function pointer that you pass.

It may be of benefit to see how the I've used the context argument in a binding being developed for the mdb_reader_list function.

https://github.com/bmatsuo/lmdb-go/blob/bmatsuo/reader-list-context-fix/lmdb/lmdbgo.c

The callback passed to mdb_reader_list is always the same static function because correctly calling a Go function from C requires an annotated static Go function. The context argument allows dispatch to the correct Go function that was configured at runtime. I believe that is the "other" Go function you mentioned.

The implementation would be similar for mdb_set_compare. The callback would always be the same static function which handles the dynamic dispatch.

Cheers,

- Bryan

On Thu, Oct 29, 2015 at 3:12 AM Jürg Bircher <juerg.bircher@helmedica.com> wrote:

Actually I’m not commenting on binding Go but I’m voting for a context passed to the compare function.

I fully agree that the compare function is part of the critical path. But as I need to define custom indexes with compound keys the compare functions varies and it would be impractical to predefine for any compound key combination a c function.

The compare context would be stored on the struct MDB_dbx.

typedef struct MDB_dbx {
        MDB_val         md_name;                /**< name of the database */
        MDB_cmp_func    *md_cmp;        /**< function for comparing keys */
        void            *md_cmpctx; /** user-provided context for md_cmp **/
        MDB_cmp_func    *md_dcmp;       /**< function for comparing data items */
        void            *md_dcmpctx;/** user-provided context for md_dcmp **/
        MDB_rel_func    *md_rel;        /**< user relocate function */
        void            *md_relctx;             /**< user-provided context for md_rel */
} MDB_dbx;

The following is a draft (not tested yet) of a generic compare function. The context contains a compare specification which is a null terminated list of <type><order> pairs.

// compareSpec <type><order>...<null>
int key_comp_generic(const MDB_val *a, const MDB_val *b, char *compareSpec) {
    int result = 0;
    char *pa = a->mv_data;
    char *pb = b->mv_data;

    while (1) {
        switch (*compareSpec++) {
           case 0:
                break;
           case INT32_KEY :
           {
                unsigned int va = *(unsigned int *)pa;
                unsigned int vb = *(unsigned int *)pb;

                if (*compareSpec++ == ASCENDING_ORDER) {
                    result = (va < vb) ? -1 : va > vb;
                }
                else {
                    result = (va > vb) ? -1 : va < vb;
                }

                if (result != 0) {
                    break;
                }
                else {
                    pa += 4;
                    pb += 4;
                }
            }
            case INT64_KEY :
            {
                unsigned long long va = *(unsigned long long *)pa;
                unsigned long long vb = *(unsigned long long *)pb;

                if (*compareSpec++ == ASCENDING_ORDER) {
                    result = (va < vb) ? -1 : va > vb;
                }
                else {
                    result = (va > vb) ? -1 : va < vb;
                }

                if (result != 0) {
                    break;
                }
                else {
                    pa += 8;
                    pb += 8;
                }
            }
            case STRING_KEY :
            {
                unsigned int la = *(unsigned int *)pa;
                unsigned int lb = *(unsigned int *)pb;

                pa += 4;
                pb += 4;

                if (*compareSpec++ == ASCENDING_ORDER) {
                    result = strncmp(pa, pb, (la < lb) ? la : lb);
                    if (result != 0) {
                        break;
                    }
                    else {
                        result = (la < lb) ? -1 : la > lb;
                    }
                }
                else {
                    result = strncmp(pb, pa, (la < lb) ? la : lb);
                    if (result != 0) {
                        break;
                    }
                    else {
                        result = (la > lb) ? -1 : la < lb;
                    }
                }

                if (result != 0) {
                    break;
                }
                else {
                    pa += la;
                    pb += lb;
                }
            }
        }
    }

    return result;
}

Regards
Juerg


On 29/10/15 10:40, "openldap-technical on behalf of Howard Chu" <openldap-technical-bounces@openldap.org on behalf of hyc@symas.com> wrote:

>Bryan Matsuo wrote:
>> openldap-technical,
>>
>> I am working on some Go (golang) bindings[1] for the LMDB library and I have
>> some interest in exposing the functionality of mdb_set_compare (and
>> mdb_set_dupsort). But it is proving difficult and I have a question about the
>> function(s).
>>
>> Calling mdb_set_compare from the Go runtime is challenging. Using C APIs with
>> callbacks comes with restrictions[2][3]. I believe it impossible to bind these
>> functions way that is flexible, as one would expect. A potential change to
>> LMDB that would make binding drastically easier is having MDB_cmp_func to take
>> a third "context" argument with type void*. Then a binding could safely use an
>> arbitrary Go function for comparisons.
>>
>> Is it possible for future versions of LMDB to add a third argument to the
>> MDB_cmp_func signature? Otherwise would it be acceptable for a variant API to
>> be added using a different function type, one accepting three arguments?
>>
>> Thanks for the consideration.
>>
>> Cheers,
>> - Bryan
>>
>> [1] Go bindings -- https://github.com/bmatsuo/lmdb-go
>> [2] Cgo pointer restrictions --
>> https://github.com/golang/proposal/blob/master/design/12416-cgo-pointers.md
>> [3] Cgo documentation -- https://golang.org/cmd/cgo/
>
>I see nothing in these restrictions that requires extra information to be
>passed from Go to C or from C to Go.
>
>There is a vague mention in [2]
>
>"A particular unsafe area is C code that wants to hold on to Go func and
>pointer values for future callbacks from C to Go. This works today but is not
>permitted by the invariant. It is hard to detect. One safe approach is: Go
>code that wants to preserve funcs/pointers stores them into a map indexed by
>an int. Go code calls the C code, passing the int, which the C code may store
>freely. When the C code wants to call into Go, it passes the int to a Go
>function that looks in the map and makes the call."
>
>But it's nonsense in this case - you want to pass a Go function pointer to C,
>but the only way for C to use it is to call some *other* Go function? Sorry
>but there is no other Go function for the mdb_cmp() function to call, the only
>one it knows about is the function pointer that you pass.
>
>If this is what you're referring to, adding a context pointer doesn't achieve
>anything. If this isn't what you're referring to, then please explain exactly
>what you hope to achieve with this context pointer.
>
>--
>   -- Howard Chu
>   CTO, Symas Corp.           http://www.symas.com
>   Director, Highland Sun     http://highlandsun.com/hyc/
>   Chief Architect, OpenLDAP  http://www.openldap.org/project/