-
Notifications
You must be signed in to change notification settings - Fork 24
Support using landmarks with OCaml 5 domains #46
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Nathan Rebours <nathan.rebours@ocamlpro.com>
And factorize some code to prepare for landmark_state_ocaml5
…specific in the parallel case Remove duplicate def on `clock` and rename `get_landmark_body` to `get_ds_landmark`
|
Note: we also updated the PPX to work with the latest |
|
That looks super nice, I'll try to review ASAP. |
Small thing: when the value binding is rebuilt, |
|
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. |
|
Sure thing. I'll get right on it. |
Keep constraint on value binding in ppx
|
Thanks for the fix @brandonzstride ! |
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.mlfile 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_exitfunction 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.mland thelandmark_state*.mlfiles were moved to a separate fileutils.ml.Constraints
For this to work:
Every child domain has to terminate before the parent domain (so that we can collect its measurements).
Each domain has to run on a separate physical core (for the
clockfunction 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
Domainmodule extended with these functions.Implementation Details
A change we had to make is splitting the
landmarktype into two types:landmark: which is abstract and defined differently for eachlandmark_state*file.landmark_body: which is the original record type and is now defined inutils.ml.In
landmark_state_ocaml4.ml,landmarkandlandmark_bodyare the same, while inlandmark_state_ocaml5.ml,landmarkis of the typelandmark_body Lazy.t DLS.key.The reason for this change is:
landmark_bodyfor each domain (through the_ DLS.key)._ 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.keyare 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 thelandmark.mlmodule 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.