Should both dls_state and dls_root be renamed to the same thing, similar to other fields? One option is tls_state or tls_root.

All uses of dls should be renamed to tls?

We do not know much about the future usages of Domain.at_exit so we don't know whether the mutex is expensive. Do note, though, that it is fairly straightforward to avoid holding the mutex in most cases (more details on request).

Minor detail, this change fixes a thread-unsafety of Domain.at_exit of current trunk.

Thanks. Does the thread-unsafety of Domain.at_exit have an issue on ocaml/ocaml?

Do note, though, that it is fairly straightforward to avoid holding the mutex in most cases (more details on request).

If it is very straightforward, I am happy to hear. With that said, do you think we can have it as a separate PR? We've spent quite a bit of time on this PR and #12719 already. I'm keen to get this done and dusted as a first cut.

Thanks. Does the thread-unsafety of Domain.at_exit have an issue on ocaml/ocaml?

Not that I know of, I just noticed it while reviewing.

If it is very straightforward, I am happy to hear. With that said, do you think we can have it as a separate PR? We've spent quite a bit of time on this PR and #12719 already. I'm keen to get this done and dusted as a first cut.

Sure, at the same time we can check if we need a "yet to run" flag as for Stdlib.at_exit. In this case I think this commit is good to go.

we can check if we need a "yet to run" flag as for Stdlib.at_exit. In this case I think this commit is good to go.

sounds good.

My intuition is that this example is now broken, it should use a Thread.at_exit function that does not appear to currently exist? (And so it should rather be documented as a use-case for Thread.at_exit than Domain.at_exit.)

The example isn't broken (well it is, since there is a typo). A temporary file descriptor will now be created per thread, which will be closed when the domain that the thread is running on exits. It is not what you would want ideally, but the file does get closed.

It may be useful to have a Thread.at_exit, but that's a conversation that I'd like to not have in this PR.

(I think the typo is that new_key should now be make.)

When I wrote the comment I thought that the domain would only close one file descriptor on exit (which is broken), but in fact the cumulative behavior of Domain.at_exit (adding a new callback instead of replacing the callback) means, as you point out, that all descriptors will be closed on domain termination.

I still consider that this example is not right, however. Consider in particular the case of a single domain that regularly spawns short-lived threads (to execute a blocking call, for example). What the user needs is to be able to keep a number of file descriptors alive that corresponds to the number of threads that are alive. Instead the number of file descriptors open will grow in an unbounded way until program termination. This is not just "no ideal", in practice it is not acceptable for this workload.

Remark: putting a finalizer on the value produced by the initialisation function would work better¹ than using Domain.at_exit. If we don't want to have Thread.at_exit, maybe we should rewrite the example to use a finalizer?

¹: I assume that thread termination clears the TLS array of the thread, but I am not sure that this is the case with the current PR.

I think that it would also be nice to provide a code example of accessing Domain.id to determine whether we are spawning a new domain, or a new thread on the same domain.

This implementation is still incorrect due to re-entrancy issues: if init () in turn calls maybe_grow, st could be wrong at this point. I think that the following would work better:

let v' = Obj.repr (init ()) in
(* the tls array may have been resized by [init ()],
    call [maybe_grow] again to get the up-to-date array. *)
let st = maybe_grow idx in
st.(idx) <- Sys.opaque_identity v';

This and other similar issues you pointed out are not due to this PR's change -- making DLS thread-local. These issues are present on trunk. I would prefer if these unrelated issues do not delay this PR getting in. There are already many related issues that need sorting out.

We should move this and others to a separate issue. What do you think?

Fixed this one as suggested.

Note that I am happy to defer those three reentrancy issues to a later PR as you suggested. They make the code more complex and would probably deserve an independent review.

This one is straightforward. So picked this change here. Other ones are more complicated, and I'd like to defer those to a separate issue.

Nitpick: this implementation is incorrect in presence of asynchronous callbacks -- code executed on allocation and poll points. The call to Array.make may call asynchronous code that would maybe_grow and initialize and define new keys that are missing from st.

