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.

[nojb]

Thanks @mjambon for the explanations.

We discussed the issue with @mlasson. The conclusion was that we should keep the existing approach (no separate "init" function), and instead delay the resolution of the backend name until needed (in particular after module initialization).

• This allows the backend implementations to be completely independent of the landmarks library;
• Not requiring to call Landmark.init() preserves the property that it is possible to instrument code without modifying it at all.
• On the downside, if one makes a mistake when entering the name of the backend, the error will only be reported "later", but @mlasson thought this was not very serious.

[mjambon]

Here's a summary of what we have in the latest revision:

• support for format=XXX where XXX can be any name as long as it's registered in time; if not registered by the time of exporting, a non-fatal warning is printed.
• the landmarks-exports library requires a call to Landmarks_exports.register (). It provides the speedscope exporter and possibly more in the future.
• note the module name for the landmarks-exports library is Landmarks_exports whereas the module Landmark is singular (but the library name is pluralized as landmarks).

[nojb]

Let's put speedscope in the name, since if we need to add a different backend we can simply define a new library. Also, I switched the dash to an underscore in the public name (= ocamlfind name) so that it matches the .cma/.cmxa name.

Suggested change

	(name landmarks_exports)
	(public_name landmarks-exports)
	(name landmarks_speedscope)
	(public_name landmarks_speedscope)

[mjambon]

Also, I switched the dash to an underscore in the public name (= ocamlfind name) so that it matches the .cma/.cmxa name.

This causes the opam package to also have the name landmarks_speedscope because dune requires the package name to be a prefix of the public library name.

As a result, we ship the following opam packages:

• landmarks
• landmarks-ppx
• landmarks_speedscope

I don't really like the inconsistence between landmarks-ppx and landmarks_speedscope but I like to avoid differences between package name, library name, and module name. We probably shouldn't rename landmarks-ppx, though.

[nojb]

Oops, you are right! Let's keep landmarks-speedscope then, sorry for the noise.

[mjambon]

I'm done with all the proposed changes but make sure to check my comment about package names.

[mlasson]

Why ? Only to make it explicit that this is not Stdlib.exit ?
In that case, maybe we should just drop the "let open Landmark in" above and qualify all identifier (which is not that much here).

[mlasson]

You could maybe use on the two traversal functions in Landmark_graph here ?