Emancipate dynlink from compilerlibs by shindere · Pull Request #11996 · ocaml/ocaml

shindere · 2023-02-07T10:07:23Z

As is well-known, currently the dynlink other library depends on compilerlibs. More precisely, it has to embed a copy of compilerlibs, meaning that compilerlibs has to be compiled twice, its dynlink-embedded version being compiled with -for-pack.

This PR, based on a suggestion by @stedolan, is a sequence of changes that emancipate dynlink from compilerlibs, making it depend on a smaller module, CamlinternalDynlink, which is added to the standard library.

Roughly speaking, the involved changes are the following ones:

Translating the structured constants into their final representation at compile-time rather than at runtime, to simplify the way they are stored in cmo files and make their stored representaiton more independent from the compilers' internals
Changing the way identifiers are stored in cmo files, to use only standard types rather than their representation as used by the type-checker
Putting the definition of magic numbers under configure's control, to make it possible and easy to use it at several places in the build system without dupplicating code
Roughly speaking, moving code around and sometimes splitting modules into two parts (e.g. the Dll module), one part used for compilation and static linkingthat remains in bytecomp and the other used for dynamic loadin gof objects by both dynlink and the toplevels, moved to the CamlinternalDynlink standard library module.

Each of the two first changes has required a compielr bootstrap. After these two changes, the types used to represent CMO and CMA files only use standard OCaml types and do not rely on any type internal to the compiler.

Currently, the PR works, meaning that itpasses the testsuite in both lambda and flambda modes.

On my machine, not compiling compilerlibs twice saves ~6s of total build time, which goes down from 4m58,261s to 4m52,251s. The size of dynlink.cma is reduced from 4,1M to 112K and the size of dynlink.cmxa is divided by two, going down from 8K to 4K (these numbers feel surprisingly small to me).

I didn't undertake any diff testing so far because I wanted to gather some input first and make sure the version I'd use for diff testing would be as close as possible to what peope would agree to merge, especially given that rebasing this on 5.0 is non-trivial and could introduce errors, in particular because of the changes to the bytepackager module that happened after 5.0 had been branched.

Among the aspects that will IMO require some discussion, there is the way the Symtable module has been split. Currently, much of its code is duplicated and it'd certainly be good to avoid such a duplication as much as possible, given that, as far as symbol table is concerned, it may not be possible to make the toplevel as independent of compilerlibs as dynlink because it more heavily relies on the Lambda module, thus oon compiler internals.

Obviously this PR is best approached in a per-commit way.

nojb · 2023-02-07T10:11:02Z

the size of dynlink.cmxa is divided by two, going down from 8K to 4K (these numbers feel surprisingly small to me).

This file does not contain any code, you should look at the size of dynlink.a.

shindere · 2023-02-07T10:21:09Z

Nicolás Ojeda Bär (2023/02/07 02:11 -0800):

> the size of dynlink.cmxa is divided by two, going down from 8K to 4K (these numbers feel surprisingly small to me). This file does not contain any code, you should look at the size of `dynlink.a`.

My bad, sorry! Then the decrease in size is from 4,3M to 128K.

gasche · 2023-02-07T10:21:35Z

Translating the structured constants into their final representation at compile-time rather than at runtime, to simplify the way they are stored in cmo files and make their stored representaiton more independent from the compilers' internals.

This change sounds like something that would be very useful for camlboot. I would be interested in considering it as a separate buildup PR that could be discussed and reviewed separately.

shindere · 2023-02-07T10:24:32Z

Of course, one possibility is to split this PR in several smaller / easier to chew ones. The aim of this PR is to provide a complete story, it does not necessarily mean it has to be merged all at once, assuming it's appreciated.

nojb · 2023-02-07T10:32:03Z

Translating the structured constants into their final representation at compile-time rather than at runtime, to simplify the way they are stored in cmo files and make their stored representaiton more independent from the compilers' internals

Changing the way identifiers are stored in cmo files, to use only standard types rather than their representation as used by the type-checker

I think these two changes (at least) should be split off into separate PRs since they have independent interest and can be reviewed in a self-contained manner.

shindere · 2023-02-07T10:32:47Z

