Adds asm.js support

[arcanis]

Here and back again 😃

This is a followup to atom/text-buffer#183 and atom-archive/buffer-offset-index#2

This PR adds asm.js support to this library through embind (instead of nbind). I've tried to keep changes as minimal as possible in the core. Everything tested is supported.

Possibly breaking changes:

• master...arcanis:master#diff-959292f9667b4cf4919bbae193a31523L57

  (returning sometimes an array and sometimes an object was causing unnecessary complications)

• master...arcanis:master#diff-e5c4747c80e4a1872cde8e24121784c0L9

  (inheriting instead of typedefing allows me to write more stable template overloads)

• master...arcanis:master#diff-ae762b481da801d5c00c8c9e5890f6e9L148

  (Buffer aren't supported under browser environments)

... and of course the delete() method (which has been polyfilled as a noop inside the v8 bindings).

All the tests are currently passing, except for Patch.compose that I have not yet been able to implement. Even if it is technically supported, the embind library has safety checks that cannot be disabled (but should, cf emscripten-core/emscripten#4863) that make it hard to support vectors of objects.

WDYT?

[arcanis]

Note that the failing test is also failing in master. You may have fixed it already in #5.

[arcanis]

I went ahead and implemented Patch.compose by hijacking the embind cast (here). It's not great, but it's not that bad either, the required change being very small (just a type that needed to be wrapped inside AllowedRawPointer<T> when working with pointers). Plus, it hopefully won't break even after the fix lands in the emscripten.

Since it was the last thing remaining, I think that this PR should be ready to merge. Let me know 😄

[arcanis]

I reverted the snakecase bindings on BufferOffsetIndex, which I believe were unintentional and were breaking the text-buffer package.

[maxbrunsfeld]

@arcanis This is really cool. We have a few questions for you:

1. Can you explain what your WRAP macros are doing? From the embind docs, it looks like you can just pass a pointer to a non-static member function directly to embind's .function method. Is there something about our methods that requires additional wrapping?

2. Are there any ways that we could change our core APIs to make the asm.js wrapping code simpler?

3. After having done this work, what are your thoughts on using embind vs nbind? Are you happy with the way this ended up, as compared to your earlier PR? Do you think we were wrong to suggest embind; would it have been better to create the asm.js bindings using nbind and continue to use the v8 API directly for the node bindings?

4. Regarding your fix for unaligned reads in Patch deserialization, do you think it would be better to ensure that all 32-bit values are written to aligned addresses during serialization, so that we could perform 32-bit reads during deserialization? I'm not sure if there would be a noticeable performance difference.

Overall, we really want this feature, but the template meta-programming in autowrap.h is pretty challenging for us to understand, with our current C++ skill level. I think we can probably maintain it if necessary, but if there's anything we can do to simplify the wrapping code, it would make our jobs easier. Thanks so much!

[arcanis]

• The em_wrap_type template links a "local type" (the type that is used on the C++ side) to a "wire type" (the type that is used on the JS side). Its receive method takes a Wire Type and use it to build a Local Type, and the transmit method do the exact opposite, taking a Local Type and 'serializing' it into a Wire Type (for example, vectors are serialized into JS arrays). There's a template specialization for each significant Local Type, and more specializations can be added the future (for example, if you decide to use std::set in the future, a new entry will have to be added), but you should never have to use them directly.

  The em_wrap_fn template takes a member function address as parameter, and return the address of a function that call the specified member function after automatically applying the em_wrap_type<>::receive method on each parameter, and the em_wrap_type<>::transmit method on the return value. Basically, it unserialize each argument and return a serialized value to the JS side. Those templates shouldn't have to be modified, their implementation is complete.

  The WRAP macros are just shorthands to call the templates without specifying redundant informations (for example, the em_wrap_fn template requires both the function pointer type and the function pointer itself, but the former can be deduced from the later and that's what WRAP is doing).

  This micro-framework is useful to support every type that isn't natively supported by embind. For example, the flat_set type needs to be casted to a javascript Set, but that obviously isn't supported by embind. Same for std::vector, std::unordered_map, etc.

• Not really. The embind library does a lot of magic for sure, but some is missing (sometimes because embind cannot make assumptions about the userland code) and has to be implemented on the wrapper itself. I'm not sure that can be easily avoided.

• Hm, they're really different. After my initial PR I went on and used nbind to port https://github.com/facebook/yoga, and it worked pretty well. There's still a couple issues that are quite troublesome, but the maintainer is usually really quick at fixing them, and it helps things a lot. All things considered, I think that nbind is the right move generally speaking (a larger community should support it, test it, improve it), but if you already have V8 bindings then maybe embind is the easiest way to add new ones. The API is full C++ and doesn't use macros, which make it a bit easier to figure out how to hack the engine like I do with auto-wrap & co.

• Not really. The issue isn't only that data aren't aligned in the buffer, but also that the buffer itself isn't aligned on 4 bytes. Since we're allocating a std::vector<uint8_t>, the allocated buffer will be aligned on 1 byte. I've tried allocating a std::vector<uint8_t __attribute__((aligned(4)))> instead, but it seems that the alignment attribute isn't used by the standard container allocators (what's the point, you might ask ...). I've read some docs, and it seems to be a problem in the C++ standard, and both Clang and GCC are affected.

• I can see the issue with the metaprogramming. The whole idea behind it is that it makes it quite easy to register new types (about ten lines), and that once implemented such a type binding will be automatically used for each function, being more DRY and avoiding possible mistakes. Unfortunately, it comes with a cost, and this cost is clarity. That being said, I would be happy to review any changes you would like to make to this micro-framework in the future if that can help you.

[nathansobo]

Hey @arcanis. Good answers. @maxbrunsfeld and myself both have overlapping vacations until the end of next week. When we return we will investigate this a bit more and proceed. Just wanted to let you know this could sit for a few days but we haven't forgotten about it.

[arcanis]

No worry, thanks for the heads-up :)

[arcanis]

@nathansobo Have you had the chance to review this?

[nathansobo]

We've gotten pulled off to more reactive topics. I'll talk to @maxbrunsfeld about it this week.

[nathansobo]

Hey @arcanis. As you can see we're pushing some adjustments to your branch. We plan to drop reliance on the legacy Patch change fields in text-buffer and then merge. Thanks for your patience.

[maxbrunsfeld]

Thanks so much for figuring this all out, @arcanis. I'll publish a new version of this module shortly and make the necessary changes in text-buffer to use the new version.

[arcanis]

[nathansobo]

❤️ Thanks for the contribution.