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

Add support for doing the hashing in a worker thread #16

Closed
sindresorhus opened this issue Mar 12, 2019 · 13 comments · Fixed by #21
Closed

Add support for doing the hashing in a worker thread #16

sindresorhus opened this issue Mar 12, 2019 · 13 comments · Fixed by #21
Labels
enhancement 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt help wanted

Comments

@sindresorhus
Copy link
Owner

sindresorhus commented Mar 12, 2019

Issuehunt badges

Could maybe be useful for hashing large files, as the crypto Node.js module is synchronous, so it could block the main thread for some time.

https://nodejs.org/api/worker_threads.html

Thoughts?


IssueHunt Summary

stroncium stroncium has been rewarded.

Backers (Total: $80.00)

Submitted pull Requests


Tips


IssueHunt has been backed by the following sponsors. Become a sponsor

@tomasdev
Copy link

I like it. It's usually not super expensive, but a 1gb file shasum takes ~4s on my macbook 2015 2.8 GHz Intel Core i7

I would actually prefer a C++ binding (that I'd guess would be faster, since in OS X for example, running openssl sha1 :file takes 4x less time than the crypto approach in node.

@sindresorhus
Copy link
Owner Author

I would actually prefer a C++ binding (that I'd guess would be faster, since in OS X for example, running openssl sha1 :file takes 4x less time than the crypto approach in node.

Not interested in doing native bindings. That comes it its own set of horribleness.

@sindresorhus
Copy link
Owner Author

Related to sindresorhus/crypto-hash#7

// @stroncium

@stroncium
Copy link
Contributor

Can easily be done for file hashes(and will work perfectly).
For strings/buffers, I don't see a way to go further than is done in sindresorhus/crypto-hash#7 so I'd just use it.
As far as my understanding goes, we will have to always clone(not transfer) bytes for arbitrary streams, and as chunks are expected to be pretty small, the chance clone+send task overhead will be be bigger than just hashing in place is quite high. Needs some benchmarks, but I'm pretty sure it's not viable.

Speaking of performance comparison with openssl, it sounds strange to me, as node is using openssl underneath. If such issue actually exists it could use some serious debugging on libuv/nodejs/app level.

@issuehunt-oss issuehunt-oss bot added the 💵 Funded on Issuehunt This issue has been funded on Issuehunt label May 2, 2019
@IssueHuntBot
Copy link

@IssueHunt has funded $80.00 to this issue.


@stroncium
Copy link
Contributor

On it.
@sindresorhus Should I add crypto-hash as a dependency or duplicate code?

@sindresorhus
Copy link
Owner Author

Does crypto-hash expose enough functionality? Would be good to share code if possible, yes.

@stroncium
Copy link
Contributor

@sindresorhus Need to check it, but should be enough.

stroncium added a commit to stroncium/hasha that referenced this issue May 2, 2019
@stroncium
Copy link
Contributor

@sindresorhus Never mind. I didn't notice there was no async version of string/buffer hashing in hasha. Without it there is almost no code intersection with crypto-hash.

@sindresorhus
Copy link
Owner Author

I didn't notice there was no async version of string/buffer hashing in hasha.

Sorry if the issue was not clear enough on this, but the intention is to add it now that it's possible.

@stroncium
Copy link
Contributor

@sindresorhus Well, then there are some questions:

  • Should it be a breaking change(to promise returning function) or additional method?
  • crypto-hash doesn't support md5, should we add it just for nodejs? (And while at it, should we add a method you pass algorithm string to for consistency?)
  • If not, should we use crypto-hash at all? Most code will need to be duplicated here to implement md5 anyhow.

@sindresorhus
Copy link
Owner Author

Should it be a breaking change(to promise returning function) or additional method?

Additional method.

crypto-hash doesn't support md5, should we add it just for nodejs? (And while at it, should we add a method you pass algorithm string to for consistency?)

Nah

If not, should we use crypto-hash at all? Most code will need to be duplicated here to implement md5 anyhow.

Probably not worth using crypto-hash at all then.

@issuehunt-oss
Copy link

issuehunt-oss bot commented Sep 22, 2019

@sindresorhus has rewarded $72.00 to @stroncium. See it on IssueHunt

  • 💰 Total deposit: $80.00
  • 🎉 Repository reward(0%): $0.00
  • 🔧 Service fee(10%): $8.00

@issuehunt-oss issuehunt-oss bot added 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt and removed 💵 Funded on Issuehunt This issue has been funded on Issuehunt labels Sep 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt help wanted
Projects
None yet
4 participants