Skip to content

Web optimizations #559

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

Merged
newpavlov merged 12 commits into from
Dec 9, 2024
Merged

Web optimizations #559

Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
Next Next commit
Don't use Uint8Array when not necessary
  • Loading branch information
@daxpedda
daxpedda committed Dec 7, 2024
commit b2ab9a1a20bcf4c880d4355ed700bbafb43966c9
43 changes: 29 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,26 @@ jobs:
# run: cargo test

web:
name: Web
name: Web ${{ matrix.rust.description }}
runs-on: ubuntu-24.04
strategy:
fail-fast: false
matrix:
rust:
- { version: stable }
- {
description: with Atomics,
version: nightly,
components: rust-src,
flags: '-Ctarget-feature=+atomics,+bulk-memory',
args: '-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 +259,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: -Dwarnings --cfg getrandom_backend="wasm_js" ${{ matrix.rust.flags }}
run: wasm-pack test --node -- --features std ${{ 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: -Dwarnings --cfg getrandom_backend="wasm_js" ${{ matrix.rust.flags }}
run: wasm-pack test --headless --firefox -- --features std ${{ 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: -Dwarnings --cfg getrandom_backend="wasm_js" ${{ matrix.rust.flags }}
run: wasm-pack test --headless --chrome -- --features std ${{ 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: -Dwarnings --cfg getrandom_backend="wasm_js" ${{ matrix.rust.flags }}
run: wasm-pack test --headless --firefox -- --features std ${{ 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: -Dwarnings --cfg getrandom_backend="wasm_js" ${{ matrix.rust.flags }}
run: wasm-pack test --headless --firefox -- --features std ${{ matrix.rust.args }}
- name: Test (service worker)
env:
WASM_BINDGEN_USE_SERVICE_WORKER: 1
RUSTFLAGS: -Dwarnings --cfg getrandom_backend="wasm_js"
RUSTFLAGS: -Dwarnings --cfg getrandom_backend="wasm_js" ${{ 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 -- --features std ${{ 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
run: cargo clippy -Zbuild-std --target wasm32-unknown-unknown
- name: Linux (linux_android.rs)
env:
RUSTFLAGS: -Dwarnings --cfg getrandom_backend="linux_getrandom"
Expand Down
62 changes: 39 additions & 23 deletions src/backends/wasm_js.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,39 +7,51 @@ 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;
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: u16 = 256;

pub fn fill_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {
CRYPTO.with(|crypto| {
let crypto = crypto.as_ref().ok_or(Error::WEB_CRYPTO)?;

// 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 chunk_len: u32 = chunk
.len()
.try_into()
.expect("chunk length is bounded by CRYPTO_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 == u32::from(CRYPTO_BUFFER_SIZE) {
buf.clone()
} else {
buf.subarray(0, chunk_len)
};

if crypto.get_random_values(&sub_buf).is_err() {
return Err(Error::WEB_GET_RANDOM_VALUES);
#[cfg(not(target_feature = "atomics"))]
{
for chunk in dest.chunks_mut(MAX_BUFFER_SIZE.into()) {
if crypto.get_random_values(chunk).is_err() {
return Err(Error::WEB_GET_RANDOM_VALUES);
}
}
}
#[cfg(target_feature = "atomics")]
{
// 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(MAX_BUFFER_SIZE.into());
for chunk in dest.chunks_mut(MAX_BUFFER_SIZE.into()) {
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 == u32::from(MAX_BUFFER_SIZE) {
buf.clone()
} else {
buf.subarray(0, chunk_len)
};

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

// SAFETY: `sub_buf`'s length is the same length as `chunk`
unsafe { sub_buf.raw_copy_to_ptr(chunk.as_mut_ptr().cast::<u8>()) };
// SAFETY: `sub_buf`'s length is the same length as `chunk`
unsafe { sub_buf.raw_copy_to_ptr(chunk.as_mut_ptr().cast::<u8>()) };
}
}
Ok(())
})
Expand All @@ -53,6 +65,10 @@ extern "C" {
#[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

@josephlr josephlr Dec 7, 2024

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

@daxpedda daxpedda Dec 7, 2024

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

@josephlr josephlr Dec 7, 2024

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

@daxpedda daxpedda Dec 7, 2024

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

@daxpedda daxpedda Dec 9, 2024

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

@daxpedda daxpedda Dec 9, 2024

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

@daxpedda daxpedda Dec 9, 2024

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

@daxpedda daxpedda Dec 9, 2024

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