Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding Big Endian support #12780

Closed
wants to merge 5 commits into from
Closed

Adding Big Endian support #12780

wants to merge 5 commits into from

Conversation

miladfarca
Copy link
Contributor

This PR modifies the getter/setters of the global HEAP objects. It adds a Proxy to the multi byte arrays as an intercepter and forces Little Endian ordering when reading/writing to the buffer by using Data Views. We force little endian as WASM itself is LE enforced on all architectures.

@welcome
Copy link

welcome bot commented Nov 15, 2020

Thank you for submitting a pull request! If this is your first PR, make sure to add yourself to AUTHORS.

src/preamble.js Outdated
return new DataView(target.buffer)[getter](byteCount * (key), true);
},
set: function (target, key, value) {
new DataView(target.buffer)[setter](byteCount * (key), value, true);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, isn't it super-inefficient to allocate a new dataview on every single memory load and store operation?

@juj
Copy link
Collaborator

juj commented Nov 15, 2020

Emscripten strongly follows "you don't pay for what you don't use" idiom on code size, because all added code increases network download size. In the history a lot of people have been turned off by Emscripten by considering it to be "bloated" when they build a small hello world and find there to be lots of code generated by default that they do not need.

Big Endian support will definitely be one of those features. Because of that, we should not unconditionally add to the code size for all users to have to pay, but instead, this should be optional. I would recommend a -s BIG_ENDIAN=0/1 flag, where BIG_ENDIAN support would kick in when -s BIG_ENDIAN=1, and we'd build by default to -s BIG_ENDIAN=0.

(maybe could also have -s BIG_ENDIAN=2 mode, which would allow retaining the LE check even for release builds, if someone wants to carry that to production. ASSERTIONS would then imply -s BIG_ENDIAN=2 by default)

Btw, if I was to add support for BE, I would not probably bother with DataView, that seems like a suboptimal solution and a large performance impact. Instead, I would recommend either to 1) add a JS macro to do heap accesses, or 2) to do an acorn-based heap access rewrite.

Solution 1) would be best for performance, but may be hard to maintain. Solution 2) will be more automatic, but can be a bit brittle. We have the same challenge with multithreaded heap accesses.

@kripken
Copy link
Member

kripken commented Nov 16, 2020

+1 for the acorn approach, I think that would make the most sense. See #12387 (comment)

@miladfarca
Copy link
Contributor Author

Thanks for reviewing.

In terms of low level memory access in JS and Endianness we can use Typed arrays /bit shifting, or DataViews.

Performance of DataViews has been improved significantly since V8 6.9 to match (or even outperform) TypedArrays. Instead of relying on C++ runtime, V8 is using Torque/TurboFan and emits machine specific instructions:
https://v8.dev/blog/dataview

Non float types could have their bytes reversed using bit shifting, however, what would be an efficient
way to reverse bytes of a Float32/64 value without using a DataView?

@kripken
Copy link
Member

kripken commented Nov 24, 2020

Interesting about the performance of DataView, that's good to know, and sounds good to use.

Reversing a float could use a singleton object on the side, to copy to in reverse order and then read from, perhaps.

Base automatically changed from master to main March 8, 2021 23:49
kripken pushed a commit that referenced this pull request Mar 9, 2021
Related to previous discussion about supporting Big Endian architectures:
#12387
#12780
@miladfarca
Copy link
Contributor Author

Closing this as #13413 has fixed the issues.

@miladfarca miladfarca closed this Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants