Multicore OCaml #10831

kayceesrk · 2021-12-21T15:13:53Z

This PR adds support for shared-memory parallelism through domains and direct-style concurrency through effect handlers (without syntactic support). It intends to have backwards compatibility in terms of language features, C API, and also the performance of single-threaded code.

For users

If you want to learn more about Multicore OCaml, please have a look at the multicore wiki for papers, talks, tutorials and blog posts.

If you are interested in using Multicore OCaml, we suggest having a look at the following work-in-progress libraries:

domainslib -- a library for nested task parallelism
eio -- asynchronous io in direct-style

Here is a snapshot of the multicore scalability results on parallel benchmarks from sandmark on a 2 processor AMD EPYC 7551 server with 64 cores in total:

The number in the parenthesis next to the benchmark name is the time (in seconds) taken by the sequential baseline of the corresponding benchmark.

Review process

This PR comes at the back of asynchronous code reviews by the OCaml core developers followed by a week-long synchronous code review session. The summary of the discussions is available in the November multicore monthly.

As mentioned in the November multicore monthly, this PR constitutes the minimum viable product (MVP) for Multicore OCaml. The "pre-MVP" tasks have been completed, and the "post-MVP for 5.00" tasks are yet to be done. The aim is that these tasks, and the ones that follow, can be completed on ocaml/ocaml Github repo, through the usual OCaml PR review process rather than on the Multicore OCaml repo.

Given that the PR is quite large, if you spot major breakages and functionality gaps, we suggest that you make separate issues on ocaml/ocaml Github repo. This will help keep the discussion threads readable. Feel free to comment on minor issues (typos, formatting edits, etc) directly in this PR, and we shall be happy to fix those.

What's in the box

The only supported backend is amd64 on Linux and macOS; arm64 is in the works. The following features are not implemented:

statmemprof
compilation with frame pointers
ocamldebug
AFL support
flambda

Acknowledgements

Multicore OCaml has been a long running project. We would like to thank all those who have helped find issues, debugged issues, reviewed code and contributed code along the way.

-- The Multicore OCaml team

…o/skip-unsupported-tests Skip unsupported and incompatible tests

…threads_wg3_comments Systhreads WG3 comments

…ackup_thread

…ards

…ment_on_bt_install Comment on caml_domain_spawn also calling in install_backup_thread

…nitize-exports Rename / hide some global variables

…emoved

…ert_testsuite_summarize

…gnals_coalesce Signals changes from sync review and WG3

xavierleroy · 2022-01-10T16:13:05Z

just pushed a multicore branch to shindere/ocaml. At the head of this branch is a commit that removes the mention of naked
pointers from Inria's CI script. Could this be cherry-picked to this PR,please?

Done (just after the merge).

gadmm

Some of these comments point out bugs.
There is a question about a discrepancy between the implementation of non-atomic writes according to the PLDI paper and the actual implementation of the write barrier.
Many comments are about improvements that were lost at some point during rebase/merges regarding the polling behaviour. It is easier to get multicore up to speed with the recent changes separately (and help you with it), but I have to use this PR to point out the issues.

gadmm · 2022-01-10T16:16:53Z

runtime/array.c

-  // Give the GC a chance to run, and run memprof callbacks
-  return caml_process_pending_actions_with_root (result);
+  /* Give the GC a chance to run */
+  return caml_check_urgent_gc (result);


(For context caml_check_urgent_gc is nowadays supposed to not to call any asynchronous callback to avoid raising asynchronous exceptions, which will be the topic of another review comment.)

I do not like losing memprof-specific code and comments which were the work of several error-prone PRs (or do you have a strategy for both authors and reviewers to make sure that they will be reintroduced when the time comes?). I find it better keep the memprof code and comments until it is upgraded to multicore, at the cost of commenting-out some of it (not needed here).

Idem further below in the file.

gadmm · 2022-01-10T16:16:53Z

runtime/array.c

