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

Save the project #106

Merged
merged 3 commits into from
Sep 29, 2017
Merged

Save the project #106

merged 3 commits into from
Sep 29, 2017

Conversation

buu700
Copy link
Contributor

@buu700 buu700 commented Sep 29, 2017

Addresses #105.

I haven't tested any part of this, and as mentioned it'll depend on emscripten-core/emscripten#5296 landing, but this is what I was going for:

  • Consolidate the whole build back into one file (with no run-time dependencies on external subresources)

  • Preferentially use -O3 WebAssembly, with -Oz asm.js as a fallback for when wasm is unsupported or fails to compile

  • Strip use asm from the fallback asm.js build to avoid unneeded AOT compilation; this means that more aggressive minification can be used, but also sucks for clients that support asm.js but not wasm

  • Expose a sodium.ready promise to let consuming code know when the emscripten module is initialized and methods are ready to be used

Any thoughts on the general direction / design goals?

@jedisct1
Copy link
Owner

This is fantastic.

I'm just a bit confused about why you recommend stripping use asm.
Why is it a good thing to avoid AOT compilation here? Doesn't removing it have an overall performance impact?

@buu700
Copy link
Contributor Author

buu700 commented Sep 29, 2017

Thanks!

The reason I stripped use asm is that browsers would otherwise block the thread to AOT compile the asm.js fallback even when WebAssembly is supported. (This was an issue pointed out by @kripken during the initial planning of that emscripten PR.)

@jedisct1
Copy link
Owner

I see. Blocking the thread for something that may not be useful is not great, but if we don't, we also may become more vulnerable to timing attacks.

@buu700
Copy link
Contributor Author

buu700 commented Sep 29, 2017

Yeah, that's really the biggest concern I see with this, more so than the degraded asm.js performance. In my other libraries that use this pattern (mceliece.js, ntru.js, rlwe.js, sidh.js, sphincs.js, and xkcd-passphrase), the reason I ended up deciding in favor of startup time + space-efficiency* over the increased timing attack risk is the vanishingly small number of clients that support asm.js and not WebAssembly (with browsers these days being mostly evergreen).

Specifically, the following clients would be impacted:

  • Chrome 28 - 56 (to the extent that Chrome ever really supported asm.js)

  • Edge 13 - 15

  • Firefox 22 - 51

  • Some range of Node.js versions (but starting next month wasm support will be in Node LTS)

*: In that not caring about strict asm.js compliance allows aggressive minification of the entire JS output.

@jedisct1 jedisct1 merged commit f8475cb into jedisct1:master Sep 29, 2017
@jedisct1
Copy link
Owner

Merged into a buu branch since a couple changes seem to be required in order for the distfiles to be built.

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