Gabriel Scherer (2023/02/07 02:21 -0800):

> Translating the structured constants into their final representation at compile-time rather than at runtime, to simplify the way they are stored in cmo files and make their stored representaiton more independent from the compilers' internals. This change sounds like something that would be very useful for [camlboot](https://github.com/Ekdohibs/camlboot).

Oh! Nice if it has another positive outcome!

I would be interested in considering it as a separate buildup PR that could be discussed and reviewed separately.

Shall I go ahead and split that out right now or rather wait a bit? One thing to keep in mind about this change is that it has an impact on `ocamlobjinfo`, more precisely on the way constants are displayed. Indeed, structured constants carry some type information which are used to display constants in a nice way. With this change, this type information is lost and the way constants are printed becomes slightly less friendly (well, in other words, untyped).

Octachron · 2023-02-07T10:34:38Z

What is the motivation for adding an internal dynlink module to the standard library? This move seems to go in the opposite direction of the rest of the PR. This is an internal module to the compiler that exposes internal implementation details of the compiler, ,which is not used by other modules in the standard library. Do we really want to make this module visible to all users of the standard library and start offering backward-compatibility guarantee for this module?

shindere · 2023-02-07T10:36:31Z

Nicolás Ojeda Bär (2023/02/07 02:32 -0800):

> - Translating the structured constants into their final representation at compile-time rather than at runtime, to simplify the way they are stored in cmo files and make their stored representaiton more independent from the compilers' internals > - Changing the way identifiers are stored in cmo files, to use only standard types rather than their representation as used by the type-checker I think these two changes (at least) should be split off into separate PRs since they have independent interest and can be reviewed in a self-contained manner.

Okay, will do.

shindere · 2023-02-07T10:39:54Z

Florian Angeletti (2023/02/07 02:34 -0800):

What is the motivation for adding an internal dynlink module to the standard library? This move seems to go in the opposite direction of the rest of the PR. This is an internal module to the compiler that exposes internal implementation details of the compiler, ,which is not used by other modules in the standard library. Do we really want to make this module visible to all users of the standard library and start offering backward-compatibility guarantee for this module?

Well, I think the high level goal was to have a place from which it's easy to share code. It was part of Stephen's suggestion to add thisshared module to the standard library but it may be possible to add it to the compiler itself, I guess it would be enough to achieve the purpose.

nojb · 2023-02-07T10:49:04Z

start offering backward-compatibility guarantee for this module?

I thought we didn't offer any guarantees at all for the camlinternal* modules... Their API is not even documented anywhere.

hhugo · 2023-02-07T10:37:55Z

bytecomp/dll.mli

   when starting the running program. *)
 val init_toplevel: string -> unit
-
-val reset: unit -> unit


The fact that it is unused doesn't mean it should be removed. I think we have a reset function every time there is a mutable global state.

hhugo · 2023-02-07T10:39:54Z

bytecomp/symtable.ml

+  | Const_block(tag, fields) ->
+      let block = Obj.new_block tag (List.length fields) in
+      let pos = ref 0 in
+      List.iter


List.iteri ?

hhugo · 2023-02-07T10:46:51Z

tools/dumpobj.ml

    Reloc_literal sc -> print_obj sc; printf "\n"
-  | Reloc_getglobal id -> printf "require    %s\n" (Ident.name id)
-  | Reloc_setglobal id -> printf "provide    %s\n" (Ident.name id)
+  | Reloc_getglobal id | Reloc_getpredef id ->


Maybe a newline between the or pattern so that the printf can be aligned with others and we can check the number of spaces is correct.

hhugo · 2023-02-07T10:47:38Z

otherlibs/dynlink/byte/dynlink.ml


  let unsafe_get_global_value ~bytecode_or_asm_symbol =
-    let id = Ident.create_persistent bytecode_or_asm_symbol in
+    let id = bytecode_or_asm_symbol in


this binding is no longer useful

hhugo · 2023-02-07T10:48:08Z

