Skip to content

wasm_js: remove the separate codepath for Node.js and TLS caching - #557

Merged
newpavlov merged 17 commits into
masterfrom
wasm_js/simplify
Dec 4, 2024
Merged

wasm_js: remove the separate codepath for Node.js and TLS caching#557
newpavlov merged 17 commits into
masterfrom
wasm_js/simplify

Conversation

@newpavlov

@newpavlovnewpavlov commented Dec 4, 2024•
edited
Loading

Copy link
Copy Markdown
Member

Acquire of "global" objects is relatively cheap (adds ~40 ns of overhead), so it make sense to do it on each call instead of trying to cache objects in a TLS variable.

Additionally, removes the separate codepath for Node.js since it fully supports the Web Crypto API since v19 (released 2022-10-18).

@newpavlovnewpavlov mentioned this pull request Dec 4, 2024
Closed
@newpavlovnewpavlov changed the title wasm_js: remove TLSwasm_js: remove TLS and the separate codepath for Node.jsDec 4, 2024
@newpavlov
newpavlov marked this pull request as ready for review December 4, 2024 22:10
@newpavlov

newpavlov commented Dec 4, 2024

Copy link
Copy Markdown
MemberAuthor

@daxpedda
Can you take a look? If you agree with the changes and have no suggestions, I will merge it.

I think we can leave additional optimization suggested by you here for a separate PR since I do not fully understand how to implement them (I am not very familiar with wasm-bindgen).

@newpavlovnewpavlov mentioned this pull request Dec 4, 2024
Closed
@daxpedda

daxpedda commented Dec 4, 2024

Copy link
Copy Markdown
Contributor

Ah, I thought you are going to merge #541 first.
Sure, I will take a look.

@newpavlov

newpavlov commented Dec 4, 2024

Copy link
Copy Markdown
MemberAuthor

I thought that when you wrote "Sure, I'm a fan of splitting things up." you meant that we should continue with this PR. :)

@newpavlovnewpavlov changed the title wasm_js: remove TLS and the separate codepath for Node.jswasm_js: remove the separate codepath for Node.js and TLS cachingDec 4, 2024
daxpedda
daxpedda suggested changes Dec 4, 2024
View reviewed changes

@daxpeddadaxpedda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There are also some Node.js related leftovers on the Error type.

I thought that when you wrote "Sure, I'm a fan of splitting things up." you meant that we should continue with this PR. :)

Apologies, I probably didn't read carefully enough. I understood it just as a follow-up, not a replacement.

Comment threadREADME.md Outdated
Comment threadsrc/backends/wasm_js.rs Outdated
Comment threadsrc/backends/wasm_js.rs Outdated
Comment threadsrc/backends/wasm_js.rs
daxpedda
daxpedda approved these changes Dec 4, 2024
View reviewed changes
Comment threadREADME.md
daxpedda
daxpedda reviewed Dec 4, 2024
View reviewed changes
Comment threadsrc/backends/wasm_js.rs Outdated
newpavlovand others added 2 commits December 5, 2024 02:27
@newpavlov@daxpedda
Update src/backends/wasm_js.rs
27d9dc0
Co-authored-by: daxpedda <daxpedda@gmail.com>
@newpavlov
Remove outdated links
20827a7
@newpavlov
newpavlov merged commit 5fbfd2e into masterDec 4, 2024
@newpavlov
newpavlov deleted the wasm_js/simplify branch December 4, 2024 23:49
@daxpedda

daxpedda commented Dec 5, 2024

Copy link
Copy Markdown
Contributor

We entirely forgot the whole no_std support.
Will make a PR for that.

@daxpeddadaxpedda mentioned this pull request Dec 5, 2024
Merged
newpavlov pushed a commit that referenced this pull request Dec 5, 2024
@daxpedda
Addno_stdsupport for Web (#560)
ae0c807
In #557 we forgot to port the `no_std` support from #541.
This also adds running Clippy for `wasm32v1-none`.
@dhardydhardy mentioned this pull request May 29, 2025
Closed
takumi-earth pushed a commit to earthlings-dev/getrandom that referenced this pull request Jan 27, 2026
@newpavlov
wasm_js: remove the separate codepath for Node.js and TLS caching (ru…
c350048
…st-random#557)
Acquire of "global" objects is relatively cheap (adds ~40 ns of
overhead), so it make sense to do it on each call instead of trying to
cache objects in a TLS variable.
Additionally, removes the separate codepath for Node.js since it fully
supports the Web Crypto API since v19 (released 2022-10-18).
takumi-earth pushed a commit to earthlings-dev/getrandom that referenced this pull request Jan 27, 2026
@daxpedda
Addno_stdsupport for Web (rust-random#560)
e51cab4
In rust-random#557 we forgot to port the `no_std` support from rust-random#541.
This also adds running Clippy for `wasm32v1-none`.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@daxpeddadaxpeddadaxpedda 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

@newpavlov@daxpedda