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

Fixes curve25519_dalek_bits defaults for cross and wasm #465

Conversation

pinkforest
Copy link
Contributor

@pinkforest pinkforest commented Dec 9, 2022

As discussed in #456 this sets well known defaults for:
cfg(target_family = "wasm") and cfg(target_arch = "arm")

By setting the 64 bit serial arithmetric via cfg(curve25519_dalek_bits = "64")

EDIT: I've used platforms crate via env TARGET to determine the actual cross target and this now fixes build.rs only.

@pinkforest pinkforest changed the title Set well known defaults for curve25519_dalek_bits Set well known defaults for cfg(target_family = "wasm") and cfg(target_arch = "arm") Dec 9, 2022
@pinkforest pinkforest changed the title Set well known defaults for cfg(target_family = "wasm") and cfg(target_arch = "arm") Set well known defaults for wasm and arm Dec 9, 2022
@pinkforest pinkforest force-pushed the feat-wasm-armv7-default-u64-serial branch 2 times, most recently from f9f4cc2 to 826a532 Compare December 9, 2022 11:00
@rozbb
Copy link
Contributor

rozbb commented Dec 9, 2022

Awesome! One thing: could we have documentation somewhere (can even be in the comments of build.rs) that explains what's going on and why? A link to something about WASM benchmarks would be nice to include too if you have it

@pinkforest
Copy link
Contributor Author

I'm fixing the build script for cross.. this is WIP for an hour or so. sorry

@tarcieri
Copy link
Contributor

tarcieri commented Dec 9, 2022

Sidebar: we should probably add some cross tests, particularly for big-endian architectures.

Here's an example of the ones we use for the @RustCrypto elliptic curve crates (although testing on both MSRV and stable is probably overkill):

https://github.com/RustCrypto/elliptic-curves/blob/0337e2a/.github/workflows/p256.yml#L89-L109

@pinkforest pinkforest force-pushed the feat-wasm-armv7-default-u64-serial branch from 826a532 to 6383f2d Compare December 9, 2022 17:06
@pinkforest
Copy link
Contributor Author

K. I'll add those here

@tarcieri
Copy link
Contributor

tarcieri commented Dec 9, 2022

I'm not super familiar with WASM benchmarking but this seems like an option:

https://docs.rs/easybench-wasm/latest/easybench_wasm/

@burdges you brought this up originally... any thoughts on how to do WASM benchmarks?

@pinkforest pinkforest force-pushed the feat-wasm-armv7-default-u64-serial branch from 6383f2d to 97c84d7 Compare December 9, 2022 17:11
@pinkforest
Copy link
Contributor Author

pinkforest commented Dec 9, 2022

Should we also build / test on windows and mac runners ?
I have a portable builder that uses diff runners https://github.com/rinse-repeat/rust-auditable-builder here
zig allows easy cross w/ cc w/o cross but then qemu could be used to test if we need (as you pointed elsewhere I think)

@tarcieri
Copy link
Contributor

tarcieri commented Dec 9, 2022

@pinkforest that's something I think is helpful when using std APIs with different std::os-specific backends, but this is a purely computational crate now which doesn't link std at all so I think it's probably unnecessary in this case

@pinkforest pinkforest force-pushed the feat-wasm-armv7-default-u64-serial branch from 97c84d7 to c0d5ff6 Compare December 9, 2022 17:35
@pinkforest
Copy link
Contributor Author

Yeah that's a very good point

@pinkforest
Copy link
Contributor Author

This is now documented here alongside other backend selection changes:

@tarcieri
Copy link
Contributor

tarcieri commented Dec 9, 2022

It would be good to confirm via a benchmark that WASM benefits from this, which is something I can poke at this weekend unless someone else wants to.

@pinkforest
Copy link
Contributor Author

pinkforest commented Dec 9, 2022

With criterion could have to wrap the wasm blob with a runtime - e .g. with lunatic - system oriented - we do:
https://github.com/lunatic-solutions/lunatic/blob/main/benches/benchmark.rs

But I think the easybench_wasm looks okay

For browser few pop up:
https://github.com/originjs/WASM-benchmark

https://github.com/PSPDFKit-labs/pspdfkit-webassembly-benchmark
that provides this:
http://iswebassemblyfastyet.com/

Though those got a lot of host/guest (js/wasm) boundary stuff going that creates overhead but still diff runtimes.

wasm is complicated beast to bench as there is tons of diff runtimes and not everyone agrees to "standards" 🤷‍♀️

@rozbb
Copy link
Contributor

rozbb commented Dec 9, 2022

I think sticking with browser benches (easybench looks okay) is probably enough for now. For this crate, I'd guess the most common place people will try to deploy their WASM is Chrome.

@pinkforest
Copy link
Contributor Author

FWIW - wasmer seems to benchmark sha1 crate via their runtime test
https://github.com/wasmerio/wasmer-bench/blob/master/benchmarks/src/sha.rs

@pinkforest pinkforest force-pushed the feat-wasm-armv7-default-u64-serial branch 3 times, most recently from 97bd7df to 31564a9 Compare December 9, 2022 23:28
@pinkforest pinkforest changed the title Set well known defaults for wasm and arm Fixes curve25519_dalek_bits defaults for cross and wasm Dec 9, 2022
@pinkforest
Copy link
Contributor Author

pinkforest commented Dec 9, 2022

I've set this to only fix the build.rs for cross-compile and wasm targets for now as keen to get this merged.
This also gives the structure for well known targets -
Another follow-up PR perhaps can be instead used to provide well known defaults armv7 and wasm potentially (original intent of this PR) when we have more test / bench coverage on those overrides to justify these defaults.
This also now includes CI for some cross targets.

@pinkforest pinkforest force-pushed the feat-wasm-armv7-default-u64-serial branch 5 times, most recently from c32b031 to 2bba4de Compare December 10, 2022 00:11
build.rs was using cfg(target) but it has to evaluate this from env TARGET
as build.rs cfg(target) in build context is the builder host and not the target.

This change fixes curve25519_dalek_bits lottery to determine the correct
automatic curve25119_dalek_bits with the help of platforms crate.

As discussed in dalek-cryptography#456 this also prepares for well known defaults for wasm and
arm serial backend via cfg(curve25519_dalek_bits = "64")

If the wasm32 or armv7 are going to be u64 serial by default these will be
followed up on later.
@pinkforest pinkforest force-pushed the feat-wasm-armv7-default-u64-serial branch from 2bba4de to 3d2d3d9 Compare December 10, 2022 00:46
@rozbb rozbb merged commit cb42e87 into dalek-cryptography:release/4.0 Dec 10, 2022
@burdges
Copy link
Contributor

burdges commented Dec 10, 2022

you brought this up originally... any thoughts on how to do WASM benchmarks?

I donno, but I'll forward the question to people who do.

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.

None yet

4 participants