Skip to content

Move compressed marshaling away from the stdlib and into the compilers (trunk version)#12758

Merged
xavierleroy merged 6 commits intoocaml:trunkfrom
xavierleroy:no-marshal-compression-flag-trunk
Jan 26, 2024
Merged

Move compressed marshaling away from the stdlib and into the compilers (trunk version)#12758
xavierleroy merged 6 commits intoocaml:trunkfrom
xavierleroy:no-marshal-compression-flag-trunk

Conversation

@xavierleroy
Copy link
Copy Markdown
Contributor

@xavierleroy xavierleroy commented Nov 21, 2023

This is the "forward port" of #12734 from the 5.1 branch to the trunk.

Besides adapting to recent changes in extern.c and in the top Makefile, this PR also proposes a different way to auto-link compressed marshaling support when ocamlcommon.cmxa is used:

  • a new C library libcomprmarsh.a is built (from runtime/zstd.n.o) and installed along with libasmrun.a
  • ocamlcommon.cmxa includes instructions to link with -lcomprmarsh and then with -lzstd.

@dra27 and I did not want to follow this approach in a minor release like 5.1.1, as it introduces a new installed file, which could upset packagers. But it seems appropriate for a major release like 5.2.

This is the only significant change from #12734. Compare 833ca02 (this PR) with 9398511 (the 5.1.1 solution).

@xavierleroy xavierleroy changed the title Move compressed marshaling away from the stdlib and into the compilers (forward port) Move compressed marshaling away from the stdlib and into the compilers (trunk version) Nov 21, 2023
@xavierleroy
Copy link
Copy Markdown
Contributor Author

xavierleroy commented Nov 21, 2023

a new C library libcomprmarsh.a is built (from runtime/zstd.n.o) and installed along with libasmrun.a

Late thought: libcomprmarsh.a could probably just as well reside in compilerlibs/ , both during build and after installation. Keep this possibility in mind during review.

@xavierleroy xavierleroy force-pushed the no-marshal-compression-flag-trunk branch from b00370c to f26bbf6 Compare November 24, 2023 10:25
@xavierleroy xavierleroy mentioned this pull request Nov 24, 2023
Merged
@nojb
Copy link
Copy Markdown
Contributor

nojb commented Nov 24, 2023

this PR also proposes a different way to auto-link compressed marshaling support

I understand this choice corresponds to the second bullet point in #12734 (comment). Was this chosen because of simplicity, to avoid changing the way the compiler works? And regardless of this PR, do we agree that the change mentioned in the first bullet point would be an improvement in general?

@xavierleroy
Copy link
Copy Markdown
Contributor Author

xavierleroy commented Nov 25, 2023

I understand this choice corresponds to the second bullet point in #12734 (comment).

Yes! You're right I should have been more explicit.

Was this chosen because of simplicity, to avoid changing the way the compiler works? And regardless of this PR, do we agree that the change mentioned in the first bullet point would be an improvement in general?

For reference: the first bullet point is about changing ocamlopt -a so that .o arguments are added to the generated .a code archive instead of being recorded as "to be linked later" in the generated .cmxa archive descriptor.

I'm not 100% comfortable with this change because there's no way to change ocamlc -a in the same manner: it does not create a .a code archive, so there's no place to store the .o arguments... So, we would be adding a feature that is usable only for natively-compiled libraries. I really think all OCaml libraries should be compiled both to native-code and to bytecode, and remain usable in both contexts.

At any rate, the "first bullet" approach that is followed in this PR (an explicit libcomprmarsh.a library, autolinked from ocamlcommon.cmxa) is a step towards an otherlibs/compression library providing compressed marshaling and other data compression facilities to the users. (Stay tuned!)

@xavierleroy xavierleroy force-pushed the no-marshal-compression-flag-trunk branch from 6454050 to 535d4e4 Compare November 27, 2023 15:11
@xavierleroy xavierleroy mentioned this pull request Dec 3, 2023
Closed
@hhugo hhugo mentioned this pull request Dec 4, 2023
Closed
11 tasks
@Octachron Octachron added this to the 5.2 milestone Dec 13, 2023
@gasche gasche added this to the 5.2 milestone Dec 13, 2023
@gasche
Copy link
Copy Markdown
Member

gasche commented Dec 13, 2023

@hhugo is there a possibility that you would be interested in reviewing this PR?

