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

Revive instrumented pausetimes using eventring #358

Closed
kayceesrk opened this issue Jun 9, 2022 · 9 comments · Fixed by #390
Closed

Revive instrumented pausetimes using eventring #358

kayceesrk opened this issue Jun 9, 2022 · 9 comments · Fixed by #390
Assignees
Labels
enhancement New feature or request high-priority Requires immediate work

Comments

@kayceesrk
Copy link
Contributor

OCaml 5 has support for eventring: https://github.com/ocaml/ocaml/blob/trunk/otherlibs/runtime_events/runtime_events.mli, a new ring-buffer based runtime tracing capability. This replaces the earlier eventlog support. The instrumented pausetimes in Sandmark used to be built on the eventlog support. This support has to be rebuilt using eventring.

Given that eventring has a nice OCaml API, it is possible that orun may be modified to directly produce the tail latency distribution without having to go through the python post-processing.

@ElectreAAS
Copy link
Contributor

My proposal for this is to move from using orun to using olly, which does substantially the same thing, only with the more modern Runtime_events from Stdlib 5.0.
I made a PR linked above to add a json printing option to olly, so it can be easily consumed by sandmark.
The consumption happens in what used to be bash scripts in sandmark, in #390.

@kayceesrk
Copy link
Contributor Author

kayceesrk commented Sep 2, 2022

Thanks @ElectreAAS. I agree with the observersation. In fact, I added the latency subcommand to olly for exactly this purpose. Thanks for moving this forward. The latency analysis is going to be super important as we work on the next round of improvements to the OCaml GC.

@ElectreAAS
Copy link
Contributor

I have so far modified the bash script pausetimes/pausetimes_multicore, which seems to do the job, but there is another bash script (pausetimes_trunk) in the same directory that uses the outdated eventlog. Should I modify this one too?

@kayceesrk
Copy link
Contributor Author

I don't think pausetimes_trunk needs to be modified. It may have been originally needed since trunk and multicore were separate compilers.

@kayceesrk
Copy link
Contributor Author

This issue and #362 seem similar. It may be better to close one of them so that the discussion is not split between the two.

@Firobe Firobe added the high-priority Requires immediate work label Nov 14, 2022
@Firobe Firobe self-assigned this Nov 14, 2022
@kayceesrk
Copy link
Contributor Author

kayceesrk commented Nov 22, 2022

Is there an issue for tracking the development of UI / jupyter notebook for the instrument pausetimes?

FYI the latency metrics should not / need not use the instrumented runtime now. The default runtime itself produces the runtime events in the ring buffer. Olly consumes this buffer to produce latency graphs. Please ensure that the builds are not using instrumented pausetimes.

@Firobe
Copy link
Contributor

Firobe commented Nov 22, 2022

Is there an issue for tracking the development of UI / jupyter notebook for the instrument pausetimes?

I just created an issue for it #420

FYI the latency metrics should not / need not use the instrumented runtime now. The default runtime itself produces the runtime events in the ring buffer. Olly consumes this buffer to produce latency graphs. Please ensure that the builds are not using instrumented pausetimes.

OK!

@shakthimaan
Copy link
Contributor

Is there an issue for tracking the development of UI

Do you need the latency graphs to be displayed on a separate page in the Sandmark Nightly service?

@kayceesrk
Copy link
Contributor Author

Do you need the latency graphs to be displayed on a separate page in the Sandmark Nightly service?

I don't mind if this is a separate page or is on the same page as the sequential and parallel benchmarks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request high-priority Requires immediate work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants