Performance issues on Wasm build

[gregdhill]

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]

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

[real-or-random]

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

[elichai]

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:

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

After re-using the context:

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

Diff

diff --git a/src/lib.rs b/src/lib.rs
index e36aa5e..98fe0de 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -41,12 +41,10 @@ pub mod parity_libsecp256k1 {
 }
 
 pub mod bitcoin_core_libsecp256k1 {
-    use bitcoin_core_secp256k1::{ffi::types::AlignedType, Error, PublicKey, Secp256k1};
+    use bitcoin_core_secp256k1::{ffi::types::AlignedType, Error, PublicKey, Secp256k1, AllPreallocated};
     use test::Bencher;
 
-    fn ecmul() -> Result<(), Error> {
-        let mut buf = vec![AlignedType::zeroed(); Secp256k1::preallocate_size()];
-        let secp = Secp256k1::preallocated_new(&mut buf)?;
+    fn ecmul(secp: &Secp256k1<AllPreallocated<'_>>) -> Result<(), Error> {
 
         let secret_key = &[
             137, 16, 46, 159, 212, 158, 232, 178, 197, 253, 105, 137, 102, 159, 70, 217, 110, 211,
@@ -72,12 +70,16 @@ pub mod bitcoin_core_libsecp256k1 {
 
     #[test]
     fn test_libsecp256k1() {
-        ecmul().unwrap();
+        let mut buf = vec![AlignedType::zeroed(); Secp256k1::preallocate_size()];
+        let secp = Secp256k1::preallocated_new(&mut buf).unwrap();
+        ecmul(&secp).unwrap();
     }
 
     #[bench]
     fn bench_libsecp256k1(b: &mut Bencher) {
-        b.iter(|| ecmul().unwrap());
+        let mut buf = vec![AlignedType::zeroed(); Secp256k1::preallocate_size()];
+        let secp = Secp256k1::preallocated_new(&mut buf).unwrap();
+        b.iter(|| ecmul(&secp).unwrap());
     }
 }

[gmaxwell]

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]

@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]

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]

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]

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

[gregdhill]

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

[elichai]

@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]

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.

[gregdhill]

@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.