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

Modularise Olly #43

Merged
merged 6 commits into from
Apr 18, 2024
Merged

Modularise Olly #43

merged 6 commits into from
Apr 18, 2024

Conversation

eutro
Copy link
Contributor

@eutro eutro commented Apr 2, 2024

This PR splits the single monolithic bin/olly.ml file into several separate libraries, (olly_common, olly_trace, olly_gc_stats), as well as abstracting and modularising the trace formats of olly_trace. Other changes are:

  • There is a new Opam package runtime_events_tools_bare which builds the new bin/olly_bare.exe, which depends only on ocaml, dune, and cmdliner (omitting gc-stats and trace --format=fuchsia).
  • Minor and hopefully uncontroversial overall changes, that I wanted to make while I was touching the code anyway:

The benefits are:

  • Practically, modularising this way allows for both a minimal build with minimal dependencies, while still allowing a fuller build with all dependencies
    • Olly currently unconditionally depends on tracing, which transitively depends on many other packages, which aren't portable to trunk builds of OCaml, nor to Windows.
      • This could also be "fixed" by either using a different library, rolling our own trace writer, etc., but that would require a much larger behavioural change
    • The minimal build, olly_bare, builds with only dune and cmdliner as dependencies; these can be installed without Opam.
      • For instance, a build of olly_bare on trunk or Windows can be done by just installing OCaml and Dune, then adding cmdliner as a git submodule, and running dune build bin/olly_bare.exe
        • We now know that Olly doesn't currently work on Windows, due to how Unix on Windows handles PIDs (as HANDLEs casted to int), which is incompatible with Runtime_events needing real PIDs; the potential fixes aren't difficult, involving a GetProcessId somewhere (though PIDs should also just be handled better by core OCaml on Windows, it shouldn't be on the user to hack around it, e.g. win32unix: cannot kill and wait for arbitrary processes ocaml/ocaml#11021)
  • It is my personal opinion that this is much more maintainable; a single sprawling undocumented OCaml file is unclear. Now we have multiple undocumented OCaml files, but at least they have clear names and intentions.

Potential issues are:

  • This is a major change to the structure of the codebase
    • It may be controversial, in whether the changes made are desirable, or sufficiently high quality
    • Outstanding work on Olly would need to be substantially changed to fit the new structure. This should hopefully be simple, the two currently outstanding PRs, for instance, are relatively minor, but there may be other outstanding work that is not a public PR yet.

I am looking for feedback with whether this is desirable overall, as well as with the specific approach I took, since I am largely unexperienced with Dune as a build system, and with OCaml's ecosystem. For instance, the statically-linked minimal-olly_bare/full-olly approach is somewhat unsatisfying due to inflexibility (e.g. to include gc-stats without --format=fuchsia, without changing Olly's code). It may be desirable, perhaps as future work, if the subcommands (and format backends) could instead be loaded dynamically, rather than linked statically.

eutro added 3 commits March 25, 2024 16:29
- Minimise the number of arguments being passed around, improving clarity
- Add (currently unused) `runtime_counter`
@kayceesrk
Copy link
Collaborator

Thanks @eutro. Do you have a reviewer in mind already?

eutro added 3 commits April 2, 2024 13:16
Make olly_gc_stats and olly_format_fuchsia (optional)

- These are built anyway for the full olly package, but have unavailable dependencies on olly_bare

Clean up dune-project
@eutro
Copy link
Contributor Author

eutro commented Apr 3, 2024

Thanks @eutro. Do you have a reviewer in mind already?

Thanks; @sadiqj should definitely look at it; otherwise whoever is available.

@tmcgilchrist
Copy link
Collaborator

The dependency on tracing is unfortunate, for a fundamental tool like this something lighter would be preferrable. Right now it'll support Linux (x86_64, ARM64), macOS (x86_64, ARM64) and FreeBSD (x86_64) but adding support for other Tier-1 OCaml platforms is difficult. No idea about Windows, that's outside of my knowledge.

For outstanding work, #40 is trivial to update. I started an implementation of #41 for Unix but haven't finished it. How to implement that on Windows I am not sure.

@tmcgilchrist
Copy link
Collaborator

The modularity in this change is good @eutro. Not merging so @sadiqj can provide his comments.

My preference for the cli is just have olly and fix the dependencies so they can work on Windows, released versions of the compiler and OCaml trunk.

@sadiqj
Copy link
Collaborator

sadiqj commented Apr 15, 2024

This looks like a nice reorganisation to me, thanks @eutro

With regards to tracing as a dependency, it would be preferable to have something a little lighterweight. @talex5 are you working on something for eio tracing?

@talex5
Copy link

talex5 commented Apr 15, 2024

eio-trace has its own minimal library for fuchsia support: https://github.com/ocaml-multicore/eio-trace/tree/main/fxt

It does use Eio.Buf_{read,write} for buffered IO, though.

@sadiqj sadiqj merged commit bf8b316 into tarides:main Apr 18, 2024
3 checks passed
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.

5 participants