Performance issues on Wasm build

gregdhill commented at 12:11 pm on February 2, 2021: none

Not sure if this is an issue with the rust-bindings, but the performance of bitcoin-core/secp256k1 is distinctly slower than other Rust based libraries when compiled to Wasm. See the example benchmarks here.

real-or-random commented at 12:13 pm on February 2, 2021: contributor

Possibly related: https://twitter.com/alexeiZamyatin/status/1355605818218053637

real-or-random commented at 12:16 pm on February 2, 2021: contributor

Hey, I believe we should investigate this in the rust bindings first. I’m opening an issue there.

real-or-random cross-referenced this on Feb 2, 2021 from issue Performance regression on wasm by real-or-random

elichai commented at 3:12 pm on February 2, 2021: contributor

I believe this is the issue: https://github.com/gregdhill/bench-secp256k1-wasm/blob/dce71c9f3a86cd841178b33e24741316fb8e351b/src/lib.rs#L49 You’re creating a new Context for every run, you should create the context once and re-use it. the context is also thread-safe so you can just have a global context or just use the global-context feature (secp256k1::SECP256K1) (EDIT: this uses std so probably can’t do that on wasm, but you can still create a context yourself and re-use it)

The result on my machine, before:

0test bitcoin_core_libsecp256k1::bench_libsecp256k1 ... bench:   8,519,226 ns/iter (+/- 2,270,843)
1test parity_libsecp256k1::bench_libsecp256k1       ... bench:     154,244 ns/iter (+/- 45,649)
2test rust_crypto_libsecp256k1::bench_libsecp256k1  ... bench:      77,482 ns/iter (+/- 23,938)

After re-using the context:

0test bitcoin_core_libsecp256k1::bench_libsecp256k1 ... bench:      49,442 ns/iter (+/- 10,967)
1test parity_libsecp256k1::bench_libsecp256k1       ... bench:     141,952 ns/iter (+/- 32,162)
2test rust_crypto_libsecp256k1::bench_libsecp256k1  ... bench:      74,883 ns/iter (+/- 10,976)

 0diff --git a/src/lib.rs b/src/lib.rs
 1index e36aa5e..98fe0de 100644
 2--- a/src/lib.rs
 3+++ b/src/lib.rs
 4@@ -41,12 +41,10 @@ pub mod parity_libsecp256k1 {
 5 }
 6 
 7 pub mod bitcoin_core_libsecp256k1 {
 8-    use bitcoin_core_secp256k1::{ffi::types::AlignedType, Error, PublicKey, Secp256k1};
 9+    use bitcoin_core_secp256k1::{ffi::types::AlignedType, Error, PublicKey, Secp256k1, AllPreallocated};
10     use test::Bencher;
11 
12-    fn ecmul() -> Result<(), Error> {
13-        let mut buf = vec![AlignedType::zeroed(); Secp256k1::preallocate_size()];
14-        let secp = Secp256k1::preallocated_new(&mut buf)?;
15+    fn ecmul(secp: &Secp256k1<AllPreallocated<'_>>) -> Result<(), Error> {
16 
17         let secret_key = &[
18             137, 16, 46, 159, 212, 158, 232, 178, 197, 253, 105, 137, 102, 159, 70, 217, 110, 211,
19@@ -72,12 +70,16 @@ pub mod bitcoin_core_libsecp256k1 {
20 
21     #[test]
22     fn test_libsecp256k1() {
23-        ecmul().unwrap();
24+        let mut buf = vec![AlignedType::zeroed(); Secp256k1::preallocate_size()];
25+        let secp = Secp256k1::preallocated_new(&mut buf).unwrap();
26+        ecmul(&secp).unwrap();
27     }
28 
29     #[bench]
30     fn bench_libsecp256k1(b: &mut Bencher) {
31-        b.iter(|| ecmul().unwrap());
32+        let mut buf = vec![AlignedType::zeroed(); Secp256k1::preallocate_size()];
33+        let secp = Secp256k1::preallocated_new(&mut buf).unwrap();
34+        b.iter(|| ecmul(&secp).unwrap());
35     }
36 }

gmaxwell commented at 10:26 pm on February 2, 2021: contributor

Arguably a design flaw in the rust interface that such a huge performance footgun is essentially invisible in the source code and that the correct way of using the library is a lot more complicated and less obvious than an incorrect way. — also another argument for just making the verify tables static.

elichai commented at 8:53 am on February 3, 2021: contributor

@gmaxwell I completely agree with you, that’s also the reason we added the global-context recently, but A. It doesn’t help in no-std cases like wasm. B. It isn’t visible enough in the docs, we should make it more obvious.

So yeah a static context can go a long way here, especially if we hide it from the users (by removing the need for context from the API)

(FYI, the “global-context” is basically a static pointer with something similiar to a pthread_once initialization)

real-or-random commented at 9:46 am on February 3, 2021: contributor

such a huge performance footgun is essentially invisible in the source code

I’m not sure. The line is very visible in the source code. Or are you talking about the names?

gmaxwell commented at 9:52 am on February 3, 2021: contributor

I believe this is the issue: https://github.com/gregdhill/bench-secp256k1-wasm/blob/main/src/lib.rs#L49

Ah. This link is (now and when I looked at it) to the fixed code. So perhaps you see why I thought it was completely invisible that it was creating a context. :)

elichai commented at 9:53 am on February 3, 2021: contributor

Arghh the problem with linking to master, I fixed the link for future reference.

gregdhill commented at 9:58 am on February 3, 2021: none

@elichai do you have any suggestions for creating a singleton global context in wasm?

elichai commented at 10:19 am on February 3, 2021: contributor

@gregdhill I’ll have to see a full project, as there are many flavors of wasm runtime etc, and I’ll need to play with it. (e.g. if you have an allocator, I’d just box it, leak it, and store it in a static)

real-or-random cross-referenced this on Feb 3, 2021 from issue Fully static precomputation tables by real-or-random

real-or-random commented at 11:32 am on February 3, 2021: contributor

I’m closing this since the main issue has been solved. Feel free to continue discussion about how to best use the current library.

I opened #893 for a discussion on static contexts.

real-or-random closed this on Feb 3, 2021

gregdhill commented at 11:32 am on February 4, 2021: none

@elichai this is my WIP implementation (performance is still undesirable): https://gitlab.com/interlay/btc-parachain/-/blob/greg/rust-secp256k1/crates/bitcoin/src/address.rs#L154

The project itself is built on Substrate and the runtime is compiled to Wasm but I’m not certain how this is executed exactly.

Performance issues on Wasm build #892