@@ -445,7 +480,7 @@ static value caml_array_gather(intnat num_arrays,
    /* Many caml_initialize in a row can create a lot of old-to-young
       refs.  Give the minor GC a chance to run if it needs to.
       Run memprof callbacks for the major allocation. */
-    res = caml_process_pending_actions_with_root (res);
+    res = caml_check_urgent_gc(res);


Thanks for keeping the memprof comment here; please also keep the polling behaviour for consistency of the comment with the trunk semantics of process_pending vs. check_urgent.

gadmm · 2022-01-10T16:16:53Z

runtime/backtrace.c

+  /* Beware: the allocations below may cause finalizers to be run, and another
+     backtrace---possibly of a different length---to be stashed (for example
+     if the finalizer raises then catches an exception).  We choose to ignore
+     any such finalizer backtraces and return the original one. */


By current semantics of caml_alloc, this should not happen. Instead, caml_alloc should be fixed to delay the call to finalisers like in trunk (this looks simple to do). Then one can simplify the code below it as was done in trunk some time ago.

gadmm · 2022-01-10T16:16:53Z

runtime/caml/alloc.h

@@ -27,23 +27,35 @@
 extern "C" {
 #endif

-/* It is guaranteed that these allocation functions will not trigger
-   any OCaml callback such as finalizers or signal handlers. */


As I mentioned above, this recent guarantee should be kept. It must have been accidentally lost during the rebase on top of 4.12, since I do not see any difficulty in keeping it in multicore.

gadmm · 2022-01-10T16:16:53Z

runtime/caml/domain_state.tbl

+DOMAIN_STATE(volatile uintnat, young_limit)
+/* Minor heap limit. Typically young_limit == young_start, but this field is set
+ * by other domains to signal this domain by causing a spurious allocation
+ * failure. */


Shouldn't this be atomic instead of volatile?

This is now fixed.

gadmm · 2022-01-10T16:16:53Z

runtime/signals.c

-     needed. Therefore, we set [caml_something_to_do] again in order
-     to force reexamination of callbacks. */
-  caml_set_action_pending();
-  return exn;


This function is necessary if you want to correctly implement caml_process_pending_actions*

gadmm · 2022-01-10T16:16:53Z

runtime/platform.c

+  if (!mem)
+    printf("Trimming failed\n");
+  else if (mem != (void*)aligned_start)
+    printf("Hang on a sec - it's allocated a different block?!\n");


Taking inspiration from https://github.com/golang/go/blob/c27a3592aec5f46ae18f7fd3d9ba18e69e2f1dcb/src/runtime/malloc.go#L813-L828 this seems to be on the right track, but it seems that you need to do it in a loop to take care of the possibility of a race.
I'd avoid printf to report a runtime error, and the string could be a bit more meaningful. In any case make sure that in case of error it returns 0 (you can also call caml_fatal_error for situations you know cannot happen according to specification).
Idem about printf in two more occurrences below in the same file.

(cc @dra27)

See #10908 (which, for what it's worth, would have appeared 5 days ago with an issue 😉)

gadmm · 2022-01-10T16:16:53Z

runtime/memory.c

+   Using release stores for all writes also ensures publication safety
+   for newly-allocated objects, and isn't necessary for initialising
+   writes. The cost is free on x86, but requires a fence in
+   caml_modify on weakly-ordered architectures (ARM, Power).


There is a difference between this description and what is described in the PLDI 2018 paper, isn't it? I.e. no release store for writes (i.e. in armv8 str rather than stlr, no full dmb barrier).

gadmm · 2022-01-10T16:16:53Z

runtime/memory.c

+  /* See Note [MM] above */
+  atomic_thread_fence(memory_order_acquire);
+  atomic_store_explicit(&Op_atomic_val((value)fp)[0], val,
+                        memory_order_release);


Following my comment above based on the PLDI paper here I expect memory_order_relaxed instead of memory_order_release

gadmm · 2022-01-10T16:16:53Z

runtime/interp.c

      }
      /* Fall through CHECK_SIGNALS */

 /* Signal handling */

    Instruct(CHECK_SIGNALS):    /* accu not preserved */
-      if (caml_something_to_do) goto process_actions;
+      if (Caml_check_gc_interrupt(domain_state) ||
+          caml_check_for_pending_signals())


In link with the polling behaviour changes, this can be made more efficient again. (Idem for POPTRAP.)

silene · 2022-01-11T13:36:40Z

Function caml_process_pending_actions_exn seems to have disappeared. Is it on purpose?

gadmm · 2022-01-11T13:44:29Z

@silene This is ocaml-multicore/ocaml-multicore#791. A lot of things making caml_process_pending_actions_exn possible are currently missing, my review above tracks what is missing.

sadiqj · 2022-01-11T14:22:32Z

A lot of things making caml_process_pending_actions_exn possible are currently missing, my review above tracks what is missing.

I think it's worth us pulling this in to a separate issue on ocaml/ocaml where we can work out exactly what needs to happen. There are some parts (like delaying finalisers and returning exceptions rather than raising) which actually require a fair bit of work to make happen.

gadmm · 2022-01-11T14:24:53Z

I agree, just rebasing correctly alone would have been very hard for someone not familiar with the code, and working on this part in trunk is the best way to be able to help you.

gadmm · 2022-01-11T17:49:45Z

runtime/memory.c

+  } else {
+    /* See Note [MM] above */
+    atomic_thread_fence(memory_order_acquire);
+    ret = atomic_exchange(Op_atomic_val(ref), v);


Here's a question about the implementation of the memory model again. For context, currently, Atomic.store and Atomic.exchange result directly in a call to caml_atomic_exchange.
Here if I follow naively the paper I expect instead something like:

ret = atomic_exchange(Op_atomic_val(ref), v); atomic_thread_fence(memory_order_release);

On armv8 (clang) the original code results in:
dmb (ish)ld; L: ldaxr w2, [x0]; stlxr w3, w1, [x0]; cbnz w3, L (notice the missing dmb st)
whereas the above one results in:
L: ldaxr w2, [x0]; stlxr w3, w1, [x0]; cbnz w3, .L17; dmb (ish)
which is closer to the paper (it does not seem to be possible to emit the dmb st barrier with standard C atomics; the closest is to use a full barrier).

Of course at the moment armv8 is not supported (but I assume that the armv8 prototype is currently implemented with these functions). However this runs deeper:

The implementation currently seems correct on x86 when these functions are called from OCaml. However one could imagine that when they are called from C code, a compiler might try to inline them and be too smart. How do you ensure that these functions implement the OCaml memory model on x86 when called from C? (Edit: the answer is that currently caml_atomic_exchange is not (yet) accessible from C code; only caml_modify, which seems to implement a stronger operation than the OCaml memory model. Maybe this question will be relevant later.)

Maybe the memory model has evolved between the first prototype and the paper and the documentation at the top of the file was not updated. In that case should comment it be updated (more that I initially thought)?

Naively I would think that for the OCaml-C FFI you are eventually going to implement a conservative approximation of the OCaml memory model in C, whereas for native OCaml code you will eventually emit the exact assembly code you need (plus a C call to the write barrier). Sorry if there is a simple answer that I am missing!

Edit: the questions above will be relevant eventually, but for now (x86-only) it looks correct, even if misleading.

… rebase Mentioned in ocaml#10831 review comment on runtime/io.c:552.

…se (#10975) Mentioned in #10831 review comment on runtime/io.c:552. Co-authored-by: sabine <sabine@tarides.com>

ocaml-multicore/multicore was already merge in ocaml/ocaml (ocaml/ocaml#10831) and avalaible with 5.00.0+trunk. Now it makes sense to remove ocaml-multicore repo and use directly the version 5.00 of OCaml.

Engil and others added 30 commits December 9, 2021 10:46

Merge pull request ocaml-multicore/ocaml-multicore#774 from moyodiall…

be01621

…o/skip-unsupported-tests Skip unsupported and incompatible tests

Merge pull request ocaml-multicore/ocaml-multicore#745 from Engil/sys…

0b4eb1b

…threads_wg3_comments Systhreads WG3 comments

runtime: add a comment on caml_domain_spawn also calling in install_b…

50a0ba7

…ackup_thread

block signals before spawning a domain/thread and restore them afterw…

a532dde

…ards

remove total_signals_pending

e2b4bd5

coalesce signals by CASing to 0

f815019

fix suspicious indentation

093fd80

remove POSIX_SIGNALS ifdefs

b2edb14

also block signals for caml_thread_new

05747a9

Merge pull request ocaml-multicore/ocaml-multicore#778 from Engil/com…

9b6415a

…ment_on_bt_install Comment on caml_domain_spawn also calling in install_backup_thread

Merge upstream changes

1543fd1

Add missing copyright header to runtime/caml/shared_heap.h

c05e33e

Rename exported global variable: global ↝ caml_global_heap_state

6cc610e

Make the global variable 'pool_freelist' static.

71b889f

Make the global variable 'atoms' static.

e7ae89c

Merge pull request ocaml-multicore/ocaml-multicore#779 from yallop/sa…

4d686dd

…nitize-exports Rename / hide some global variables

Retain const*, remove duplicate free

6efd08b

Merge branch '5.00' into intern_again

a7ac4f8

Use caml_global_heap_state instead of global

07e1304

Revert testsuite/summarize.awk now that testsuite/disabled has been r…

b8433d7

…emoved

Merge pull request ocaml-multicore/ocaml-multicore#784 from ctk21/rev…

3c8e7b5

…ert_testsuite_summarize

no need to add, store is sufficient

e174eab

Merge pull request ocaml-multicore/ocaml-multicore#766 from sadiqj/si…

c34f90a

…gnals_coalesce Signals changes from sync review and WG3

Make signal_install_mutex static

c2b8baf

Make marshal_flags static

3ce4b9f

Make ephe_sweep static

279474c

Make ephe_mark static

7acbd8b

Make ephe_cycle_info static

95dc395

Make verify_push static

f48d866

Make reset_minor_tables static

a49fb1e

nojb mentioned this pull request Jan 10, 2022

Get rid of <caml/compatibility.h> #10863

Merged

gadmm reviewed Jan 10, 2022

View changes

jonahbeckford mentioned this pull request Jan 10, 2022

PIC runtime for 32-bit ARM with __PIC__ #10860

Open

Engil mentioned this pull request Jan 11, 2022

caml_thread_setname compilation and adjustments #10870

Closed

ctk21 mentioned this pull request Jan 11, 2022

Fixup Filename.check_suffix; remove duplicate ',' fix for OCAMLRUNPARAM #10873

Merged

gadmm reviewed Jan 11, 2022

View changes

dra27 mentioned this pull request Jan 12, 2022

Fix closure marshalling from Dynlink-loaded code on Windows #10884

Closed

gadmm mentioned this pull request Jan 12, 2022

Use a bit-vector to record pending signals #10880

Merged

OlivierNicole mentioned this pull request Jan 12, 2022

Wrong memory order constraint in caml_modify #10886

Closed

dra27 mentioned this pull request Jan 15, 2022

Fix possible race in caml_mem_map on Windows #10908

Merged

gadmm mentioned this pull request Jan 18, 2022

Meta-issue on asynchronous action handling in multicore #10915

Open

20 tasks

dra27 mentioned this pull request Jan 28, 2022

Ignore compatible --disable options in configure #10962

Merged

sabine pushed a commit to sabine/ocaml that referenced this pull request Feb 1, 2022

add missing changes from ocaml#10136 that may have been lost during a…

64e4aed

… rebase Mentioned in ocaml#10831 review comment on runtime/io.c:552.

sabine mentioned this pull request Feb 1, 2022

add missing changes from #10136 that may have been lost during a rebase #10975

Merged

gadmm mentioned this pull request Feb 1, 2022

Arm64 multicore support #10972

Merged

xavierleroy pushed a commit that referenced this pull request Feb 1, 2022

add missing changes from #10136 that may have been lost during a reba…

9a7b6ac

…se (#10975) Mentioned in #10831 review comment on runtime/io.c:552. Co-authored-by: sabine <sabine@tarides.com>

stedolan mentioned this pull request Feb 6, 2022

Explain mapping between OCaml memory model and C #10995

Open

gadmm mentioned this pull request Feb 20, 2022

Atomic: update documentation and add institution #11038

Merged

xavierleroy mentioned this pull request Feb 24, 2022

Minor improvements to "named values" handling #11056

Merged

gadmm mentioned this pull request Feb 24, 2022

Implement quality treatment for asynchronous actions in multicore #11057

Open

moyodiallo mentioned this pull request Mar 1, 2022

Upgrade multicore-ci on OCaml-5.00 ocurrent/ocaml-multicore-ci#16

Merged

dra27 mentioned this pull request Oct 4, 2022

Turn tests/c-api/alloc_async.ml back on #11596

Open

dra27 mentioned this pull request Oct 13, 2022

Remove unused #define Context_needs_padding from stack.h #11621

Merged

Engil mentioned this pull request Dec 5, 2022

Use MAP_POPULATE when commiting the minor heap segment to improve error reporting on overcommit #11790

Open

Multicore OCaml #10831

Multicore OCaml #10831

kayceesrk commented Dec 21, 2021 •

edited

xavierleroy commented Jan 10, 2022

gadmm left a comment

gadmm Jan 10, 2022 •

edited

gadmm Jan 10, 2022

gadmm Jan 10, 2022 •

edited

gadmm Jan 10, 2022

gadmm Jan 10, 2022

gadmm Jan 27, 2022

gadmm Jan 10, 2022

gadmm Jan 10, 2022

gadmm Jan 15, 2022

dra27 Jan 15, 2022

gadmm Jan 10, 2022 •

edited

gadmm Jan 10, 2022

gadmm Jan 10, 2022

silene commented Jan 11, 2022

gadmm commented Jan 11, 2022

sadiqj commented Jan 11, 2022

gadmm commented Jan 11, 2022

gadmm Jan 11, 2022 •

edited

Multicore OCaml #10831

Multicore OCaml #10831

Conversation

kayceesrk commented Dec 21, 2021 • edited

For users

Review process

What's in the box

Acknowledgements

xavierleroy commented Jan 10, 2022

gadmm left a comment

Choose a reason for hiding this comment

gadmm Jan 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gadmm Jan 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gadmm Jan 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silene commented Jan 11, 2022

gadmm commented Jan 11, 2022

sadiqj commented Jan 11, 2022

gadmm commented Jan 11, 2022

gadmm Jan 11, 2022 • edited

Choose a reason for hiding this comment

kayceesrk commented Dec 21, 2021 •

edited

gadmm Jan 10, 2022 •

edited

gadmm Jan 10, 2022 •

edited

gadmm Jan 10, 2022 •

edited

gadmm Jan 11, 2022 •

edited