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.

Sign up for GitHub

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

Web optimizations #559

Merged
merged 12 commits into from
Dec 9, 2024
Merged

Web optimizations #559

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1dbda17
Cache global `Crypto` object
daxpedda Dec 4, 2024
d804258
Don't create sub-array unless necessary
daxpedda Dec 6, 2024
b2ab9a1
Don't use `Uint8Array` when not necessary
daxpedda Dec 5, 2024
02507fe
Improve CI test matrix
daxpedda Dec 7, 2024
dcd8f4a
Remove nesting
daxpedda Dec 7, 2024
e024029
Remove unnecessary cloning
daxpedda Dec 7, 2024
6feb9b7
Guard `js-sys` dependency behind `target_feature = "atomics"`
daxpedda Dec 7, 2024
65ba727
Fix `MAX_BUFFER_SIZE`
daxpedda Dec 8, 2024
055a7c2
Remove all `as` casts
daxpedda Dec 8, 2024
ec6f289
Use `globalThis` to access `Crypto`
daxpedda Dec 8, 2024
cf72d6c
Remove the `js_sys::Uint8Array` import
daxpedda Dec 9, 2024
e5d68a9
Move check for global `Crypto` object to `getRandomValues`
daxpedda Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 34 additions & 14 deletions .github/workflows/tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,11 +229,31 @@ jobs:
# run: cargo test

web:
name: Web
name: ${{ matrix.rust.description }}
runs-on: ubuntu-24.04
strategy:
fail-fast: false
matrix:
rust:
- {
description: Web,
version: stable,
flags: -Dwarnings --cfg getrandom_backend="wasm_js",
args: --features=std,
}
- {
description: Web with Atomics,
version: nightly,
components: rust-src,
flags: '-Dwarnings --cfg getrandom_backend="wasm_js" -Ctarget-feature=+atomics,+bulk-memory',
args: '--features=std -Zbuild-std=panic_abort,std',
}
steps:
- uses: actions/checkout@v4
- uses: dtolnay/rust-toolchain@stable
- uses: dtolnay/rust-toolchain@master
with:
toolchain: ${{ matrix.rust.version }}
components: ${{ matrix.rust.components }}
- name: Install precompiled wasm-pack
shell: bash
run: |
Expand All @@ -244,34 +264,34 @@ jobs:
- uses: Swatinem/rust-cache@v2
- name: Test (Node)
env:
RUSTFLAGS: -Dwarnings --cfg getrandom_backend="wasm_js"
run: wasm-pack test --node -- --features std
RUSTFLAGS: ${{ matrix.rust.flags }}
run: wasm-pack test --node -- ${{ matrix.rust.args }}
- name: Test (Firefox)
env:
WASM_BINDGEN_USE_BROWSER: 1
RUSTFLAGS: -Dwarnings --cfg getrandom_backend="wasm_js"
run: wasm-pack test --headless --firefox -- --features std
RUSTFLAGS: ${{ matrix.rust.flags }}
run: wasm-pack test --headless --firefox -- ${{ matrix.rust.args }}
- name: Test (Chrome)
env:
WASM_BINDGEN_USE_BROWSER: 1
RUSTFLAGS: -Dwarnings --cfg getrandom_backend="wasm_js"
run: wasm-pack test --headless --chrome -- --features std
RUSTFLAGS: ${{ matrix.rust.flags }}
run: wasm-pack test --headless --chrome -- ${{ matrix.rust.args }}
- name: Test (dedicated worker)
env:
WASM_BINDGEN_USE_DEDICATED_WORKER: 1
RUSTFLAGS: -Dwarnings --cfg getrandom_backend="wasm_js"
run: wasm-pack test --headless --firefox -- --features std
RUSTFLAGS: ${{ matrix.rust.flags }}
run: wasm-pack test --headless --firefox -- ${{ matrix.rust.args }}
- name: Test (shared worker)
env:
WASM_BINDGEN_USE_SHARED_WORKER: 1
RUSTFLAGS: -Dwarnings --cfg getrandom_backend="wasm_js"
run: wasm-pack test --headless --firefox -- --features std
RUSTFLAGS: ${{ matrix.rust.flags }}
run: wasm-pack test --headless --firefox -- ${{ matrix.rust.args }}
- name: Test (service worker)
env:
WASM_BINDGEN_USE_SERVICE_WORKER: 1
RUSTFLAGS: -Dwarnings --cfg getrandom_backend="wasm_js"
RUSTFLAGS: ${{ matrix.rust.flags }}
# Firefox doesn't support module service workers and therefor can't import scripts
run: wasm-pack test --headless --chrome -- --features std
run: wasm-pack test --headless --chrome -- ${{ matrix.rust.args }}