This is an obscure scenario and we may want to leave things as-is if there is no easy fix. But I think that there would be an easy fix, due to the fact that the tls array only grows and never shrinks:

    let new_sz = compute_new_size (max 8 sz) in
    let new_st = Array.make new_sz unique_value in
    (* the [new_st] allocation may result in an asynchronous resize of the tls_state array:
       continue from the current array. *)
    let cur_st = get_tls_state () in
    let cur_sz = Array.length cur_st in
    if idx < cur_st then cur_st
    else begin
      assert (cur_sz <= new_sz);
      Array.blit cur_st 0 new_st 0 cur_sz;
      set_tls_state new_st;
    end

(Note: I am bad at reasoning about poll points, so I am not sure that there are no poll points in the new version between the binding of cur_st and the set_tls_state call. If there are poll points in there, possibly after Array.blit for example, the new version is still incorrect for asynchronous callbacks.)

Same as #12724 (comment) (?).

There is a reentrancy issue here if the calls to get k change the value of parent_keys by registering new keys that use inheritance.

I wrote the following not-too-horrible version that adds new keys in a loop.

let get_initial_keys () : (int * Obj.t) list =
  (* compute the key-value pair for the child from an initializer *)
  let kv_of_ki (KI ((idx, _) as k, split)) =
    (idx, Obj.repr (split (get k)))
  in
  (* We compute key-value pairs for all 'parent keys' in a loop,
     because the list of parent keys may grow during the
     computation. *)
  let rec loop acc last_parent_keys =
    let new_parent_keys = Atomic.get parent_keys in
    if new_parent_keys == last_parent_keys then acc
    else
      (* process all key initializers that are in [new_parent_keys]
         but not in [last_parent_keys], then loop again. *)
      let rec add_new_keys acc keys =
        if keys == last_parent_keys
        then loop acc new_parent_keys
        else match keys with
          | [] -> loop acc new_parent_keys
          | ki :: rest ->
            add_new_keys (kv_of_ki ki :: acc) rest
      in
      add_new_keys acc new_parent_keys
  in
  loop [] []

Same as #12724 (comment) (?).

I find this implementation surprisingly complex and I wonder if this is the right approach.

1. Maybe we actually want to have Thread.at_exit, which would be naturally easier to implement on top of Thread_local_storage? (Note: even if we did, we probably want to keep a Domain.at_exit around with the current semantics, but that would mostly be for advanced out-of-tree experiments.)

2. We discussed a way to implement DLS on top of TLS, isn't this function a good opportunity to try this? For example, it would seem fairly natural to me to implement this as a (unit -> unit) ref Thread_local_storage.t that is shared across all threads of a given domain. Am I missing something? (If we try, and it appears that DLS-on-top-of-TLS is worse than we thought, maybe that is a reason to reconsider.)

I'd like to understand what is complex with this implementation. It is a single piece of shared state at_exit_callbacks protected by a mutex.

If we want to retain Domain.at_exit, I'd rather keep the current implementation, and think about Thread.at_exit in a separate PR.

Part of the argument we used to move DLS to TLS is that it "should be easy" to implement DLS on top of TLS.

But then, on the one time where we do need an implementation of DLS inside the compiler distribution, we punt and implement a globally-locked map of domain identifiers. Is this our recommendation for users as well, that they implement a globally-locked of domain identifiers? If not, why don't we follow our own recommendations?

I think that we should do our homework and implement Domain.at_exit as DLS on top of TLS. I see two possible outcomes:

1. If it turns out to be simple and pleasant, this would be better I think than your current implementation. Plus we have an example of DLS-over-TLS implementation we can point people at. Win!
2. If it turns out to be harder than we thought, or have performance downsides that we find problematic, then it suggests that our initial intuition was wrong and maybe we should reconsider or at least think harder. I would rather figure this out now than after a release.

Part of the argument we used to move DLS to TLS is that it "should be easy" to implement DLS on top of TLS.

It was not my understanding that this was easy. But I sympathise with the push for having a DLS for Domain.at_exit.

