Skip to content

Support undefined static imports with Option#4319

Merged
daxpedda merged 6 commits intowasm-bindgen:mainfrom
daxpedda:static-option
Dec 6, 2024
Merged

Support undefined static imports with Option#4319
daxpedda merged 6 commits intowasm-bindgen:mainfrom
daxpedda:static-option

Conversation

@daxpedda
Copy link
Copy Markdown
Member

@daxpedda daxpedda commented Dec 5, 2024 •
edited
Loading 

extern "C" {
    type Crypto;
    #[wasm_bindgen(thread_local_v2, js_name = crypto)]
    static CRYPTO: Option<Crypto>;
}

This currently throws an error in JS if crypto is undefined.

This change would set CRYPTO to None instead.

@daxpedda daxpedda mentioned this pull request Dec 5, 2024
Merged
@RunDevelopment
Copy link
Copy Markdown
Contributor

RunDevelopment commented Dec 5, 2024

The title of this PR doesn't make sense to me. If the global crypto variable was set to undefined, then wouldn't the imported value be JsValue::UNDEFINED (or whatever the constant is called)?

I think what you really mean is that it now supports undeclared variables, right? So for your example, crypto might not be defined by the browser/runtime, so just accessing it might give a ReferenceError at runtime. If so, then this is very different from the global variable being set to undefined. Please change the changelog entry if that's what you meant.

Also, please document this feature. Its behavior is not at all obvious. E.g. WBG doesn't support Option<JsValue> in functions at all, so it's surprising that it's allowed here. Globally defined variables set to undefined or null are also handled differently from just static FOO: JsValue.

@daxpedda
Copy link
Copy Markdown
Member Author

daxpedda commented Dec 5, 2024 •
edited
Loading

The title of this PR doesn't make sense to me. If the global crypto variable was set to undefined, then wouldn't the imported value be JsValue::UNDEFINED (or whatever the constant is called)?

It would not, that's why Option<JsValue> isn't supported in the first place. I fixed OP.

I think what you really mean is that it now supports undeclared variables, right? So for your example, crypto might not be defined by the browser/runtime, so just accessing it might give a ReferenceError at runtime. If so, then this is very different from the global variable being set to undefined. Please change the changelog entry if that's what you meant.

Right, the distinction between undefined and undeclared wasn't quite clear to me. It should be noted that in either case the output would be None.

Also, please document this feature. Its behavior is not at all obvious. E.g. WBG doesn't support Option<JsValue> in functions at all, so it's surprising that it's allowed here. Globally defined variables set to undefined or null are also handled differently from just static FOO: JsValue.

I might consider expanding this feature to cover JsValue as well, that would make sense.

@daxpedda
Copy link
Copy Markdown
Member Author

daxpedda commented Dec 5, 2024

Also, please document this feature. Its behavior is not at all obvious. E.g. WBG doesn't support Option<JsValue> in functions at all, so it's surprising that it's allowed here. Globally defined variables set to undefined or null are also handled differently from just static FOO: JsValue.

I might consider expanding this feature to cover JsValue as well, that would make sense.

Or not actually? As you said, this feature is not about undefined, its about undeclared.

@daxpedda
Copy link
Copy Markdown
Member Author

daxpedda commented Dec 5, 2024

Adjusted and added documentation.

So to be clear: this PR does not add support for Option<JsValue>!

RunDevelopment
RunDevelopment reviewed Dec 5, 2024
View reviewed changes
RunDevelopment
RunDevelopment reviewed Dec 6, 2024
View reviewed changes
RunDevelopment
RunDevelopment reviewed Dec 6, 2024
View reviewed changes
daxpedda and others added 2 commits December 6, 2024 16:55
@daxpedda @RunDevelopment
Adjust uncommon JS style
1f81bc1 
Co-Authored-By: Michael Schmidt <msrd0000@gmail.com>
@daxpedda @RunDevelopment
Improve documentation
2b5ebdc 
Co-Authored-By: Michael Schmidt <msrd0000@gmail.com>
RunDevelopment
RunDevelopment approved these changes Dec 6, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you @daxpedda!

@daxpedda daxpedda merged commit 89f2af8 into wasm-bindgen:main Dec 6, 2024
@daxpedda daxpedda mentioned this pull request Dec 7, 2024
Merged
newpavlov pushed a commit to rust-random/getrandom that referenced this pull request Dec 9, 2024
@daxpedda
Web optimizations (#559)
30a7f1f 
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
@daxpedda
Web optimizations (rust-random#559)
e2574ba 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@RunDevelopment RunDevelopment RunDevelopment approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@daxpedda @RunDevelopment