Move compressed marshaling away from the stdlib and into the compilers (5.1 version) by xavierleroy · Pull Request #12734 · ocaml/ocaml

xavierleroy · 2023-11-12T17:17:17Z

This is the second attempt at fixing the problem reported in #12562 concerning the compressed marshaling facility introduced in OCaml 5.1 by #12006.

The first attempt #12705 kept the API introduced in #12006 (an extra flag Marshal.Compression in the Marshal standard library module) and tried to make sure that the ZSTD library is dynamically linked with ocamlopt-generated executables only if the Marshal module is actually used. This failed because of limitations of some system linkers.

The present attempt removes the Marshal.Compression API and makes compressed marshaling unavailable via the standard library and private to the OCaml compilers. A new module Compression is added to the compilers and to compiler-libs, and used to compress and uncompress the same compilation artefacts as in #12006. (Unless ZSTD is not available or configured out, in which case plain marshaling is used.)

The implementation reuses parts of the first attempt #12705:

The ZSTD-specific C code is moved to runtime/zstd.c and hooks into extern.c and intern.c when activated.
The bytecode interpreter ocamlrun and executables produced with ocamlc -custom always include zstd.c and always dynamically link with ZSTD. (Doing otherwise complicates coldstart and bootstrap too much.)

What's new in this second attempt:

Native-code executables produced by ocamlopt do NOT include zstd.c and do NOT link with ZSTD, unless they use compiler-libs (see next).
zstd.c and -lzstd are automatically included when linking with ocamlcommon.cmxa, the first part of native-code compiler-libs. This makes them usable from ocamlc.opt, ocamlopt.opt, and any native-code program that uses compiler-libs and therefore needs to work with compressed compilation artifacts.

This PR is against 5.1, so that it can be integrated in release 5.1.1. Two points of attention:

Accepting this PR means that we introduce an API incompatibility between 5.1.1 and 5.1 (the removal of the Marshal.Compression flag). IIRC, we never did this (a minor release with an incompatible API) before. The working group composed of @dra27, @Octachron, @nojb and I think this is acceptable given the circumstances. This will have to be confirmed at the next developers meeting.
The implementation of this PR was intentionally kept small and a bit hackish to minimize the changes in 5.1.1. If it is accepted, we (the working group) have ideas to clean up the implementation and perhaps make compressed marshaling accessible to users again (as a separate library in otherlibs/, explicitly linked, like the Unix library) in time for 5.2.

Already done in trunk.

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.

Manual cherry-pick from 7ed7268 . Co-authored-by: David Allsopp <david.allsopp@metastack.com>

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`.

- Removes the Marshal.Compression constructor, in preparation for explicit API in 5.2 - Add just to_channel (for now) to ocamlcommon - caml_zstd_initialize called from Compression for now.

…value` The "flags" argument was always [], so remove it. This avoids an Obj.magic in compression.ml.

Under the hood, it's just `Stdlib.input_value`. However, it documents the places in the compiler where we expect to read compressed marshaled data.

…erlibs We attach -lzstd to ocamlcommon.cmxa and insert zstd.n.o in ocamlcommon.a. (The latter is a hack, to be replaced by something better eventually.)

xavierleroy · 2023-11-12T17:44:25Z

compilerlibs/Makefile.compilerlibs

+compilerlibs/ocamlcommon.cmxa: $(COMMON_CMI) $(COMMON:.cmo=.cmx) $(ZSTD_OBJ)
+	$(V_LINKOPT)$(CAMLOPT) -a -linkall -o $@ \
+                               $(ZSTD_FLAGS) $(COMMON:.cmo=.cmx)
+	$(V_MKLIB)$(ARCMD) r compilerlibs/ocamlcommon.$(A) $(ZSTD_OBJ)


This creative use of ar is due to @dra27. If we just add zstd.n.o to the ocamlopt -a command line, this file name is recorded in ocamlcommon.cmxa and passed to the system linker when the latter file is used, but the system linker will not find it and will not search for it in the -L search path. Instead, we include zstd.n.o and its contents in the ocamlcommon.a archive, where the system linker will find it reliably.