otherlibs/dynlink/byte/dynlink.ml

  let fold_initial_units ~init ~f =
    List.fold_left (fun acc (comp_unit, interface) ->
-        let id = Ident.create_persistent comp_unit in
+        let id = comp_unit in


This binding is no longer useful

hhugo · 2023-02-07T11:12:50Z

bytecomp/meta.mli

 type bytecode
 external reify_bytecode :
-  bytes array -> Instruct.debug_event list array -> string option ->
+  bytes array -> 'a -> string option ->


I think you should try again to use an abstract type and resolve the issue in toplevel.ml using the newType.Equal

shindere · 2023-02-07T12:54:42Z

uJust submitted #11997 for the translation of structured constants bit.

Since changing the representation of identifiers as done here touches
code which is also changed by #11997, I plan to wait until we are done
with #11997 and it (hopefully) gets merged before opening the PR
about identifiers.

Octachron · 2023-02-07T12:58:57Z

@nojb , we don't offer support for the internal runtime API either, this still doesn't stop us from caring about eventual breakage there. Moreover, for a module with function sthat look useful in the standard library, there is a non-zero risk that people start to use it independently on how scary is the module name.

@shindere: if the purpose was just to make it is easier to build a proof-of-concept with the module in the standard library, this is perfectly fine. However, the module should really be moved to its own separate library beyond the proof-of-concept stage.

shindere · 2023-02-07T12:59:11Z

Many thanks for all your feedback Hugo and for having taken the time to review and done so in such a prompt way. All your suggestions make a lot of sense to me and I will make sure to integrate them as it goes and to give feedback for each of them (not all of them have been integratedyet). hhugo (2023/02/07 03:17 -0800):

+(* Translate structured constants *) + +let rec transl_const = function + Const_base(Const_int i) -> Obj.repr i + | Const_base(Const_char c) -> Obj.repr c + | Const_base(Const_string (s, _, _)) -> Obj.repr s + | Const_base(Const_float f) -> Obj.repr (float_of_string f) + | Const_base(Const_int32 i) -> Obj.repr i + | Const_base(Const_int64 i) -> Obj.repr i + | Const_base(Const_nativeint i) -> Obj.repr i + | Const_immstring s -> Obj.repr s + | Const_block(tag, fields) -> + let block = Obj.new_block tag (List.length fields) in + let pos = ref 0 in + List.iter `List.iteri` ?

Yes, absolutely, you are right. Thanks. As you can see this has been integrated to #11997.

shindere · 2023-02-07T13:03:12Z

Florian Angeletti (2023/02/07 04:59 -0800):

@shindere: if the purpose was just to make it is easier to build a proof-of-concept with the module in the standard library, this is perfectly fine. However, the module should really be moved to its own separate library beyond the proof-of-concept stage.

What I understand is that it's important that allthe clients use the very same symbol table. I think it is for this reasoon that Stephen has suggested a module in the standard library. I, personnally, am not attached to any particular solution so whatever will work and make people happy will be okay for me.

shindere · 2023-02-22T20:02:01Z

Now that #11997 has been merged, the present PR has been rebased on top of
it to keep track of the
complete not-yet-merged emancipation of dynlink from compilerlibs history.

Simultaneously, #12031 has just been opened to propose a second bit
of this history that may be worth being discussed and reviewed in isolation.

lthls · 2023-02-24T15:38:34Z

I've had an occasion to discuss with @shindere on this subject, and for clarification I'll restate my position here: I'm 100% in favour of reducing the dependencies of Dynlink, but I think that as a library that manipulates artifacts produced by the compiler it will always naturally depend on (a subset of) compiler-libs, and we shouldn't try to hide that.
If the only thing we want is to stop building the special copy of compiler-libs for dynlink, we could reconsider linking against compiler-libs normally. Most users should have namespaced their own modules correctly by now, so I think the risks of conflicts are low. And we could always consider namespacing compiler-libs ourselves in a more systematic way (like we did for the stdlib) if we still want to support users with conflicting modules.

shindere · 2023-02-24T15:55:52Z

Many thanks for having taken the time to state your position publicly, Vincent. I don't know how exactly we should proceed from here, whether e.g. we should organise a short dedicated online meeting, but what I can tell is that it would help if we could make a decision, something I can be reasonably sure we all agree on before continuing to invest time in the project at the risk of doing it in a direction which ends up not being the right one.

lpw25 · 2023-02-24T17:20:56Z

I'd just like to register my disagreement with Vincent. The dependency on compiler-libs has always been a huge pain, and seems entirely unnecessary. I strongly support this PR.

shindere · 2023-02-24T17:37:25Z

Leo White (2023/02/24 09:21 -0800):

I'd just like to register my disagreement with Vincent. The dependency on compiler-libs has always been a huge pain, and seems entirely unnecessary. I strongly support this PR.

Many thanks, Leo. The devil is in the details, so I am fully ocnvinced that some parts of the history as it currently is are ugly and will need to be improved, that's where I'd really appreciate any feedback I can receive.

shindere · 2024-05-16T09:10:53Z

Leo White (2024/04/26 07:34 -0700):

Can we just press merge now, or is there something else we are waiting for?

The PR got discussed during yesterday's triaging meeting. There was the request for a review to make sure that the refactoring does preserve the previous behaviour. It has also been suggested that the added module somehow embeds references to the code fragments it duplicates, either as comments or as `#line` directives. I felt slightly reluctant to do so, first because i am slightly unconvinced of the interest and exhausted of working on this PR, second because the actual author of this portion of the code is @stedolan. During the triaging meeting, @gasche also pushed for @damiendoligez to do the review, rather than us finding another reviewer. After having slept on all that (notmuch, though), I came to the idea that the best way to go might be that @damiendoligez and @stedolan go over the code together, in a synchronous way. Anyway, thanks for your interest, Leo!

gasche · 2024-05-16T09:28:17Z

Yes, my position is that reviewing this stuff is a pain, and that we should limit the number of people that we send do it. Either @damiendoligez is concerned with something and he should clarify, or he actually believes that the code is fine and he should approve. It is a waste to send someone completely new to do all the work of getting into the water again.

On the other hand, personally I find the request to have an explicit comment about code being duplicated rather reasonable -- this sounds like something that could be useful later on, to improve our chances of thinking of synchronizing the code. If this is too cumbersome for @shindere, would someone else be willing to do it?

shindere · 2024-05-27T15:37:23Z

I have just rebased this PR on latest trunk to resolve the (trivial)
conflicts in configure and otherlibs/dynlink/Makefile.

I also took this opportunity to update the associated Changes entry by
moving it to the right place (that is, from 5.2 to 5.3) and crediting
the reviewers.

As the discussion is quite long, I may very well have forgotten somebody and
would appreciate to be told, so that all the reviewers can be
credited as deserved.

gasche · 2024-05-27T16:11:04Z

You listed me in the entry but I don't remember reviewing the current iteration of this PR. It started as a much larger PR and I reviewed part of it (#12031), but not the current version/PR, you should remove the mention of me. Looking at the discussion again, I only see recognizable review activity from @hhugo and @damiendoligez.

shindere · 2024-05-27T17:18:23Z

I wanted to be generous in crediting and mention all those who helped the discussion to progress in one way or another but I went with your suggestion.

shindere · 2024-06-06T07:54:57Z

I just merged this PR from @Octachron. It annotates the Dynlink_symtable module with #line directives showing where the code comes from and has a script to regenerate it from its source modules, which mechanically proves that it only duplicates code.

Big thanks to Florian for this very nice contribution.

With this addition, I hope all the blockers have been removed and this can (finally) be considered for merging. :)

