Skip to content
This repository has been archived by the owner on Jul 21, 2023. It is now read-only.

Enable WebWorkers to make it go faster #9

Closed
daviddias opened this issue Sep 11, 2016 · 11 comments
Closed

Enable WebWorkers to make it go faster #9

daviddias opened this issue Sep 11, 2016 · 11 comments
Assignees
Labels
kind/enhancement A net-new feature or improvement to an existing feature need/community-input Needs input from the wider community

Comments

@daviddias
Copy link
Member

node-forge supports WebWorkers
image

enable here: https://github.com/libp2p/js-libp2p-crypto/blob/master/src/keys/rsa.js#L131

@dignifiedquire
Copy link
Member

Sorry, but this is already enabled, the default is 2 workers: https://github.com/digitalbazaar/forge/blob/master/js/rsa.js#L1574

@victorb
Copy link
Member

victorb commented Sep 12, 2016

Guess it would make sense to set it to os.cpus().length by default (for us at least) rather than 2.

@dignifiedquire
Copy link
Member

os is not available in the browser, and web workers are only available in the browser

@Kubuxu
Copy link
Member

Kubuxu commented Sep 12, 2016

Why not use -1 as suggested?

@victorb
Copy link
Member

victorb commented Sep 12, 2016

os is not available in the browser, and web workers are only available in the browser

Sorry, navigator.hardwareConcurrency would be more appropriate in browser land. Otherwise https://github.com/feross/cpus might help abstract it all away.

@dignifiedquire
Copy link
Member

Why not use -1 as suggested?

Yes we could use the internal estimator -1.

@dignifiedquire
Copy link
Member

Oh, I'm so sorry everyone libp2p-crypto currently uses the blocking sync version. So this is an issue. It means I need to change the interface, but will do that asap and switch to the faster async version...

@dignifiedquire dignifiedquire added kind/enhancement A net-new feature or improvement to an existing feature libp2p-ms1 labels Sep 12, 2016
@dignifiedquire dignifiedquire self-assigned this Sep 12, 2016
@dignifiedquire
Copy link
Member

It's not as easy as I would like, as we are depending on this being synchronous in the constructors of js-peer-id and js-peer-info. That means changing this requires as us to refactor the use of all those constructors to methods like createPeerId(cb).

@dignifiedquire dignifiedquire added the need/community-input Needs input from the wider community label Sep 12, 2016
@daviddias
Copy link
Member Author

@dignifiedquire can we get a bench test to run in a couple of laptops to see if node-forge is efficient using WebWorkers? I want to know if we save enough time that justifies all of those changes

@dignifiedquire
Copy link
Member

@diasdavid we need to make the change in any case to use webcrypto later down the line, i.e. all crypto operations should be done async everywhere. But will need to generate some tests what speed diffs we get

@dignifiedquire
Copy link
Member

Fixed in #12

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/enhancement A net-new feature or improvement to an existing feature need/community-input Needs input from the wider community
Projects
None yet
Development

No branches or pull requests

4 participants