It's unclear whether this hack can work with the MSVC librarian lib. Good thing we're not supporting MSVC again yet.

To make things cleaner in the future, we can either

Change ocamlopt -a so that all .o files given on the command line are included in the generated .a file. The current behavior (of recording the file names in the .cmxa file) is useless anyway.

Put zstd.n.o in a proper .a archive, and install this archive. This would be the case if we make an otherlibs that exposes compressed marshaling to users.

kit-ty-kate

Successfully tested on macOS, FreeBSD and Alpine Linux. Thank you so much!

xavierleroy · 2023-11-17T10:26:45Z

The incompatible API change (removal of Marshal.Compression flag and Marshal.compression_available function) was approved at this week's developer meeting. @damiendoligez promised a review of this PR.

damiendoligez

Looks good modulo two nitpicks.

configure.ac

otherlibs/dynlink/byte/dynlink.ml

damiendoligez · 2023-11-20T14:26:28Z

Because #12006 was released, you'll also need a Changes entry for this PR.

- Add type constraint in dynlink/byte/dynlink.ml - Reword configure message about ZSTD support Co-authored-by: Damien Doligez <damien.doligez@gmail.com>

xavierleroy · 2023-11-20T18:36:55Z

CI is green (here and on Jenkins). Merging in 5.1 branch. Will port to trunk and improve soon.

hhugo · 2023-11-27T14:09:44Z

runtime/intern.c

+                                       const unsigned char *,
+                                       uintnat) = NULL;
+
+CAMLprim value caml_compression_available(value vunit)


This is not used, is it intended ?

Yes, indeed. To avoid an unnecessary bootstrap, the primitive will be removed before at release.

hhugo · 2023-11-27T14:11:10Z

runtime/zstd.c

