-
Notifications
You must be signed in to change notification settings - Fork 371
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
Make SHA computation faster by using ocaml-sha #5042
Conversation
Nice, thanks for all this! I am a bit surprised by the poor results of Digestif here; I think that to be fair you should use its bigstrings functions and an We'll need to remember to check how the static linking scripts fare with the new C code for next release (I think it should be fine 🤞 but...) Another precision: there are lots of cases where opam computes hashes from already in-memory strings. The timings would be negligible in this case, except if we need to exec() an external binary to compute the hash like it used to be the case with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some things around to fix, but on the idea, ok!
c50527b
to
f87bc3a
Compare
b01c0c4
to
2df52db
Compare
As there is usage replacement in |
@@ -18,17 +18,8 @@ val sha512_file: string -> string | |||
val hash_file: [< `SHA256 | `SHA512 ] -> string -> string | |||
|
|||
|
|||
val sha256_bytes: Bytes.t -> string | |||
val sha256_string: string -> string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why renaming these functions from sha256
to sha256_string
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s not a renaming, only removal of functions that are not used.
fixed
|
(new) error is
|
84a6ff9
to
3ec98b2
Compare
In the spirit of #3217 and #4823 here is a PR that removes the runtime dependency on
openssl
.In the expected range of use of opam it also makes installation and updates faster than before.
Here is the benchmark i used to show both correctness and speed:
Using these typical files:
and this large atypical file:
Here is the result I get for SHA512:
The same order of result can be observed with SHA256. e.g.
Awaiting review to put the rest of the work (vendoring) for this PR to pass CI.
This PR is not urgent at all and rebase should be easy so no rush to put this before opam 2.2.0
Fixes #5037
Would help #4158