Skip to content

Conversation

@sadiqj
Copy link
Contributor

@sadiqj sadiqj commented Mar 10, 2019

Added simple tests for:

  • ocaml-c api
  • lazy
  • finalisers
  • stacks
  • weak arrays

@stedolan
Copy link
Contributor

Looks good! Glad to have these microbenchmarks, it'll be good to have some regression tests for performance.

Two general comments about the implementations:

  • Some of these benchmarks look like a smart optimiser could delete chunks of them. (e.g. ignore(ref v) in finalise.ml). The standard library has a feature specifically to help with this: Sys.opaque_identity is a function that just returns its argument. However, the optimiser does not know that Sys.opaque_identity returns its argument, so it has to compute the argument fully.

    So, instances of ignore v should be ignore (Sys.opaque_identity v) to guarantee that v is computed, and benchmark loops like for i = 1 to 1000 do ignore (f x) done should be for i = 1 to 1000 do ignore (Sys.opaque_identity f x) done.
    G
    There's a subtlety here: it should be Sys.opaque_identity f x not Sys.opaque_identity (f x) - the latter means that the optimiser can see that f is applied to x, and can optimise the body of f accordingly.

  • Some of the benchmarks adjust their iteration counts depending on Gc stats. (e.g. while Gc.allocated_words () < x do ... done). This means that the benchmark will give misleading results on configurations that change the number of allocations: e.g. on 32-bit systems, allocating a float takes twice the number of words as on 64-bit systems, and some Flambda configurations can reduce the number of allocations compared to trunk (by e.g. combining allocations or noticing that some are unnecessary). If the benchmark adjusts its iteration count in response, its runtime won't reflect these differences.

@sadiqj sadiqj force-pushed the more-simple-tests branch from b2437c5 to 661e06d Compare March 14, 2019 07:40
@sadiqj sadiqj force-pushed the more-simple-tests branch from 95e44e4 to 76365af Compare March 20, 2019 14:28
@ctk21 ctk21 self-requested a review March 25, 2019 09:42
Copy link
Member

@ctk21 ctk21 left a comment

Choose a reason for hiding this comment

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

Looks good to me (although didn't run the code).

Minor things about consistent application of Sys.opaque_identity across the different tests.

@sadiqj sadiqj merged commit 3bdd36c into master Mar 27, 2019
@sadiqj sadiqj deleted the more-simple-tests branch March 27, 2019 11:33
kayceesrk pushed a commit that referenced this pull request Sep 2, 2020
…0SEQ

Revert "Graph 500 Benchmark [SEQUENTIAL]"
moazzammoriani added a commit to moazzammoriani/sandmark that referenced this pull request Jun 8, 2022
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.

4 participants