search

ocaml / ocaml

The core OCaml system: compilers, runtime system, base libraries
https://ocaml.org
Other
5.5k stars 1.11k forks source link

Unify separate atomic counter functions in runtime #13593

Closed NickBarnes closed 2 weeks ago

NickBarnes commented 2 weeks ago

This small PR unifies two separate implementations of atomic counters in the runtime. There are functions for atomic_uintnat counters in atomic_refcount.h, used in bigarray.c, and we use similar counters in major_gc.c, with one inline function atomic_fetch_add_verify_ge0 in platform.h which is only used for decrement but has an assertion to guard against underflow. I unify the two into inline functions in camlatomic.h (which seems like the natural home for them). Incidentally, the increment/decrement functions now return the value after the update—obviously more intuitive than the previous atomic_refcount semantics—which means a 1 in bigarray.c has become a 0. There's also a tiny tweak in misc.h to avoid the circular header dependency which otherwise arises.

gasche commented 2 weeks ago

(I had some questions/discussions in my comment and my expectation would be that maybe @NickBarnes has an opinion on them, which I would be curious to hear -- maybe he decides to update the PR as a result -- before we decide to merge.)

NickBarnes commented 2 weeks ago

There are some calls to caml_atomic_store_release(&num_domain_to_* that could be turned into caml_atomic_refcount_init calls

Updating these.

and it may also make sense to define a helper function to encapsulate the caml_atomic_load_acquire(&foo) == 0 logic, which is frequent in major_gc.c, and where the acquire ordering might be important for performance.

As I understand it, atomic_load(p) is the same as atomic_load_explicit(p, memory_order_seq_cst) which is the same as atomic_load_explicit(p, memory_order_acquire) which is the same as atomic_load_acquire(p). I assume the reason we have atomic_load_acquire() at all is to explicitly document the memory order for the programmer. I'm replacing all accesses to these atomic counters with calls to the appropriate inline caml_atomic_counter_ functions.

My impression is that this PR leaves the code slightly inconsistent in that some of the num_domains_to_* variables use some counter operations and also some plain loads and stores.

Fixing these up for consistency. Thank you.

NickBarnes commented 2 weeks ago

Updated to reflect review comments: all access to atomic counters is now through caml_atomic_counter_... inline functions. As an experiment, I also made this branch which uses the singleton-struct hack to force all users to use this camlatomic.h API. That found an instance in otherlibs/unix/mmap_ba.c which was just using --, which I have here replaced with the appropriate function call. Atomic types in C are defined such that -- and ++ decrement and increment operators on them do the right thing; this whole PR is based on the assumption that we avoid using those to emphasize the atomic nature - and cost - of the operations to the programmer.