wasi:
name: WASI
Expand Down
4 changes: 4 additions & 0 deletions .github/workflows/workspace.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ jobs:
env:
RUSTFLAGS: -Dwarnings --cfg getrandom_backend="wasm_js"
run: cargo clippy -Zbuild-std --target wasm32-unknown-unknown
- name: Web WASM with atomics (wasm_js.rs)
env:
RUSTFLAGS: -Dwarnings --cfg getrandom_backend="wasm_js" -Ctarget-feature=+atomics,+bulk-memory
josephlr marked this conversation as resolved.
Show resolved Hide resolved
run: cargo clippy -Zbuild-std --target wasm32-unknown-unknown
- name: Linux (linux_android.rs)
env:
RUSTFLAGS: -Dwarnings --cfg getrandom_backend="linux_getrandom"
Expand Down
5 changes: 3 additions & 2 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,9 @@ windows-targets = "0.52"

# wasm_js
[target.'cfg(all(getrandom_backend = "wasm_js", target_arch = "wasm32", any(target_os = "unknown", target_os = "none")))'.dependencies]
wasm-bindgen = { version = "0.2.96", default-features = false }
js-sys = { version = "0.3.73", default-features = false }
wasm-bindgen = { version = "0.2.98", default-features = false }
[target.'cfg(all(getrandom_backend = "wasm_js", target_arch = "wasm32", any(target_os = "unknown", target_os = "none"), target_feature = "atomics"))'.dependencies]
js-sys = { version = "0.3.75", default-features = false }
daxpedda marked this conversation as resolved.
Show resolved Hide resolved
[target.'cfg(all(getrandom_backend = "wasm_js", target_arch = "wasm32", any(target_os = "unknown", target_os = "none")))'.dev-dependencies]
wasm-bindgen-test = "0.3"

Expand Down
60 changes: 41 additions & 19 deletions src/backends/wasm_js.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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!");

use js_sys::{global, Uint8Array};
use wasm_bindgen::{prelude::wasm_bindgen, JsCast, JsValue};
#[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};

// Size of our temporary Uint8Array buffer used with WebCrypto methods
// Maximum is 65536 bytes see https://developer.mozilla.org/en-US/docs/Web/API/Crypto/getRandomValues
const CRYPTO_BUFFER_SIZE: u16 = 256;
// 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> {
let global: Global = global().unchecked_into();
let crypto = global.crypto();
CRYPTO.with(|crypto| {
let crypto = crypto.as_ref().ok_or(Error::WEB_CRYPTO)?;
inner(crypto, dest)
})
}