Octachron · 2024-06-06T08:00:08Z

Note that some functions of the module Dynlink_symtable are specialized version of functions from the Dll and Symtable, so there is a bit a new code, but those specialization are now commented.

This commit introduces a dedicated configured Dynlink_config module that contains only the configuration values needed by dynlink. Co-authored-by: Stephen Dolan <sdolan@janestreet.com>

damiendoligez

I got a briefing from the PR authors, then reviewed again, this time with confidence that I understand what's going on.

A few small changes and this is good to go.

damiendoligez · 2024-05-28T09:10:39Z

otherlibs/dynlink/byte/dynlink.ml

          else begin
            seek_in ic compunit.cu_debug;
-            [| (Compression.input_value ic : Instruct.debug_event list) |]
+            [| (Compression.input_value ic) |]


Unmarshalling without a type constraint is a big red flag, and it's easy enough to add in this case.

Suggested change

[| (Compression.input_value ic) |]

[| (Compression.input_value ic : instruct_debug_event list) |]

damiendoligez · 2024-06-17T14:42:16Z

tools/sync_dynlink.ml

+(*                                                                        *)
+(*             Florian Angeletti, projet Cambium, Inria Paris             *)
+(*                                                                        *)
+(*   Copyright 2021 Institut National de Recherche en Informatique et     *)


