Allow maximum number of domains to be specified as a OCAMLRUNPARAM parameter

[kayceesrk]

This PR introduces a new OCAMLRUNPARAM parameter d that permits the user to set the maximum number of domains. This idea was suggested by @shindere at #13256 (comment).

Fixes #13256.

[ghost]

Shouldn't the user-configurable value be bounds checked to be within reasonable limits (i.e. 1 <= user value < no more than a few k)?

[kayceesrk]

Shouldn't the user-configurable value be bounds checked to be within reasonable limits (i.e. 1 <= user value < no more than a few k)?

That sounds reasonable. Would 4096 be a good upper limit for a start?

[gasche]

I believe that this is a step in the right direction -- hardcoding a fixed maximum of domains is bound to create issues sooner or later.

I tried to use github search to find code out there that relies on the current limit to the number of domains. It's hard because there is a lot of noise from copies of the compiler codebase. I found the following:

• boxroot, as found in a vendored copy at https://github.com/lord/ocaml-swift-example/blob/1fd0033c24d54ff4ae962c59b59a10594c2268c6/boxroot/platform.c#L11-L13
• multicoretests in https://github.com/shym/multicoretests/blob/22e3440e3158efeb52d5418b242207e100c3b6a2/src/domain/domain_spawntree.ml#L3 ; this seems slightly harder to fix as it runs as a separate process, and does not have access to the OCAMLRUNPARAM value

Boxroot is easily fixed by reading the new global instead, I believe. (This code runs inside the OCaml process, so it sees the same OCAMLRUNPARAM value). For multicoretests this is less obvious because it runs as a separate process, so it cannot access the "real" value, but in fact this is just fine as long as the test runner controls the OCaml process and can fix the max_domains value there.

For runtime_events consumers we should be in the clear, as the runtime_events codebase had the excellent insight of passing the max_domains value in the metadata header from the start: it is already possible to write consumers that are robust to different values of this.

To summarize: I think that we are making this change early enough that there is not a lot of sensitivity on this value in the ecosystem, and we should be fine compatibility-wise.

[ghost]

That sounds reasonable. Would 4096 be a good upper limit for a start?

I think so.

Or maybe 2 x sysconf(_SC_NPROCESSORS_CONF) on Unix-like platforms. But that's probably too much bikeshed already.

[shindere]

KC Sivaramakrishnan (2024/07/01 02:54 -0700):

> Shouldn't the user-configurable value be bounds checked to be within reasonable limits (i.e. 1 <= user value < no more than a few k)? That sounds reasonable. Would 4096 be a good upper limit for a start?

I have no idea / experience but are there values that would just make no sense in terms of cache sizes or so?

[gasche]

Or maybe 2 x sysconf(_SC_NPROCESSORS_CONF) on Unix-like platforms. But that's probably too much bikeshed already.

One issue with this is that it would break on the current default for most machines, so for example if I run the multicoretest runner with OCAMLRUNPARAM="domains=128" I get a hard failure.

@kayceesrk why are we using one-letter name for OCAMLRUNPARAM variables, is this enforced by the specification or implementation? Otherwise I wouldn't mind domains or max_domains instead of d.

[shindere]

Gabriel Scherer (2024/07/01 02:59 -0700):

> Or maybe `2 x sysconf(_SC_NPROCESSORS_CONF)` on Unix-like platforms. But that's probably too much bikeshed already. One issue with this is that it would break on the current default for most machines, so for example if I run the multicoretest runner with `OCAMLRUNPARAM="domains=128"` I get a hard failure. @kayceesrk why are we using one-letter name for OCAMLRUNPARAM variables, is this enforced by the specification or implementation? Otherwise I wouldn't mind `domains` or `max_domains` instead of `d`.

@damiendoligez may have something to say on that one?

[kayceesrk]

For runtime_events consumers we should be in the clear, as the runtime_events codebase had the excellent insight of passing the max_domains value in the metadata header from the start: it is already possible to write consumers that are robust to different values of this.

Thanks for writing this out. I did want to mention this in the PR message. There was nothing to be done here since @sadiqj has already done the necessary work. Thanks Sadiq!

[kayceesrk]

To summarize: I think that we are making this change early enough that there is not a lot of sensitivity on this value in the ecosystem, and we should be fine compatibility-wise.

Thanks for the summary.

To fix the failing windows builds, @dra27 mentioned that this may need a bootstrap because of flexdll bootstrap. I'll do that next.

[gasche]

Nit: I would rather keep the [foo, bar) intervals on a single line each for readability, which requires doing some manual line-breaking here.

[kayceesrk]

Done in 263be36.

[gasche]

I think this call should probably move to caml_init_gc, as it initializes a runtime service that is independent from the domains machinery. (I have no idea what caml_init_signal_handling is doing at the end of the current function, and the same comment would apply -- or maybe even move it to caml_start_* -- but oh well.)

[kayceesrk]

Done in 263be36.

[kayceesrk]

Still seeing Windows CI failures after the bootstrap. @dra27 do you see other possibilities for failures that we see?

[gadmm]