+
+CAMLprim value caml_zstd_initialize(value vunit)
+{
+  return Val_unit;


return Val_false;

hhugo · 2023-11-27T14:11:23Z

runtime/zstd.c

+{
+  caml_extern_compress_output = caml_zstd_compress;
+  caml_intern_decompress_input = caml_zstd_decompress;
+  return Val_unit;


return Val_true;

Fixed in 8abed5e .

xavierleroy and others added 6 commits November 4, 2023 19:37

Avoid dependency from Cmxs_format to Misc

5e2d331

Already done in trunk.

Correct libs used in output-complete-obj test

df411ff

Manual cherry-pick from 7ed7268 . Co-authored-by: David Allsopp <david.allsopp@metastack.com>

Move the functions that use ZSTD compression to a separate zstd.c file

aae3ad5

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`.

Remove Marshal.Compression and Marshal.compression_supported

5068559

- Removes the Marshal.Compression constructor, in preparation for explicit API in 5.2 - Add just to_channel (for now) to ocamlcommon - caml_zstd_initialize called from Compression for now.

Internal API change: Compression.to_channel -> `Compression.output_…

02b0d07

…value` The "flags" argument was always [], so remove it. This avoids an Obj.magic in compression.ml.

xavierleroy force-pushed the no-marshal-compression-flag-1 branch from d0c4a14 to 63f2cfa Compare November 12, 2023 17:30

xavierleroy added 3 commits November 12, 2023 18:36

Internal API change: introduce and use Compression.input_value

e5c9d45

Under the hood, it's just `Stdlib.input_value`. However, it documents the places in the compiler where we expect to read compressed marshaled data.

ocamlopt: do not link -lzstd nor zstd.n.o by default, only for compil…

9398511

…erlibs We attach -lzstd to ocamlcommon.cmxa and insert zstd.n.o in ocamlcommon.a. (The latter is a hack, to be replaced by something better eventually.)

Bootstrap after removal of Marshal.Compression flag

768beb3

xavierleroy force-pushed the no-marshal-compression-flag-1 branch from 63f2cfa to 768beb3 Compare November 12, 2023 17:37

xavierleroy commented Nov 12, 2023

View reviewed changes

kit-ty-kate approved these changes Nov 13, 2023

View reviewed changes

anmonteiro mentioned this pull request Nov 13, 2023

Support a broader range of versions of the underlying OCaml compiler melange-re/melange#886

Closed

xavierleroy requested a review from damiendoligez November 17, 2023 10:27

xavierleroy added this to the 5.1.1 milestone Nov 18, 2023

damiendoligez approved these changes Nov 20, 2023

View reviewed changes

configure.ac Outdated Show resolved Hide resolved

otherlibs/dynlink/byte/dynlink.ml Outdated Show resolved Hide resolved

xavierleroy force-pushed the no-marshal-compression-flag-1 branch from 3814387 to d3f94e9 Compare November 20, 2023 15:04

Address review comments on ocaml#12734

6db7ccb

- Add type constraint in dynlink/byte/dynlink.ml - Reword configure message about ZSTD support Co-authored-by: Damien Doligez <damien.doligez@gmail.com>

xavierleroy force-pushed the no-marshal-compression-flag-1 branch from d3f94e9 to 0713d6a Compare November 20, 2023 15:12

Changes for 12734

49b6988

xavierleroy force-pushed the no-marshal-compression-flag-1 branch from 0713d6a to 49b6988 Compare November 20, 2023 15:23

xavierleroy merged commit 9f9c1f7 into ocaml:5.1 Nov 20, 2023

xavierleroy mentioned this pull request Nov 21, 2023

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

Merged

xavierleroy changed the title ~~Move compressed marshaling away from the stdlib and into the compilers~~ Move compressed marshaling away from the stdlib and into the compilers (5.1 version) Nov 21, 2023

xavierleroy mentioned this pull request Nov 24, 2023

[5.1] Use the PIC object file zstd.npic.o in ocamlcommon.a #12783

Merged

kit-ty-kate mentioned this pull request Nov 26, 2023

OCaml 5.1.1 support ocaml/merlin#1713

Closed

hhugo reviewed Nov 27, 2023

View reviewed changes

runtime/zstd.c

CAMLprim value caml_zstd_initialize(value vunit)

{

return Val_unit;

Copy link
Copy Markdown

Contributor

hhugo Nov 27, 2023

Choose a reason for hiding this comment

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

return Val_false;

hhugo reviewed Nov 27, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move compressed marshaling away from the stdlib and into the compilers (5.1 version)#12734

Move compressed marshaling away from the stdlib and into the compilers (5.1 version)#12734
xavierleroy merged 11 commits intoocaml:5.1from
xavierleroy:no-marshal-compression-flag-1

xavierleroy commented Nov 12, 2023

Uh oh!

xavierleroy Nov 12, 2023 •

edited

Loading

Uh oh!

kit-ty-kate left a comment

Uh oh!

xavierleroy commented Nov 17, 2023

Uh oh!

damiendoligez left a comment

Uh oh!

Uh oh!

Uh oh!

damiendoligez commented Nov 20, 2023

Uh oh!

xavierleroy commented Nov 20, 2023

Uh oh!

hhugo Nov 27, 2023

Uh oh!

Octachron Nov 27, 2023

Uh oh!

hhugo Nov 27, 2023

Uh oh!

hhugo Nov 27, 2023 •

edited

Loading

Uh oh!

Octachron Nov 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

xavierleroy commented Nov 12, 2023

Uh oh!

xavierleroy Nov 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kit-ty-kate left a comment

Choose a reason for hiding this comment

Uh oh!

xavierleroy commented Nov 17, 2023

Uh oh!

damiendoligez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

damiendoligez commented Nov 20, 2023

Uh oh!

xavierleroy commented Nov 20, 2023

Uh oh!

hhugo Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

Octachron Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

hhugo Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

hhugo Nov 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Octachron Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

xavierleroy Nov 12, 2023 •

edited

Loading

hhugo Nov 27, 2023 •

edited

Loading