Add support for number slices of type MaybeUninit<T>#4316
Merged
daxpedda merged 3 commits intowasm-bindgen:mainfrom Dec 6, 2024
Merged
Add support for number slices of type MaybeUninit<T>#4316daxpedda merged 3 commits intowasm-bindgen:mainfrom
MaybeUninit<T>#4316daxpedda merged 3 commits intowasm-bindgen:mainfrom
Conversation
a23c511 to
2c6b2d0
Compare
Contributor
RunDevelopment
left a comment
There was a problem hiding this comment.
Please document the behavior of reading MaybeUninit<T> values in JS. I think a small paragraph saying that it's not UB, but that the value is unspecified would be enough.
Also, please add a CLI reference test, so we can see the generated JS and TS.
Contributor
RunDevelopment
left a comment
There was a problem hiding this comment.
One last comment, then gtg 👍
Comment on lines
+7
to
+8
| > **Note:** While reading from an uninitialized `MaybeUninit` is unsafe in | ||
| > Rust, it is not in Wasm. Though they might contain unspecified values. |
Contributor
There was a problem hiding this comment.
Minor corrections:
- Reading uninitialized memory in Rust isn't just unsafe, it's UB.
- WASM doesn't do the reading, the JS users do.
Suggested change
| > **Note:** While reading from an uninitialized `MaybeUninit` is unsafe in | |
| > Rust, it is not in Wasm. Though they might contain unspecified values. | |
| > **Note:** While reading from an uninitialized `MaybeUninit` is UB in Rust, it is safe in JavaScript. Though they might contain unspecified values. |
Member
Author
There was a problem hiding this comment.
Hm ... I was making a note for Rust users as well, that when receiving MaybeUninit there is nothing to worry about. But does that even make sense?
Maybe the note should only talk to JS users?
Contributor
There was a problem hiding this comment.
You're right, that's a good idea. I think it would be good to say that data coming from JS is always initialized, so taking a &[MaybeUninit<u8>], while supported, typically doesn't make sense.
Member
Author
There was a problem hiding this comment.
Okay, after a lot of different tries I arrived at the current state. I was trying not to explain Rust or Wasm but stay concise to what users need to know only for this feature and nothing else.
Member
Author
There was a problem hiding this comment.
Let me know if you want me to improve anything still, we can do that in a follow-up.
e36290e to
8275a98
Compare
Co-Authored-By: Michael Schmidt <msrd0000@gmail.com>
Co-Authored-By: Michael Schmidt <msrd0000@gmail.com>
8275a98 to
7b030d6
Compare
newpavlov
pushed a commit
to rust-random/getrandom
that referenced
this pull request
Dec 9, 2024
As discussed in #541. This PR adds the following improvements: - Caching of the global `Crypto` object. - Detecting if our Wasm memory is based on a `SharedArrayBuffer`. If not, we can copy bytes directly into our memory instead of having to go through JS. This saves allocating the buffer in JS and copying the bytes into Wasm memory. This is also the most common path. `SharedArrayBuffer` requires `target_feature = "atomics"`, which is unstable and requires Rust nightly. See #559 (comment) for full context. - The atomic path only creates a sub-array when necessary, potentially saving another FFI call. - The atomic path will now allocate an `Uint8Array` with the minimum amount of bytes necessary instead of a fixed size. - The maximum chunk size for the non-atomic path and the maximum `Uint8Array` size for the atomic paths have been increased to 65536 bytes: the maximum allowed buffer size for `Crypto.getRandomValues()`. All in all this should give a performance improvement of ~5% to ~500% depending on the amount of requested bytes and which path is taken. See #559 (comment) for some benchmark results. This spawned a bunch of improvements and fixes in `wasm-bindgen` that are being used here: - wasm-bindgen/wasm-bindgen#4315 - wasm-bindgen/wasm-bindgen#4316 - wasm-bindgen/wasm-bindgen#4318 - wasm-bindgen/wasm-bindgen#4319 - wasm-bindgen/wasm-bindgen#4340
takumi-earth
pushed a commit
to earthlings-dev/getrandom
that referenced
this pull request
Jan 27, 2026
As discussed in rust-random#541. This PR adds the following improvements: - Caching of the global `Crypto` object. - Detecting if our Wasm memory is based on a `SharedArrayBuffer`. If not, we can copy bytes directly into our memory instead of having to go through JS. This saves allocating the buffer in JS and copying the bytes into Wasm memory. This is also the most common path. `SharedArrayBuffer` requires `target_feature = "atomics"`, which is unstable and requires Rust nightly. See rust-random#559 (comment) for full context. - The atomic path only creates a sub-array when necessary, potentially saving another FFI call. - The atomic path will now allocate an `Uint8Array` with the minimum amount of bytes necessary instead of a fixed size. - The maximum chunk size for the non-atomic path and the maximum `Uint8Array` size for the atomic paths have been increased to 65536 bytes: the maximum allowed buffer size for `Crypto.getRandomValues()`. All in all this should give a performance improvement of ~5% to ~500% depending on the amount of requested bytes and which path is taken. See rust-random#559 (comment) for some benchmark results. This spawned a bunch of improvements and fixes in `wasm-bindgen` that are being used here: - wasm-bindgen/wasm-bindgen#4315 - wasm-bindgen/wasm-bindgen#4316 - wasm-bindgen/wasm-bindgen#4318 - wasm-bindgen/wasm-bindgen#4319 - wasm-bindgen/wasm-bindgen#4340
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds support for numeric slice types with
MaybeUninit<T>. While, generally in Rust, unsafety requirements have to be kept by the user, this isn't unsafe in Wasm to begin with, so there is no concern in this regard.The use case here is interfacing with cross-platform Rust code where using this type probably made much more sense.