As an alternative, perhaps simpler solution, these two variables implementing some domain-local state could be moved into dedicated Caml_state fields. It should not be a problem if people not using systhreads allocate two fields but do not use them.

[kayceesrk]

It seemed appropriate to move it to the thread_table in st_stubs.c. Done that in 263be36.

[gadmm]

ok

[gasche]

I have mixed feelings about this suggestion (for and against). This does not matter as I think you came to a satisfying resolution already, but let me elaborate in case it can help for future discussions.

1. I think that systhreads are there to stay, and that we should not hesitate to integrate them more in the runtime to make their implementation better, simpler, more correct.
2. On the other hand, I like that we still have code lying around in the runtime that behaves sort of like a third-party consumer of the runtime interfaces, instead of being hardcoded in every place. The ability for third-party libraries to have domain-local state as an array is useful, and keeping this aspect of the systhreads implementation is useful as a canary to check that we preserve this use-case. (@gadmm points out that the present change incurs a performance regression, which may matter for our current boxroot prototype; I don't know if it does matter for systhreads).

To conclude: if there are strong benefits to moving things into Caml_state, let's do it. But if we can do without, it gives us useful information about what third-party libraries can do, and there is value in that as well.

[gasche]

(This reasoning somehow applies to gc_stats as well.)

[gadmm]

That's a roundabout way of saying that the runtime could implement a proper DLS for itself because it would also benefit libraries.

[gasche]

Yep, "roundabout" is my middle name.

[gadmm]

Out of curiosity, is there a reason for not having this as a field in the domain state variable?

[kayceesrk]

There are a few more failure cases to handle if sampled_gc_stats were moved to Caml_state. In particular, Caml_state is created on demand when domains are created. If gc_stats* were a field in Caml_state, even with the guarantee that stats are only sampled in stop-the-world sections, we need to check for whether gc_stats is not NULL (since newly created domains may not participate in stw sections).

I tried to make this change, but the changes for handling failure cases got messy. Perhaps one solution is to have sampled_gc_stats in dom_internal struct. But that struct is internal to domain.c.

Something to come back to later.

[gadmm]

ok, sounds like a good enough reason

[gadmm]

Boxroot is easily fixed by reading the new global instead, I believe. (This code runs inside the OCaml process, so it sees the same OCAMLRUNPARAM value).

It's not that easy, and the change is not ideal for us. Max_domain being known at compilation-time means that the array is allocated statically. This is used to implement an efficient domain-local state, which is relied-on for the fast path of boxroot. This usage of a fixed Max_domain has previously been discussed as we introduced the timing hook caml_domain_terminated_hook for this purpose (#11209).

As previously discussed with @gasche, simply removing the limitation of Max_domains would remove an application without providing an alternative solution. Yet alternative implementations of DLS for C code would require (simple, but) more intrusive modifications to the runtime, and therefore need coordination with upstream prior to removal.

This PR presents an intermediate solution that does not remove Max_domains entirely, but makes it chosen at runtime. This changes the implementation of DLS as follows:

• One additional test for null when loading the DLS field.
• One additional indirection with a data depency on the previous test.
• This becomes a memory-inefficient way to implement a DLS when Max_domains is large (same issue for the present PR actually).

So this is not ideal for us, as the fast path of boxroot was carefully optimised (and we really gave advance notice about this usage of Max_domains).

Two possible solutions are not too complex to implement:

• More efficient DLS could be offered natively. For instance, another domain state variable that works like Caml_state could be introduced, with slots allocated dynamically like an implicit TLS). This also ought to be useful for the runtime, e.g. the two uses of DLS inside systhreads (except if you accept my alternative suggestion to reserve a Caml_state slot for them).
• A quick solution could be to please add one field for us (boxroot) inside the domain state. (A quick solution indeed although I don't like the idea of boxroot being priviledged compared to other libraries.)

[gadmm]

Thinking further, there might be a third solution to our problem of an efficient DLS, via caching inside an (efficient) implicit TLS variable. In this case, it might not matter very much if DLS is made a bit less efficient. I will think about it further and let you know if there are any issue in the end with the current PR.

[gadmm]

This looks good to me.

[gadmm]

Regarding the usage of Max_domains to implement a DLS, I am now pretty confident that we can get better performance by caching a pointer to DLS using implicit TLS, than any other possible implementation of DLS. Indeed, implicit TLS is backed by linker tricks that are otherwise inaccessible. (This makes the assumption that a system thread cannot change domains, but this assumption is probably deeply entrenched everywhere in the runtime implementation anyway.)

It is still useful to have a limit number of domains to implement the backing DLS, though. So it is a different story than if users started taking very large values for max_domains (currently limited to 4096) or if max_domains was removed entirely.

[kayceesrk]

(This makes the assumption that a system thread cannot change domains, but this assumption is probably deeply entrenched everywhere in the runtime implementation anyway.)

Indeed. We have also documented this fact here: https://ocaml.org/manual/5.2/parallelism.html#s:par_systhread_interaction.

So it is a different story than if users started taking very large values for max_domains (currently limited to 4096)

We will have plenty of reasons to celebrate if someone usefully takes advantage of 4096 CPU cores in a single process. :-)