The lazy-based implementation (#12719 (comment)) has a concurrency issue in that multiple threads on a domain may concurrently perform DLS.get, which will concurrently force the lazy value, which is not concurrency safe.

I haven't investigated the concurrent hash map + caching solution. This requires a concurrent hash map in the standard library @gadmm? or am I misunderstanding the solution?

If we are providing a DLS, I would prefer to implement it directly as a field in domain state, just as TLS is. We could potentially share the implementation between the two and focus on making them both concurrency-safe and reentrant.

I think that since Caml_state->tls_state is registered as a generational global root, this should use caml_modify_generational_global_root.

Question: shouldn't each th->tls_state field also be registered as a root?

Answer: there is no need to register each th->tls_state as a root, because it is scanned during caml_thread_scan_roots. This also answers my question related to finalizers: it should be the case that the tls_state of a terminated thread is collected by the GC, as it will not be kept alive by caml_thread_scan_roots after thread termination.

Since I think we are free to change the API compared to the existing DLS one, a couple of suggestions:

1. I think this would make more sense as two functions:

val make : (unit -> 'a) -> 'a t
val make_with_split : 'a -> split:('a -> 'a) -> 'a t

2. I think it would be good to tell split whether we are splitting a new thread or splitting a new domain. Something like:

val make_with_split : 'a -> split:(same_domain:bool -> 'a -> 'a) -> 'a t

@lpw25 I'm not sure what you mean with the 'a parameter of make_with_split. Maybe your intention is that this is the initial value on the main domain, that new domains will split from. But note that at the point where we call make_with_split, there may be several domains running already, what is the behavior you have in mind in this case?

With the current API, any domain that was created before the key will call the (unit -> 'a) initializer to get its value, lazily, on the first key access. (Note that this (unit -> 'a) thunk could include some subtle logic to derive those values from a common one. The only thing it cannot reconstruct is the parent-child relationship between existing domains.)

Some possible approaches that one may have in mind:

• All existing domains get assigned the same 'a value. I don't think this works, especially in the case where 'a has mutable state, as the result could be very racy.
• The initial domain has the 'a value. Other pre-existing domains initialize their value by splitting the initial-domain value on the first access. This is probably the best we can do with the API you propose, but note that for non-pre-existing domains the split functions is called on the parent, not on the child, and here we would call it on the child. I don't think this really works well. (In particular, the split function may mutate the parent's state, this is what the PRNG split does, and calling it from the child with the parent's value is very racy again.)

Note: I am reopening the discussion of DLS API in #13356 and would be happy to incorporate @lpw25's suggestion, but I am currently blocked by the interrogations above.

I wrote that a while ago, but I think I was suggesting that if you use a split that it should run eagerly for existing domains starting from the initial domain based on the tree structure of their creation. This gives you a consistent semantics regardless of when you happen to call make relative to the creation of other domains. Perhaps the cost of running the split functions eagerly overrides the benefit, but I would have thought that the cases that wanted split were more likely to be run at top-level before the creation of any additional domains.

Separately from that part of the suggestion, I really think you should separate make_with_split from make without. Providing the split parameter fundamentally changes the behaviour of the function, which is not a good use of optional arguments. Another way to put it is that there is no value for split_from_parent that reproduces the behaviour of providing no split_from_parent and I think that is an API smell.

I think I was suggesting that if you use a split that it should run eagerly for existing domains starting from the initial domain based on the tree structure of their creation.

This would be tricky to implement as those existing domains are already running concurrently.

I guess that we could interrupt domains in turn following the spawning tree, so that they call the split function asynchronously at the next poll point. First we interrupt domain 0, it splits a value for each domain that it spawned, and then in turn it interrupts those domains if they themselves have children, etc. We can do this asynchronously, only DLS.get operations need to block until the value for the current child domain has been computed.

I'm not sure what to do if a domain in the spawn tree has already terminated. (I don't think we currently impose that child domains terminate before their parents, only that all domains are eventually joined.) We could reasonably decide that its own parent (the grand-parent of the domain we are spawning) is then in charge of computing the split value. (I think the behavior is less well-behaved if the child itself does it.)

Sorry, I meant that the call to make would immediately run split to get the values for all the existing domains, and then set them. This ensures that the splitting is all going to be thread safe. However, it does mean that split will not always be run on the parent thread, so if you use Thread_local_state.get from within a split function you won't always receive the values for the parent thread. Perhaps that is enough of a downside not to do it this way?

I find it a little difficult to judge the split API without a more precise understanding of the nature of its use cases. Are there use cases for it beyond the splittable PRNG? If not, should we tailor it more specifically to that case?