@damiendoligez damiendoligez self-requested a review January 10, 2024 14:23
damiendoligez
damiendoligez approved these changes Jan 25, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@damiendoligez damiendoligez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, I found only a typo.

@damiendoligez
Copy link
Copy Markdown
Member

damiendoligez commented Jan 25, 2024

Late thought: libcomprmarsh.a could probably just as well reside in compilerlibs/ , both during build and after installation. Keep this possibility in mind during review.

IIUC, this would make sense with the current version, but you'd have to move it back to provide compression for general marshalling, right?

@xavierleroy
Reduce the dependencies of native-code Dynlink
d74c57c 
Currently, both the bytecode version and the native-code version of
Dynlink drag in many modules from the compiler, including some that
use compressed marshaling.  This causes users of dynlink.cmxa to
depend on -lzstd systematically.

Actually, the native-code version of Dynlink needs much fewer compiler
modules, none of which require compressed marshaling.

This commit just shrinks the list of compiler modules included in
dynlink.cmxa, thus cutting the dependency on -lzstd.

(cherry picked from commit c6d6e8b)
@xavierleroy xavierleroy force-pushed the no-marshal-compression-flag-trunk branch 2 times, most recently from 2cb97c9 to bee4cc8 Compare January 26, 2024 13:11
xavierleroy and others added 5 commits January 26, 2024 14:39
@xavierleroy
Move the functions that use ZSTD compression to a separate zstd.c file
ae58bd4 
These functions used to be in extern.c and intern.c.
Now, they are in zstd.c and connect themselves to extern.c and intern.c
via hooks activated by a new primitive `caml_zstd_initialize`.

(cherry picked from commit aae3ad5)
@dra27 @xavierleroy
Move compressed marshaling from stdlib/marshal to utils/compression
115f8f6 
- Removes the Marshal.Compression constructor and the Marshal.compression_supported function, in preparation for a different user-facing API later.
- Introduce the Compression module in the compiler sources, providing
  simple compressed marshaling support (just `output_value` and `input_value`
  for now)
- Call caml_zstd_initialize from Compression.
- Add missing type constraint in otherlibs/dynlink/byte/dynlink.ml
- Update configure messages accordingly.

(cherry picked from commit 5068559)
(cherry picked from commit 02b0d07)
(cherry picked from commit e5c9d45)
(cherry picked from commit 6db7ccb)
(cherry picked from commit 8abed5e)

Co-authored-by: Xavier Leroy <xavier.leroy@college-de-france.fr>
@xavierleroy
ocamlopt: do not link -lzstd nor zstd.n.o by default, only for compil…
6a2e09d 
…erlibs

To this end, we produce a new library libcomprmarsh.a, containing
runtime/zstd.n.o, and attach both -lcomprmarsh and -lzstd to
ocamlcommon.cmxa.

(cherry picked and much improved from commit
 9398511)
@xavierleroy
Bootstrap after removal of Marshal.Compression flag
4909b56
@xavierleroy
Changes entry for 12758
735e26b
@xavierleroy xavierleroy force-pushed the no-marshal-compression-flag-trunk branch from b8f21c7 to 735e26b Compare January 26, 2024 15:21
@xavierleroy xavierleroy merged commit 0efbcba into ocaml:trunk Jan 26, 2024
@xavierleroy
Copy link
Copy Markdown
Contributor Author

xavierleroy commented Jan 26, 2024

Thanks for the feedback! I merged without further delay because the bootstrap in this PR is a pain to re-do properly. (It is absolutely necessary to disable ZSTD compression until the new compilers are stable.) I will now attempt a backport to 5.2.

@xavierleroy
Copy link
Copy Markdown
Contributor Author

xavierleroy commented Jan 26, 2024

Cherry-picked to 5.2. The bootstrap had to be redone for 5.2, so I cherry-picked each commit individually. We end up with cherry-picks of cherry-picks... I hope everything is OK. That's what happens when urgent PRs are not reviewed before a release branch is cut.

@xavierleroy xavierleroy deleted the no-marshal-compression-flag-trunk branch January 26, 2024 17:02
@shym shym mentioned this pull request Jan 31, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@damiendoligez damiendoligez damiendoligez approved these changes

Assignees

@dra27 dra27

Labels

None yet

Projects

None yet

Milestone

5.2

Development

Successfully merging this pull request may close these issues.

6 participants

@xavierleroy @nojb @gasche @damiendoligez @dra27 @Octachron