Skip to content

Conversation

@hra687261
Copy link

Hello,

We (OCamlPro) propose this PR that makes it possible to use landmarks with multiple domains on OCaml >= 5.
We tried to minimize the difference with the original code, maintain the same interface, and avoid moving too much code around.

Structure changes

The PR moves the global references and top-level values in the landmark.ml file into two separate files:

  • landmark_state_ocaml4.ml: which reproduces the original behavior of the library and is used with OCaml < 5.

  • landmark_state_ocaml5.ml: which puts these references into a record of which an instance is created for each domain and is used with OCaml >= 5.

    Similarly to what is done when using a single domain, measurements are accumulated for each domain when its Domain.at_exit function is executed. When the main domain exits, instead of simply exporting its own measurements, it exports the sum of the measurements of all its child domains.

Some type definitions that are needed by both landmark.ml and the landmark_state*.ml files were moved to a separate file utils.ml.

Constraints

For this to work:

  1. Every child domain has to terminate before the parent domain (so that we can collect its measurements).

  2. Each domain has to run on a separate physical core (for the clock function to work properly).

    This can notably be done using the ocaml-processor library.

We also wrote for this purpose domainpc which is a library that offers domain spawning functions that ensure that the domains run on separate cores. It also offers the same interface as the standard library Domain module extended with these functions.

Implementation Details

A change we had to make is splitting the landmark type into two types:

  • landmark: which is abstract and defined differently for each
    landmark_state* file.
  • landmark_body: which is the original record type and is now defined in utils.ml.

In landmark_state_ocaml4.ml, landmark and landmark_body are the same, while in landmark_state_ocaml5.ml, landmark is of the type landmark_body Lazy.t DLS.key.

The reason for this change is:

  • To have a different instance of landmark_body for each domain (through the _ DLS.key).
  • To be able to initialize the instances within the child domains rather than the parent domain (through the _ Lazy.t).
    This is done notably so that when landmarks are initialized, they can use the dummy nodes from the child domains and not those of the parent domain.

Limitations

One issue with this approach is that values of type _ DLS.key are kept in a global list, and thus never de-allocated for the lifetime of the domain. This means that in the OCaml5 mode, landmarks are never freed. It doesn't seem to be an issue for landmarks generated with the ppx as they are themselves global variables, but the weak table seems to have been added intentionally, and we are not sure if that would be an issue in certain contexts.

When running in single-domain mode in OCaml 5, we were thinking that we can offer a functor that takes a landmark_state* module and produces an instance of the landmark.ml module which would allow having exactly the same behavior with OCaml 4 and OCaml 5 when only one domain is used.

We'd be happy to make this change but would like the opinions of the maintainers of the library first.

Regards.

@redianthus
Copy link

Note: we also updated the PPX to work with the latest ppxlib version. :)

@mlasson
Copy link
Collaborator

mlasson commented Jan 15, 2026

That looks super nice, I'll try to review ASAP.

@brandonzstride
Copy link

Note: we also updated the PPX to work with the latest ppxlib version. :)

Small thing: when the value binding is rebuilt, pvb_constraint needs to be retained. I ran into this in my PR #45. Without this change, the present PR cannot build the two code examples in #45. It can be fixed with a small two-line change at ppx/mapper.ml lines 280 and 283.

@redianthus
Copy link

redianthus commented Jan 23, 2026

Would you be OK about making a PR at https://github.com/hra687261/landmarks/tree/ocaml5 to fix it? So that you can get credit for it - I'm suggesting to do it this way because I guess it is easier to make your fix on this PR rather than rebasing this bigger PR on your fix.

@brandonzstride
Copy link

Sure thing. I'll get right on it.

@hra687261
Copy link
Author

Thanks for the fix @brandonzstride !

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.

4 participants