Speedscope export

[mjambon]

Hello @mlasson,

On @nojb's suggestion, I added Speedscope export to Landmarks. Here's a PR that I created with AI assistance. It's ready for review. Please let me know what you think.

Supported format

Among the various formats supported by Speedscope, three of them are documented as generic, and one of them is Speedscope's own format. I decided to target it and it seems to work fine. I don't have big programs to test it on, though.

JSON support

The original implementation uses it own tiny implementation for exporting to JSON. I'm not sure why but if I assumed it's a matter of reducing external dependencies. Keeping this in mind, here's what I did:

• I translated the original spec (commented TypeScript types that match the JSON Schema spec) to ATD types. This provides automatic mapping from OCaml types to JSON with well-defined rules to handling field renames, optional fields, etc. and ensures statically that the exported JSON complies with the type definitions.
• Since this format is unlikely to change frequently if ever, the code generator atdml is not an Opam dependency of the landmarks package. A call to atdml speedscope_fmt.atd will regenerate speedscope_fmt.mli and speedscope_fmt.ml when the need arises. These two files are kept alongside source code in src/.
• The generated files add a dependency on the Yojson library. If we want to avoid this dependency, I can add a simple translation layer (no need to vendor the whole Yojson project).

Example

Download tests/speedscope/profile.json and then upload it at https://www.speedscope.app/.

Code quality

I paid reasonable attention to the interfaces and to the tests. I did not review src/speedscope.ml closely. Trying it out on non-trivial example would be great.

[mlasson]

Do you think that a lot of infrastructure is missing so that new export formats could be implemented as a separate library (we can still keep it inside this dune project)? In that case, using yojson as an extra dependency would not be a big issue.

[mjambon]

Sorry for the late reply [GitHub did not email me because I have a tab open. Maybe I fixed it now?]

Do you think that a lot of infrastructure is missing so that new export formats could be implemented as a separate library (we can still keep it inside this dune project)?

Yes, I think so. I'll update the PR accordingly. There will be a new landmarks-exports package that remains in this repo.

Here's my reasoning:

At first glance, the main issue is here:

let () = match Sys.getenv "OCAML_LANDMARKS" with
  | exception Not_found -> ()
  | str ->
      try start_profiling ~profiling_options:(parse_env_options str) ()
      with Exit -> ()

This allows custom exporters (via something like let exporters = ref []) to be registered by another library which is good. However, we need to make sure that the exporters are registered before the code that checks for valid exporters... and I don't think it's possible because the exporter library depends on the landmarks library so its modules would always be evaluated after landmarks.

I can think of the following solutions:

1. Ensure that the exporters are reliably registered before the Landmarks module is evaluated. This would require turning let () = match Sys.getenv "OCAML_LANDMARKS" with into an init function that the target program would have to call after registering the exporters. This would work but breaks backward compatibility.
2. We could be lenient during the interpretation of the format option provided by the environment variable and accept any name for the exporter until the time comes to use it at the end of the execution in the atexit hook. This would work but it's not great if the exporter name is invalid or the exporter was not registered because the error would kick in only at the end of the execution. Failing early would be preferable.
3. A hybrid solution could consist of checking if the exporter name is valid early on but only check for its existence at the time of calling it. This would catch misspellings early but failures to register the exporter would be delayed until the end of the execution.

I think the cleanest solution is (1) but it breaks backward compatibility. As a workaround, what we could do is:

• provide let init ?(auto = false) () = match Sys.getenv "OCAML_LANDMARKS" with ...
• keep let () = init ~auto:true () in Landmarks but make it do nothing if format is a custom format i.e. if auto is true and format is unknown, stay silent and do nothing.
• this expects the user to call init () explicitly if the format is unknown. If init hasn't been called successfully already and auto is false, then it will run normally.

This is a little contrived but doesn't add much code and allows arbitrary exporters to be registered by the user.

To recap, the behavior of the proposed solution is:

• OCAML_LANDMARKS=format=speedscope,output=profile.json,time myprog would export to the speedscope format if its exporter was registered properly and Landmarks.init () was called (which would normally be done by Landmarks_exports.init ())
• OCAML_LANDMARKS=format=speedscope,output=profile.json,time myprog would do nothing if the program does not call Landmarks.init ()
• OCAML_LANDMARKS=format=asdf,output=profile.json,time myprog would fail early because adsf is not a valid exporter name
• OCAML_LANDMARKS=output=profile.json,time myprog would use the default exporter (textual)
• myprog would not do any profiling if OCAML_LANDMARKS is unset

I'll amend the current PR to implement this.

Update: the solution actually is much simpler since the core library knows the list of possible exporters. The initialization done with Landmarks_exports.init () simply registers the expected exporters after the parsing of the environment variable.

[mjambon]

I think I'm done with this revision. It includes:

• support for a custom exporter that must be registered with Landmark.register_custom_exporter
• a new library named landmarks-exports that provides export to the Speedscope format - the user needs to call Landmarks_exports.init ()
• the core landmarks library has no new dependencies
• landmarks-exports depends on yojson
• the core landmarks library knows the names of the exporters that should be provided by landmarks-exports

Naming issue: the main module of the landmarks library is Landmark without an s. The library is landmarks-exports and its module is Landmarks_exports. I don't know if Landmark_exports is clearer. Or we can also use a completely different name.

[mjambon]

If we want to move toward an explicit Landmark.init function that allows registering exporters beforehand, one option is the following:

• move all of landmarks to landmarks2, but instead of let () = ..., expose let init () = ...
• create a compatibility landmarks package that only does let () = Landmark.init ()
• produce a clear error message when Landmark.init () is called a second time
• the new landmarks-exports library and other code that registers custom exporters depend only on landmarks2, requiring users to call an initialization function

Existing users of the landmarks library would see no difference. They would still get the same Landmark module and initialization would still happen automatically.

The main advantage of this setup is that users could register various custom exporters without the landmarks2 library having to know about them (unlike the latest implementation 99106f0).

Possible refinement to avoid runtime conflicts

• old landmarks becomes landmarks-core, provides Landmark.init
• new landmarks depends on landmarks-core and is just let () = Landmark.init ()
• landmarks2 also depends on landmarks-core and does nothing but declares a package conflict with landmarks to prevent a double call to init (because for example (libraries landmarks landmarks-exports) in a dune file would fail at runtime)
• landmarks-exports and alike depend on landmarks2

This would be useful if an external library is instrumented with landmarks but it's not obvious because it's an indirect dependency, and our local code starts to use custom landmarks exporters depending on landmarks2, but we're not realizing that we're still depending on landmarks.

This may be overkill, though.