if !crypto.is_object() {
return Err(Error::WEB_CRYPTO);
#[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 = Uint8Array::new_with_length(CRYPTO_BUFFER_SIZE.into());
for chunk in dest.chunks_mut(CRYPTO_BUFFER_SIZE.into()) {
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 CRYPTO_BUFFER_SIZE");
.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 = buf.subarray(0, chunk_len);
let sub_buf = if chunk_len == buf_len {
&buf
} else {
&buf.subarray(0, chunk_len)
};

if crypto.get_random_values(&sub_buf).is_err() {
if crypto.get_random_values(sub_buf).is_err() {
return Err(Error::WEB_GET_RANDOM_VALUES);
}

Expand All @@ -46,14 +66,16 @@ pub fn fill_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {

#[wasm_bindgen]
extern "C" {
// Return type of js_sys::global()
type Global;
// Web Crypto API: Crypto interface (https://www.w3.org/TR/WebCryptoAPI/)
type Crypto;
// Getters for the Crypto API
#[wasm_bindgen(method, getter)]
fn crypto(this: &Global) -> Crypto;
// Holds the global `Crypto` object.
#[wasm_bindgen(thread_local_v2, js_name = crypto)]
Copy link
Contributor Author

@daxpedda daxpedda Dec 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I initially avoided using globalThis here because I have no idea how it works exactly on Node.js. In the meantime I have looked up the documentation and asked around: it should be perfectly fine to use globalThis in Node.js.

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 crypto can't be overwritten. So both approaches, accessing Crypto through the global crypto variable or globalThis.crypto, will work the same.

But to stick with "how it should be done" I will add globalThis now.

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)]
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using methods, statics, and thread_local_v2, can we use js_namespace like:

// 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 js_namespace should be crypto or ["globalThis", "crypto"])).

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>()) };
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are forfeiting the check if Crypto is available though.
But if your intention was just to get rid of the nesting, how about prefacing the function like this:

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, getrandom is very foundational and should support these imo. And CRYPTO is cached after all so there is no performance overhead.

Copy link
Member

Choose a reason for hiding this comment

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

Checking if the global crypto object exists doesn't really change what platforms/environments we support. It just changes what error message we get when the call to crypto.getRandomValues fails. Before, we were checking for existence of the object as part of our platform detection (and to cache the value in RngSource). Now that we don't have two different implementations for Web vs Node, we don't need to do any of that.

So if I'm understanding things correctly, having the check for the existence of the object just lets us return Error::WEB_CRYPTO to the user, informing them that "Web Crypto API is unavailable" instead of my implementation above which would make us return Error::WEB_GET_RANDOM_VALUES in all places.

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 is_none is cached).

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).

Copy link
Contributor Author

@daxpedda daxpedda Dec 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Checking if the global crypto object exists doesn't really change what platforms/environments we support. It just changes what error message we get when the call to crypto.getRandomValues fails.

Ah, you are relying on the fact that WBG just catches any exception when tagging it with catch. While this is currently the case, its not intended to be used like this. Ideally, this would only catch exceptions thrown by the function call. However, I guess this behavior has to stay in place because otherwise it would be a breaking change.

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).

Sure!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, let me know if you like this better!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 thread_local_v2?

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 Crypto. So this needs to be in a thread-local to work correctly.

Copy link
Member

@newpavlov newpavlov Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I am not familiar with the wasm-bindgen inner workings, but wouldn't js_namespace-based code result in a lighter-weight WASM file? Plus, the resulting source code will be slightly smaller, assuming that we do not care about the WEB_CRYPTO error. If we want to keep it, then I think the current code is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not familiar with the wasm-bindgen inner workings, but wouldn't js_namespace-based code result in a lighter-weight WASM file?

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.

Plus, the resulting source code will be slightly smaller (assuming that we do not care about the WEB_CRYPTO error).

If you don't care about the WEB_CRYPTO error yes, then we can simply go with the original proposal by @josephlr. I will do some digging into the specification and actually find out if Crypto.getRandomValues() can realistically fail or not actually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, that fast: according to the specification, Crypto.getRandomValues() can only fail if we don't supply the right buffer type or we exceed the maximum byte length of 65536. So instead of getting rid of WEB_CRYPTO I will go ahead and get rid of WEB_GET_RANDOM_VALUES.

Let me know what you think!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 8cf6832.

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>;
}