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.

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

Micro-optimize randomFill. NFC #23309

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jan 6, 2025

Only use the node fallback code when its actually needed, otherwise assume that crypto.getRandomValues is available out-of-the-box (which it is on node v19 and above.

@sbc100 sbc100 requested review from dschuff and kripken January 6, 2025 18:28
@sbc100 sbc100 force-pushed the optimize_randomFill branch 3 times, most recently from 47cfa63 to 8c31063 Compare January 6, 2025 19:14
src/library_wasi.js Outdated Show resolved Hide resolved
src/library_wasi.js Outdated Show resolved Hide resolved
@sbc100 sbc100 force-pushed the optimize_randomFill branch 2 times, most recently from e441d89 to 7e556bf Compare January 6, 2025 22:03
@sbc100 sbc100 enabled auto-merge (squash) January 6, 2025 22:32
@sbc100 sbc100 force-pushed the optimize_randomFill branch from 7e556bf to 91223c1 Compare January 6, 2025 23:40
@sbc100
Copy link
Collaborator Author

sbc100 commented Jan 6, 2025

Interrestingly enough it seems that the builtin crypto that comes with node v19 and above is not the same is that one you get when you require('crypto)'. Specifically crypto.randomFillSync only exists in the later!

var mycrypto = require('node:crypto');

console.log(crypto.randomFillSync);  // undefined
console.log(mycrypto.randomFillSync);  // defined!

src/library_wasi.js Outdated Show resolved Hide resolved
src/library_wasi.js Outdated Show resolved Hide resolved
@sbc100 sbc100 force-pushed the optimize_randomFill branch from 91223c1 to bddfb23 Compare January 7, 2025 00:19
a.out.js Outdated Show resolved Hide resolved
tests.txt Outdated Show resolved Hide resolved
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Oh, I see there is automerge, so removing lgtm. but lgtm with the unneeded files removed

@sbc100 sbc100 force-pushed the optimize_randomFill branch from bddfb23 to 61bcbd4 Compare January 7, 2025 00:25
Only use the node fallback code when its actually needed, otherwise
assume that `crypto.getRandomValues` is available out-of-the-box (which
it is on node v19 and above.
@sbc100 sbc100 force-pushed the optimize_randomFill branch from 61bcbd4 to 763f700 Compare January 7, 2025 00:31
@sbc100 sbc100 merged commit 229bc6d into emscripten-core:main Jan 7, 2025
29 checks passed
@sbc100 sbc100 deleted the optimize_randomFill branch January 7, 2025 01:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants