bitser icon indicating copy to clipboard operation
bitser copied to clipboard

add feature to register custom userdata handlers.

Open adriweb opened this issue 3 years ago • 15 comments

Hi,

We've been using this library and needed to extend it to support userdata-based class. It works fine internally but it would be great it this can be upstreamed!

Let us know what you think.

adriweb avatar Feb 12 '21 18:02 adriweb

I am in the process of writing tests (and updating the existing one), so the Travis failure will be gone soon after I make the check know about the new type id.

In addition, I noticed an issue when several of the same userdata are used in the same table, I'll try to debug that soon - it may be because of some caching happening in the seen table.

adriweb avatar Feb 12 '21 20:02 adriweb

There we go.

It turns out the problem was exactly the same as 1a9f3d763158015dc463550b36437e33e3e9e29d . I fixed it the same way, although now I'm wondering if there is really no way to add a reference to an already seen userdata - I'm not sure why it doesn't work as is.

adriweb avatar Feb 12 '21 21:02 adriweb

Coverage Status

Coverage decreased (-3.1%) to 96.891% when pulling dff687147c3bbbd53cc49c4f4af666e52bd28f4f on adriweb:userdata into 1d3ebe04cfd6ef2dd8ebd212a6ad06e96dfdc170 on gvx:master.

coveralls avatar Feb 12 '21 22:02 coveralls

Fascinating! I hope you don't mind I'll keep thinking about if I want to merge this for a while. I might prefer to add an API for a more generic extension interface, which would subsume your pull request. This would have the benefit of allowing for plugin modules that serialize functions (like #17) as well as userdata serializers.

I am also concerned about passing the bitser internal functions to handlers via the env argument, because the bitser internals are not really intended to be safe to call outside of bitser itself, and I fear this opens users of bitser up to segfaults and other memory safety issues.

gvx avatar Feb 13 '21 15:02 gvx

Sure. I'd like to know more about what you have in mind for a more generic extension interface, this sounds interesting.

Regarding exposing some internal bits to the callbacks, yes, this feels a bit dirty, although I'm not quite sure there's another way to do that with the current codebase. Some "base" functions could be made stable API wise, I suppose (at least across major version releases), this would make it harder to break things by mistake.

adriweb avatar Feb 13 '21 18:02 adriweb

I'm working out the details still, but my thinking is to 1) have extensions specify a type string together with a matcher function to determine which values they should handle 2) have the serializer function of an extention just return a number of values that bitser can handle natively which then get serialized like such: \254 extension-id num-items value (value ...). The deserializer function receives the decoded values as arguments.

This might be a bit slower and less memory-efficient than your patch if extensions are used heavily. On the other hand: extensions wouldn't need to know bitser internals, which also means I can freely refactor those internals without breaking any extensions. And this would also mean less of a risk of extensions accidentally messing up the buffer.

gvx avatar Feb 14 '21 10:02 gvx

Alright - when you have a PoC for that, let me know, I can run my benchmarks again :)

adriweb avatar Feb 14 '21 17:02 adriweb

Hi there, any news? :)

adriweb avatar Mar 17 '21 14:03 adriweb

Not yet, I have been distracted for personal reasons the last month. I expect to be able to get back in the game shortly. I can't promise anything, but I might have a provisional extension mechanism ready in a week or so!

gvx avatar Mar 17 '21 22:03 gvx

No problem, take care!

adriweb avatar Mar 17 '21 22:03 adriweb

I've written a prototype for the extension API! Check out the extensions branch, and in particular the tentative documentation, I'd love to hear what you think!

gvx avatar Mar 30 '21 15:03 gvx

Hi there :wave:

I was wondering if we're still waiting for each other to do something, actually :D Considering we both made in-line comments, should I do something like trying to benchmark the alternative idea? Anything else?

Thanks

adriweb avatar Jun 25 '21 17:06 adriweb

Hey, I've been busy with other things for a while. If you could benchmark the performance impact you mentioned, that would be great. I'll need to look into what I need to do on my end.

gvx avatar Jul 02 '21 19:07 gvx

Ok will do!

adriweb avatar Jul 03 '21 13:07 adriweb

I didn't have time to work on it more before I left the company I was working at but hopefully convinced them to make the benchmarking tool open source soon, so I hope I can continue :)

adriweb avatar Aug 29 '21 12:08 adriweb