Thanks for the approval. I'm debugging the Windows failures now.

[gasche]

@kayceesrk would it also be possible to have a Sandmark run for this PR? I don't expect any performance difference because I don't think we rely intensively on domain-indexed arrays in the runtime, but @gadmm comments made me curious about potential performance impact.

[gadmm]

I don't think it is necessary to benchmark the change. We're talking about microarchitectural effects here, and the code changed by this PR is not optimised that much.

[ghost]

At first I was concerned that the previous failure point in this function uses caml_failwith, but it can only occur in a specific cinematic where it makes sense to raise an exception, while these added allocation failures should be considered fatal (hence caml_fatal_error).

Eventually (in a later PR) this routine should be allowed to return a caml_result, but that's more work (and orthogonal to the problem being solved here).

[gadmm]

Proper error handling in this function is actually part of #12410.

(Incidentally I noticed that this PR interferes with many others: #12410, #12964, #13026.)

[gadmm]

Actually, I think these fatal errors should be Out_of_memory exceptions instead, since the caml_failwith above shows that it is admissible to fail with uninitialized thread machinery.

Now this raises the question of whether the caml_failwith above is sound. It is sound if we are sure that an uncaught exception during module initialization stops the program (otherwise we need hardening by checking the variable threads_initialized at the entry points of the Thread module). Unfortunately I don't know enough about module initialization to answer this question.

[kayceesrk]

Is it a good idea to continue program execution when caml_stat_calloc_noexc fails to allocate memory for thread_table? Given that the underlying calloc has failed, the program is very likely doomed to fail soon in a non-recoverable way. Better to hard fail (fatal error) as soon as we anticipate this.

Like @gadmm, I also have the question of whether caml_failwith is sound. In particular, the exception that may be raised by this function is not handled at the call site,

ocaml/otherlibs/systhreads/thread.ml

Lines 76 to 79 in 6ad156a

	let () =
	thread_initialize ();
	(* Called back in [caml_shutdown], when the last domain exits. *)
	Callback.register "Thread.at_shutdown" thread_cleanup

and will lead to module initialisation failing with an uncaught exception?

[gadmm]

My reasoning here is that the Out_of_memory will halt the program quickly anyway. Indeed, otherwise there is also an issue right above with caml_failwith. This can provide better debugging. Since this is a large allocation (e.g. can be above glibc MMAP_THRESHOLD), this does not mean that the remaining mallocs are going to fail.

But I agree that the main concern is the Failure above, which happens if another module spawns domains during initialization. Do we know if, when module initialization fails:

• no functions from this module can be called?
• the program stops?

[gasche]

The semantics I expect if the evaluation of a module raises an exception is that the whole program fails with that exception, without executing the code of later modules in link order. (The mental model of ocamlc a.ml b.ml c.ml is that the produced program performs the sequential evaluation of modules A, B, C.)

However, taking a step back: I think this part of the runtime is an unfinished design, not something set in stone that we should think deeply about. We should admit that the runtime model exposed to our users is a small number of domains with arbitrarily many non-parallel threads per domain, and move whatever parts of the thread machinery into the runtime/ logic that let us do this in a simpler, more robust way. It's "just" that no one around is actively working on this.

[kayceesrk]

Since this is a large allocation (e.g. can be above glibc MMAP_THRESHOLD), this does not mean that the remaining mallocs are going to fail.

Good point.

We should admit that the runtime model exposed to our users is a small number of domains with arbitrarily many non-parallel threads per domain, and move whatever parts of the thread machinery into the runtime/ logic that let us do this in a simpler, more robust way.

Agree with this sentiment. It may help us move towards a cleaner runtime, avoiding the various hooks.

I suspect that there will be a bit of design work that will be needed as the introduction of systhreads may add overheads in programs (due to the hooks). An easy first step may be to run Sandmark benchmarks with and without linking systhreads to see whether the overheads are significant.

[kayceesrk]

There is one last failure on MSVC 64-bits: https://github.com/ocaml/ocaml/actions/runs/9774768473/job/26994732983#step:13:1862. I'm struggling to set up a development environment to debug this.

[kayceesrk]

CI runs are successful now. It looks like not zero initialising what used to be statically allocated arrays was causing failures.

[kayceesrk]

CI is green now. We've had approvals from @gadmm and @dustanddreams. @gasche are we ok to merge?

[gasche]

@gadmm makes the point that this introduces conflicts with other in-flight PRs ( #12410, #12964, #13026 ). I still believe that "merge whichever is ready first" is a good principle, so I'm happy to merge this one, but I think we should also take this remark as a warning sign that we have a backlog of runtime PRs to handle, not to let fester for too long. I haven't been available to do much OCaml hacking in the last few weeks (and this will remain the case for another couple weeks). Clearly for #12410 the ball is in my camp and I will try to have a look.

[damiendoligez]

@gasche

why are we using one-letter name for OCAMLRUNPARAM variables, is this enforced by the specification or implementation? Otherwise I wouldn't mind domains or max_domains instead of d.

The parsing is done in C by a simple loop. We'd need a careful redesign (and more code) to switch to arbitrary identifiers.