Suggested change

(* Copyright 2021 Institut National de Recherche en Informatique et *)

(* Copyright 2024 Institut National de Recherche en Informatique et *)

damiendoligez · 2024-06-17T14:46:37Z

tools/sync_dynlink.mli

+(*                                                                        *)
+(*             Florian Angeletti, projet Cambium, Inria Paris             *)
+(*                                                                        *)
+(*   Copyright 2021 Institut National de Recherche en Informatique et     *)


Suggested change

(* Copyright 2021 Institut National de Recherche en Informatique et *)

(* Copyright 2024 Institut National de Recherche en Informatique et *)

Octachron · 2024-06-17T14:56:29Z

Before merging, there are few line directives which should be updated, and the symtable.ml might use a very small bit of refactoring to reduce the imprint of the formatting module used. (I have discussed those changes with @shindere this afternoon.)

Co-authored-by: Stephen Dolan <sdolan@janestreet.com>

This commit, kindly provided by Florian Angeletti as part of his review of PR ocaml#11996, adds line directives to otherlibs/dynlink/byte/dynlink_symtable.ml to make it possible to verify where each fragment of that file comes from. (See next commit for tooling to automate this verification.)

Also kindly provided by Florian Angeletti as part of his review of PR ocaml#11996, this commit automates the verification that otherlibs/dynlink/byte/dynlink_symtable.ml can indeed be re-generated from its bytecomp sources. Run the verification with: make sync_dynlink

shindere · 2024-06-18T06:33:38Z

Thanks a lot for the approval! And thanks a lot, too, for the very relevant suggestions which are taken into account by the updated version of the PR. The type annotation was not possible at the time when the initial change was done, hence its mere removal but it's definitely better style to restore it and it addresses a request by @hhugo earlier in the discussion.

shindere · 2024-06-18T06:34:33Z

Florian Angeletti (2024/06/17 07:56 -0700):

Before merging, there are few line directives which should be updated, and the `symtable.ml` might use a very small bit of refactoring to reduce the imprint of the formatting module used. (I have discussed those changes with @shindere this afternoon.)

This has been integrated, too.

shindere force-pushed the emancipate-dynlink-from-compilerlibs branch from a3cec01 to 4e61d24 Compare February 7, 2023 10:08

hhugo reviewed Feb 7, 2023

View reviewed changes

shindere mentioned this pull request Feb 7, 2023

Translate structured constants into their Obj.t representation at compile rather than link time #11997

Merged

shindere force-pushed the emancipate-dynlink-from-compilerlibs branch from 4e61d24 to c4dc1d0 Compare February 22, 2023 19:27

shindere mentioned this pull request Feb 22, 2023

Use a dedicated type to represent global identifiers in CMO files #12031

Merged

shindere force-pushed the emancipate-dynlink-from-compilerlibs branch 2 times, most recently from c13e7fd to 1f80a01 Compare February 23, 2023 07:40

shindere mentioned this pull request Jul 26, 2023

Simplify dynamic bytecode loading by having the runtime append RETURN 1 #12430

Merged

stedolan mentioned this pull request Sep 22, 2023

Refactor Dynlink startup to avoid parsing bytecode sections twice #12599

Merged

This was referenced Sep 25, 2023

Minor dynlink cleanups #12602

Merged

Remove the dependency of the cmo and cmxs file formats on compiler internals #12611

Merged

shindere force-pushed the emancipate-dynlink-from-compilerlibs branch 2 times, most recently from 911539f to 8d35c9f Compare May 27, 2024 15:33

