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

[Proposal] Split eqaf.bigstring and eqaf.cstruct into their own toplevel libraries #27

Merged
merged 3 commits into from Aug 6, 2021

Conversation

kit-ty-kate
Copy link
Contributor

optional libraries were a mistake

No for real, optional libraries in this context are hard to use, for example you need to know that to get eqaf.cstruct you also need to require bigstring-compat. It would be much easier for users of the library to simply require eqaf-cstruct instead

@dinosaure
Copy link
Member

/cc @hannesm who uses this library (and specially eqaf.cstruct). I agree with this patch. However, I will be able to make release when #26 will be merged.

@hannesm
Copy link
Member

hannesm commented Apr 13, 2021

hmm, what I'm wondering: can we bump the lower ocaml bound to 4.07.0 and get rid of bigarray-compat from the source base? I can't see a <4.07 user of this package. this would allow us to have two packages instead of three (or one).

~optional libraries were a mistake~

No for real, optional libraries in this context are hard to use, for example you need to know that to get eqaf.cstruct you also need to require bigstring-compat. It would be much easier for users of the library to simply require eqaf-cstruct instead
@dinosaure
Copy link
Member

Rebased on top, as @hannesm said, we should merge eqaf-cstruct and eqaf-bigstring but I don't know which name we should pick, WDYT @hannesm?

Copy link
Member

@dinosaure dinosaure left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dinosaure@turbine:~/dev/eqaf/_build/default/check$ nice -n19 ./check.exe 
Random: [|57420;41069;54817;64253;63934;64230;4054;6207;34546;20369;14111;47443;31090;25122;16285;17015;|].
> Start to test Eqaf.equal (B¹).
> Start benchmarks on [fn⁰].
[########################################] 100%
> Start benchmarks on [fn¹].
[########################################] 100%
> Merge results.
> Start linear regression.
> Start to test String.equal (B²).
> Start benchmarks on [fn⁰].
[########################################] 100%
> Start benchmarks on [fn¹].
[########################################] 100%
> Merge results.
> Start linear regression.
Eqaf.equal: 2735.389999 ns/run.
String.equal: 338.257329 ns/run.
B¹ = 0.090717, B² = -333.430381.
1 trial(s) for Eqaf.equal.
> Start to test Eqaf.compare (B¹).
> Start benchmarks on [fn⁰].
[########################################] 100%
> Start benchmarks on [fn¹].
[########################################] 100%
> Merge results.
> Start linear regression.
> Start to test String.compare (B²).
> Start benchmarks on [fn⁰].
[########################################] 100%
> Start benchmarks on [fn¹].
[########################################] 100%
> Merge results.
> Start linear regression.
Eqaf.compare: 13830.765706 ns/run.
String.compare: 84.111268 ns/run.
B¹ = -3.240837, B² = -76.313196.
1 trial(s) for Eqaf.compare.
> Start to test Eqaf.exists_uint8 (B¹).
> Start benchmarks on [fn⁰].
[########################################] 100%
> Start benchmarks on [fn¹].
[########################################] 100%
> Merge results.
> Start linear regression.
> Start to test String.contains (B²).
> Start benchmarks on [fn⁰].
[########################################] 100%
> Start benchmarks on [fn¹].
[########################################] 100%
> Merge results.
> Start linear regression.
Eqaf.exists_uint8: 17290.743723 ns/run.
String.contains: 51.692260 ns/run.
B¹ = 4.805354, B² = 4597.242297.
1 trial(s) for Eqaf.exists.
> Start to test Eqaf.find_uint8 (B¹).
> Start benchmarks on [fn⁰].
[########################################] 100%
> Start benchmarks on [fn¹].
[########################################] 100%
> Merge results.
> Start linear regression.
> Start to test String.index (B²).
> Start benchmarks on [fn⁰].
[########################################] 100%
> Start benchmarks on [fn¹].
[########################################] 100%
> Merge results.
> Start linear regression.
Eqaf.find_uint8: 17123.774675 ns/run.
String.index: 50.177482 ns/run.
B¹ = 15.254458, B² = 4604.834693.
1 trial(s) for Eqaf.find_uint8.
> Start to test Eqaf.divmod (B¹).
> Start benchmarks on [fn⁰].
[########################################] 100%
> Start benchmarks on [fn¹].
[########################################] 100%
> Merge results.
> Start linear regression.
> Start to test Int32.unsigned_div,Int32.unsigned_rem (B²).
> Start benchmarks on [fn⁰].
[########################################] 100%
> Start benchmarks on [fn¹].
[########################################] 100%
> Merge results.
> Start linear regression.
Eqaf.divmod: 85.483859 ns/run.
Int32.unsigned_div,Int32.unsigned_rem: 40.876415 ns/run.
B¹ = -0.002884, B² = -0.353050.
1 trial(s) for Eqaf.divmod.

The PR can be merged as is when it does not add trouble on our assumptions.

@dinosaure
Copy link
Member

eqaf will depends on cstruct now to avoid a headache about the process release. I think it's the best solution and sorry for troubles @kit-ty-kate. I will do the release today!

@dinosaure
Copy link
Member

🎉

@dinosaure dinosaure merged commit def664b into mirage:master Aug 6, 2021
dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Aug 6, 2021
CHANGES:

- Fix the check tool on 4.11.0 (@dinosaure, @cfcs, @stedolan, mirage/eqaf#30)
  The compilation on 4.11 triggers a case where the locality of the expected
  value when we test `exists_uint8` must be the same. Otherwise, the access to
  this value can have a cost which faults our result.
- Add several utility functions (@cfcs, @dinosaure, mirage/eqaf#26)
  * `bytes_of_hex` & `string_of_hex`, hex decoding
  * `hex_of_bytes` & `hex_of_string`, hex encoding
  * `divmod`, unsigned `int32` division with small divisors
  * `ascii_of_int32`, conversion from `int32` to decimal `string`
    representation
  * `lowercase_ascii` & `uppercase_ascii`, _constant-time_ implementation of
    `String.{lower,upper}case_ascii`
  * `select_a_if_in_range`, like `select_int` but only supporting positive
    ranges
  * `int_of_bool` & `bool_of_int`, _constant-time_ of `Bool.to_int`

  A documentation exists for each function. The _constant-time_ is checked only
  systematically for `divmod`.
- Merge optional sub-packages (@kit-ty-kate, @hannesm, @dinosaure, mirage/eqaf#27)
  `cstruct` becomes a required dependency of `eqaf`
- Fix FreeBSD support and remove support of < OCaml 4.07 and remove the
  dependency to `bigarray-compat` (@hannesm, @dinosaure, mirage/eqaf#32)
- Add a CI on FreeBSD (@dinosaure, @hannesm, mirage/eqaf#33)
- Remove the test `check/check.exe` (@dinosaure, mirage/eqaf#31)
  The test `check/check.exe` is really volatile and should be executed into a
  controlled environment (for instance, with `nice -n19` and a _bare-metal_
  computer). We still require the test for any improvement of `eqaf` but it is
  executed separately from our CI.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants