ssbc / chloride

MIT License
90 stars 17 forks source link

wasm variant is being irregularly picked #10

Closed staltz closed 5 years ago

staltz commented 5 years ago

In my app I'm using chloride and wish to use the web assembly variant (from sodium-browserify). One of the first things my backend code does is set the env var CHLORIDE_JS, and in runtime I verified with a console.log that this line is running:

https://github.com/dominictarr/chloride/blob/7593ece159a2d9952156d26e25c4eca4a77cffc0/index.js#L4

In other words ./browser.js is certainly being imported, which imports sodium-browserify.

However, later on the app, the sodium-browserify-tweetnacl functions are being called constantly (many times after startup), but we expect sodium-browserify to catch on instead. The log below was printed during runtime, showing that tweetnacl functions are being called constantly.

crypto_scalarmult (sodium-browserify-tweetnacl/index.js)
     at Object.exports.multibox_open_key (private-box/index.js)
     at Object.exports.unboxKey (ssb-keys/index.js)
     at Object.key (ssb-db/minimal.js)
     at unbox (ssb-db/minimal.js)
     at EventEmitter.db.unbox (ssb-db/minimal.js)
     at Array.unboxerMap (ssb-db/minimal.js)
     at chainMaps (ssb-db/minimal.js)
     at mapper inside get (flumedb/index.js)
     at get (flumelog-offset/inject.js)

I took a look at the implementation of sodium-browserify, and added console.logs here and there. The interesting thing is that the mutation to apply webassembly happens much before (4 seconds) the tweetnacl uses of crypto_scalarmult, see these timestamps:

21:51:02.804: mutating crypto_sign_verify_detached to use webassembly
21:51:02.804: mutating crypto_sign_detached to use webassembly
21:51:02.805: mutating crypto_sign to use webassembly
21:51:02.805: mutating crypto_sign_open to use webassembly
21:51:02.805: mutating crypto_sign_ed25519_pk_to_curve25519 to use webassembly
21:51:02.805: mutating crypto_sign_ed25519_sk_to_curve25519 to use webassembly
21:51:02.805: mutating crypto_scalarmult to use webassembly
21:51:02.805: mutating crypto_secretbox_easy to use webassembly
21:51:02.805: mutating crypto_secretbox_open_easy to use webassembly
21:51:02.805: mutating crypto_box_easy to use webassembly
21:51:02.805: mutating crypto_box_open_easy to use webassembly
21:51:02.805: mutating crypto_auth to use webassembly
21:51:02.805: mutating crypto_hash to use webassembly
21:51:06.746: low order
21:51:06.746: low order
21:51:06.746: low order
21:51:06.746: low order
21:51:06.746: low order
              (this repeats forever)

So my best guess so far is that the mutation of the exports is not propagating correctly, maybe because some consumer library is extracting functions from chloride (like var scalarmult = chloride.crypto_scalarmult) instead of using chloride methods directly (like chloride.crypto_scalarmult(sk,pk)).

dominictarr commented 5 years ago

oh, yes that makes sense. okay fixed in sodium-browserify@1.2.6 and chloride@2.2.13

staltz commented 5 years ago

Thanks @dominictarr for the quick response. What I'll share next may surprise you though.

I updated chloride to 2.2.13 and added console.logs inside tweetnacl's low order checker, and was surprised to see that the console.logs were running in runtime, i.e. tweetnacl is being used instead of webassembly.

I also profiled a manyverse-to-manyverse initial sync experience before and after updating. Phone A had a real-world database (it was @andrestaltz_phone) and Phone B had an empty database.

So apparently this was a big deal, a 6x slowdown. I took a look at your sodium-browserify commit, but still puzzled why is tweetnacl being selected instead.

dominictarr commented 5 years ago

okay I found two things - one was that if you set the envvar CHLORIDE_JS=1 it used wasm (which is what I did for testing) but if it merely failed to load the bindings it would use tweetnacl!

that is fixed. Also I used a wrapper thing that did [].slice.call(arguments) which I understand is slow, but am a bit surprised it's 6x slow! maybe it breaks optimized code though?

please try chloride@2.2.14

staltz commented 5 years ago

@dominictarr Great, chloride@2.2.14 does the trick, and that initial sync experiment took 31 seconds, so 8 seconds less. A decent improvement.

This issue is to me closed now

dominictarr commented 5 years ago

great. btw, I also realized the the libsodium js is way too big. browserifying just that is 1.3 mb(!) I think it's mainly because it loads crypto (but it only needs randomBytes) I'm gonna try compiling tweetnacl to wasm and see if I can get it smaller.