shindere force-pushed the emancipate-dynlink-from-compilerlibs branch from 8d35c9f to 97e00cd Compare May 27, 2024 17:16

dynlink: release dependency on compilerlibs' Config module

5dd2ca2

This commit introduces a dedicated configured Dynlink_config module that contains only the configuration values needed by dynlink. Co-authored-by: Stephen Dolan <sdolan@janestreet.com>

shindere force-pushed the emancipate-dynlink-from-compilerlibs branch from 249023a to 13571f8 Compare June 17, 2024 10:40

damiendoligez approved these changes Jun 17, 2024

View reviewed changes

shindere and others added 4 commits June 18, 2024 08:08

dynlink: finish releasing dependency on compilerlibs

dff4288

Co-authored-by: Stephen Dolan <sdolan@janestreet.com>

Stop building the dynlink_compilerlibs pack

9c04cd0

shindere force-pushed the emancipate-dynlink-from-compilerlibs branch from f8ac549 to 1244e1a Compare June 18, 2024 06:21

shindere merged commit f1c2857 into ocaml:trunk Jun 18, 2024

dra27 mentioned this pull request Oct 21, 2024

Correctly process otherlibs/dynlink/dynlink_config.ml in tools/bump-magic-numbers #13562

Merged

stedolan mentioned this pull request Nov 12, 2024

Parallelise building dynlink_compilerlibs oxcaml/oxcaml#3250

Merged

shindere mentioned this pull request Jan 6, 2025

[internal] warning 70 (.ml without .mli file) in otherlibs/dynlink? #12780

Closed

dra27 mentioned this pull request Jan 20, 2025

Move Dynlink into the Standard Library and use it in the toplevel #13745

Open

dra27 mentioned this pull request Aug 28, 2025

[Relocatable] trunk combined branch dra27/ocaml#92

Open

This was referenced Sep 15, 2025

Relocatable OCaml - --with-relative-libdir #14244

Merged

Relocatable OCaml - all your switch are belong to us #14247

Closed

dra27-js mentioned this pull request Feb 12, 2026

Backport (#11996) "Emancipate dynlink from compilerlibs" oxcaml/oxcaml#5420

Open

	[\| (Compression.input_value ic) \|]
	[\| (Compression.input_value ic : instruct_debug_event list) \|]

	(* Copyright 2021 Institut National de Recherche en Informatique et *)
	(* Copyright 2024 Institut National de Recherche en Informatique et *)

Conversation

shindere commented Feb 7, 2023

Uh oh!

nojb commented Feb 7, 2023

Uh oh!

shindere commented Feb 7, 2023 via email

Uh oh!

gasche commented Feb 7, 2023

Uh oh!

shindere commented Feb 7, 2023

Uh oh!

nojb commented Feb 7, 2023

Uh oh!

shindere commented Feb 7, 2023 via email

Uh oh!

Octachron commented Feb 7, 2023

Uh oh!

shindere commented Feb 7, 2023 via email

Uh oh!

shindere commented Feb 7, 2023 via email

Uh oh!

nojb commented Feb 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shindere commented Feb 7, 2023

Uh oh!

Octachron commented Feb 7, 2023

Uh oh!

shindere commented Feb 7, 2023 via email

Uh oh!

shindere commented Feb 7, 2023 via email

Uh oh!

shindere commented Feb 22, 2023

Uh oh!

lthls commented Feb 24, 2023

Uh oh!

shindere commented Feb 24, 2023 via email

Uh oh!

lpw25 commented Feb 24, 2023

Uh oh!

shindere commented Feb 24, 2023 via email

Uh oh!

shindere commented May 16, 2024 via email

Uh oh!

gasche commented May 16, 2024

Uh oh!

shindere commented May 27, 2024

Uh oh!

gasche commented May 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shindere commented May 27, 2024 via email

Uh oh!

shindere commented Jun 6, 2024

Uh oh!

Octachron commented Jun 6, 2024

Uh oh!

damiendoligez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Octachron commented Jun 17, 2024

Uh oh!

shindere commented Jun 18, 2024 via email

gasche commented May 27, 2024 •

edited

Loading