Make the benchmark setup process faster and the benchmark itself more stable #6094
Conversation
kit-ty-kate
force-pushed
the
improve-bench
branch
from
5fc3786 to
f2b3ab7
Compare
| RUN find "$(pwd)/opam-repository" -name opam -type f > /home/opam/all-opam-files | ||
| RUN opam init --bare -n --disable-sandboxing /rep/opam-repository | ||
| RUN opam switch create --fake default 4.14.0 | ||
| RUN find /rep/opam-repository -name opam -type f > /home/opam/all-opam-files |
There was a problem hiding this comment.
Reading file bench test depends on OPAMREPOSHA, that can evolve in the future.
There was a problem hiding this comment.
Sure but we'd see that evolution on the graph associated with the commit changing it so i don't think this is an issue.
There was a problem hiding this comment.
The idea of these benches is to have them stable over the time, to only see changes related to test target, no?
There was a problem hiding this comment.
stable over runs yes, not necessarily stable over time, i would say. Benchmark stable over time is an extremely sticky thing to do and i don't think we should meddle with that (e.g. at some point in the future the machine running those benchmarks will die and the result of the bench will change)
There was a problem hiding this comment.
Of course we can't rely on it indefinitely, as it depends on a machine, but i think that we shouldn't have a benchmark that changes if an external value changes (here repo hash). In fact, it is the same for other opam install bench tests too, they depend son that repo hash too.
Anyway, in the future if we need to change that hash, we can update the test accordingly (e.g. have 2 repo hashes).
| launch "opam switch create two --empty"; | ||
| launch "opam switch set-invariant core -n --sw two"; | ||
| time_cmd ~exit:0 (fmt "%s install magic-trace -y --fake --sw two" bin) | ||
| in | ||
| let time_OpamSystem_read_10 = | ||
| let time_OpamSystem_read_100 = |
There was a problem hiding this comment.
This reading bench test concerns reading a lot of opam files. Do we want to have also a test for reading huge file too? Currently (with this hash), all-opam-files file is 1.3M.
There was a problem hiding this comment.
i don't think so given the overwhelming present use-case of OpamSystem.read. Also i don't think this should be done in this PR if we'd want to do that. This PR is only for stability and performance improvements.
Required for #6078 as it makes the new benchmarks stable enough to be merged.