Web optimizations #559
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,34 +7,54 @@ pub use crate::util::{inner_u32, inner_u64}; | |
| #[cfg(not(all(target_arch = "wasm32", any(target_os = "unknown", target_os = "none"))))] | ||
| compile_error!("`wasm_js` backend can be enabled only for OS-less WASM targets!"); | ||
|
|
||
| #[cfg(target_feature = "atomics")] | ||
| use js_sys::Uint8Array; | ||
newpavlov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| use wasm_bindgen::{prelude::wasm_bindgen, JsValue}; | ||
|
|
||
| // Maximum buffer size allowed in `Crypto.getRandomValuesSize` is 65536 bytes. | ||
| // See https://developer.mozilla.org/en-US/docs/Web/API/Crypto/getRandomValues | ||
| const MAX_BUFFER_SIZE: u32 = 65536; | ||
daxpedda marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| pub fn fill_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> { | ||
| CRYPTO.with(|crypto| { | ||
| let crypto = crypto.as_ref().ok_or(Error::WEB_CRYPTO)?; | ||
| inner(crypto, dest) | ||
| }) | ||
| } | ||
|
|
||
| #[cfg(not(target_feature = "atomics"))] | ||
| fn inner(crypto: &Crypto, dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> { | ||
| for chunk in dest.chunks_mut(MAX_BUFFER_SIZE as usize) { | ||
| if crypto.get_random_values(chunk).is_err() { | ||
| return Err(Error::WEB_GET_RANDOM_VALUES); | ||
| } | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[cfg(target_feature = "atomics")] | ||
| fn inner(crypto: &Crypto, dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> { | ||
| // getRandomValues does not work with all types of WASM memory, | ||
| // so we initially write to browser memory to avoid exceptions. | ||
| let buf_len = u32::min( | ||
| dest.len().try_into().unwrap_or(MAX_BUFFER_SIZE), | ||
| MAX_BUFFER_SIZE, | ||
| ); | ||
| let buf = Uint8Array::new_with_length(buf_len); | ||
| for chunk in dest.chunks_mut(buf_len as usize) { | ||
| let chunk_len: u32 = chunk | ||
| .len() | ||
| .try_into() | ||
| .expect("chunk length is bounded by MAX_BUFFER_SIZE"); | ||
| // The chunk can be smaller than buf's length, so we call to | ||
| // JS to create a smaller view of buf without allocation. | ||
| let sub_buf = if chunk_len == buf_len { | ||
| &buf | ||
| } else { | ||
| &buf.subarray(0, chunk_len) | ||
| }; | ||
|
|
||
| if crypto.get_random_values(sub_buf).is_err() { | ||
| return Err(Error::WEB_GET_RANDOM_VALUES); | ||
| } | ||
|
|
||
|
|
@@ -46,14 +66,16 @@ pub fn fill_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> { | |
|
|
||
| #[wasm_bindgen] | ||
| extern "C" { | ||
| // Web Crypto API: Crypto interface (https://www.w3.org/TR/WebCryptoAPI/) | ||
| type Crypto; | ||
| // Holds the global `Crypto` object. | ||
| #[wasm_bindgen(thread_local_v2, js_name = crypto)] | ||
|
There was a problem hiding this comment. I initially avoided using For context: the current setup just accesses the object directly from the global scope. According to the spec this is perfectly fine. However, this is usually discouraged because global variables can simply be overwritten. While I'm not sure if there is more history behind this, fact is that read-only properties like But to stick with "how it should be done" I will add EDIT: Support should also not be a concern after #554, but you can make your own evaluations via "Can I Use". |
||
| static CRYPTO: Option<Crypto>; | ||
| // Crypto.getRandomValues() | ||
| #[cfg(not(target_feature = "atomics"))] | ||
| #[wasm_bindgen(method, js_name = getRandomValues, catch)] | ||
|
There was a problem hiding this comment. Instead of using methods, // Web Crypto API bindings (https://www.w3.org/TR/WebCryptoAPI/)
#[wasm_bindgen(js_namespace = crypto)]
extern "C" {
#[cfg(not(target_feature = "atomics"))]
#[wasm_bindgen(js_name = getRandomValues, catch)]
fn get_random_values(buf: &mut [MaybeUninit<u8>]) -> Result<(), JsValue>;
#[cfg(target_feature = "atomics")]
#[wasm_bindgen(js_name = getRandomValues, catch)]
fn get_random_values(buf: &Uint8Array) -> Result<(), JsValue>;
}(I'm not sure if Then the code can just be: // If atomics are not enabled, we assume that Rust's memory is not backed by a
// SharedArrayBuffer, so we can directly call getRandomValues() on the buffer.
#[cfg(not(target_feature = "atomics"))]
pub fn fill_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {
// The byteLength of the input must not exceed 65536 bytes, see:
// https://developer.mozilla.org/en-US/docs/Web/API/Crypto/getRandomValues
const MAX_LENGTH: usize = 65536;
for chunk in dest.chunks_mut(MAX_LENGTH) {
if crypto.get_random_values(chunk).is_err() {
return Err(Error::WEB_GET_RANDOM_VALUES);
}
}
Ok(())
}
// If atomics are enabled, Rust's memory can be backed by a SharedArrayBuffer,
// so we use a temporary Uint8Array buffer when calling getRandomValues().
#[cfg(target_feature = "atomics")]
pub fn fill_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {
// We are rarely called with long slices, so we allocate a small buffer.
const BUF_LEN: u16 = 256; // Less than MAX_LENGTH
let tmp = js_sys::Uint8Array::new_with_length(BUF_LEN.into());
for chunk in dest.chunks_mut(BUF_LEN.into()) {
// The chunk can be smaller than tmp's length, so we call to
// JS to create a smaller view of buf without allocation.
let buf = if chunk.len() == BUF_LEN.into() {
tmp
} else {
let len: u32 = chunk.len().try_into().expect("chunk.len() <= BUF_LEN <= u32::MAX");
tmp.subarray(0, len)
}
if crypto.get_random_values(&buf).is_err() {
return Err(Error::WEB_GET_RANDOM_VALUES);
}
// SAFETY: buf's length is equal the chunk's length.
unsafe { buf.raw_copy_to_ptr(chunk.as_mut_ptr().cast::<u8>()) };
}
}There was a problem hiding this comment. You are forfeiting the check if if CRYPTO.with(Option::is_none) {
return Err(Error::WEB_CRYPTO)
}
// use `get_random_values()` without nesting:
get_random_values(&buf);Let me know how you want to proceed. My recommendation is to keep the check in place, there's all sort of strange JS environments out there, There was a problem hiding this comment. Checking if the global So if I'm understanding things correctly, having the check for the existence of the object just lets us return I think returning good error messages is valuable, so I think keeping some sort of check is OK, provided that we aren't making an additional FFI call every time (which I don't think we do because the result of I personally find the un-nested version easier to read, so we might try a version that avoids too much nesting (potentially by using a helper function). There was a problem hiding this comment.
Ah, you are relying on the fact that WBG just catches any exception when tagging it with
Sure! There was a problem hiding this comment. Alright, let me know if you like this better! There was a problem hiding this comment. Ah, I see! From my understanding the issue was getting rid of the nesting, but that has been achieved, right? Is there some other motivation to get rid of On that note: your proposal will store the check globally. But different threads may have different outcomes. This isn't hypothetical either, some worklets don't have access to There was a problem hiding this comment. I am not familiar with the There was a problem hiding this comment.
Yes, we have one FFI function less present in the Wasm file. To put it into perspective: the function that actually ends up in the Wasm file after post-processing is done is just a single FFI signature, one byte cast and an if condition. Not counting the storing and loading ofc.
If you don't care about the There was a problem hiding this comment. Okay, that fast: according to the specification, Let me know what you think! |
||
| fn get_random_values(this: &Crypto, buf: &mut [MaybeUninit<u8>]) -> Result<(), JsValue>; | ||
| #[cfg(target_feature = "atomics")] | ||
| #[wasm_bindgen(method, js_name = getRandomValues, catch)] | ||
| fn get_random_values(this: &Crypto, buf: &Uint8Array) -> Result<(), JsValue>; | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.