-
Notifications
You must be signed in to change notification settings - Fork 38
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
Using eventring in pausetimes benchmarks instead of outdated eventlog #390
Conversation
I think it would be easier for anyone reading about this to keep the discussion on the original issue |
c74259a
to
8397042
Compare
I tested the current state of the branch on navajo and it seems to work |
Can you please update the PR on how to run and view the latency measurements in the |
I've revived this branch and it should now be ready for review (modulo the CI). There are a number of "hacks" to make the installation of
I've also removed the Note that |
df85bb8
to
8e1bb55
Compare
Thanks for the changes. We need to support 4.14 and 5.0.0 development branches as well and hence the SYS_DUNE_HACK is used. When I test for 5.0.0~alpha0 using the following commands, I get a package conflict resolution error.
We can install dune 3.5 on the local switch, and proceed with the existing workflow. Would it be possible to gate the necessary steps for 5.1.0 in a case statement, so that it does not get triggered for the other development branches? |
Right, the errors were triggered by a spurious version change in an unrelated package, this should now be fixed.
Do you mean that:
|
I don't think we need pausetimes in 4.14. It is a lot of work to revive pause times -- eventlog has in 4.14 compiler has been replaced by runtime events. I don't think we should invest any time in dead features. We should be aiming to do very little or no work on 4.14. Out of curiosity, why do we care about 4.14? In order to review this PR, and other similar ones in sandmark, is there a test deployment or instructions for deploying locally? I can use it to understand how the feature works. Easier for me to see the whole thing in action than reading the code. |
Yes, but, only for 5.0.0 as KC mentioned. I can confirm that the build is clean now for 5.0.0~alpha0.
Okay.
The following commands help produce the pausetimes results:
Sample output:
|
Okay, the build should now be clean for every version, and |
Don't we also need a new notebook to visualise the pause times? Such a thing used to exist in the sequential and parallel notebooks earlier: https://github.com/ocaml-bench/sandmark/blob/main/notebooks/sequential/sequential.ipynb towards the end. Observe that the sequential notebook says Rather than delay merging this PR, I would recommend merging this in, then starting to run these in the nightly runs first. We can have a separate sequential and parallel latency pages separately. (IIRC sandmark nightly also had pausetimes support earlier, which has been removed). |
Yes.
Yes, at https://github.com/ocaml-bench/sandmark/blob/429f52f7b0ea9dbe760b3e249a17cd0b7b629e95/notebooks/sequential/sequential.ipynb - Latency, Max latency and 99.9th percentile latency.
Sure! |
This PR aims to resolve #358, by removing calls to eventlog-based tools that don't work with OCaml 5.0 with eventring, and in particular its wrapper
olly
.This PR requires first merging my PR on runtime_events_tools to work properly.