Skip to content

Adding Big Endian support#12780

Closed
miladfarca wants to merge 5 commits intoemscripten-core:mainfrom
miladfarca:be-support
Closed

Adding Big Endian support#12780
miladfarca wants to merge 5 commits intoemscripten-core:mainfrom
miladfarca:be-support

Conversation

@miladfarca
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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