Fix #12948 (slow domain join) #13026

damiendoligez · 2024-03-13T14:30:14Z

I think this is what @gadmm had in mind in #12399 (comment).

fixes #12948
Also fixes the first item of #12399

gadmm · 2024-03-13T15:07:40Z

runtime/platform.c

+  int res;
+  caml_plat_assert_locked(cond->mutex);
+  res = pthread_cond_timedwait(&cond->cond, cond->mutex, deadline);
+  if (res != ETIMEDOUT) check_err("timedwait", res);


One documentation for pthread_cond_timedwait mentions spurious wakeups. In these cases one should probably restart pthread_cond_timedwait ~~rather than raise an exception~~. (edit: callers should take spurious wakeups into account)

gadmm

Reading more closely this time around, I changed where I think one should deal with spurious wakeups. I also have added a comment about the choice of clock.

gadmm · 2024-04-08T11:54:45Z

runtime/platform.c

+  caml_plat_assert_locked(cond->mutex);
+  res = pthread_cond_timedwait(&cond->cond, cond->mutex, deadline);
+  if (res != ETIMEDOUT) check_err("timedwait", res);
+  return res;


Suggested change

return res;

return res != ETIMEDOUT;

At this point, res is either 0 (signal or spurious wake-up) or ETIMEDOUT (deadline reached), so this is just adding a negation. I would rather keep this function as close as possible to pthread_cond_timedwait and leave the return value as-is.

gadmm · 2024-04-08T11:55:08Z

runtime/caml/platform.h

@@ -114,8 +114,7 @@ typedef struct { pthread_cond_t cond; caml_plat_mutex* mutex; } caml_plat_cond;
 #define CAML_PLAT_COND_INITIALIZER(m) { PTHREAD_COND_INITIALIZER, m }
 void caml_plat_cond_init(caml_plat_cond*, caml_plat_mutex*);
 void caml_plat_wait(caml_plat_cond*);
-/* like caml_plat_wait, but if nanoseconds surpasses the second parameter
-   without a signal, then this function returns 1. */
+int caml_plat_timedwait(caml_plat_cond*, const struct timespec *);


Suggested change

int caml_plat_timedwait(caml_plat_cond*, const struct timespec *);

/* return 0 on timeout */

int caml_plat_timedwait(caml_plat_cond*, const struct timespec *);

gadmm · 2024-04-08T12:06:57Z

otherlibs/systhreads/st_pthreads.h

+    }else{
+      deadline.tv_sec = curtime.tv_sec;
+    }
+    (void) caml_plat_timedwait (&Tick_thread_control.cond, &deadline);


Correcting my previous comment, I'd see spurious wakeups handled here:

Suggested change

(void) caml_plat_timedwait (&Tick_thread_control.cond, &deadline);

while(caml_plat_timedwait (&Tick_thread_control.cond, &deadline))

{

if (Tick_thread_control.state == Tick_stop) goto …;

/* In case of spurious wakeup, keep waiting */

};

gadmm · 2024-04-08T12:14:15Z

otherlibs/systhreads/st_pthreads.h

+  caml_plat_lock (&Tick_thread_control.mu);
+  while(1){
+    if (Tick_thread_control.state == Tick_stop) break;
+    gettimeofday (&curtime, NULL);


The clock for caml_plat_timedwait is set inside caml_plat_cond_init_aux in a platform-dependent manner. Please use e.g. clock_gettime to make sure the deadline is on the same clock.

Unfortunately, clock_gettime is not available on MacOS, so I had to reproduce the platform-dependent part.

gadmm · 2024-04-08T12:52:04Z

otherlibs/systhreads/st_stubs.c

+    caml_plat_unlock (&Tick_thread_control.mu);
+    caml_plat_signal (&Tick_thread_control.cond);


Suggested change

caml_plat_unlock (&Tick_thread_control.mu);

caml_plat_signal (&Tick_thread_control.cond);

caml_plat_signal (&Tick_thread_control.cond);

caml_plat_unlock (&Tick_thread_control.mu);

The DEBUG_LOCK seems to enforce that the mutex is locked when entering caml_plat_signal. But there is a PR currently under review which aims to remove the needless restriction.

However according to some sources online, schedulers are programmed to better handle the case where pthread_cond_signal is called with the mutex locked. Not an expert on these details though. Feel free to ignore this change if you have a strong opinion about it. I am curious to know the rationale for the best choice.

I have no strong opinion and I am ready to believe that schedulers are optimized that way.

damiendoligez · 2024-04-30T14:29:12Z

Many thanks for the thorough review. I changed the code to implement your suggestions.

damiendoligez · 2024-04-30T14:38:42Z

Running precheck at https://ci.inria.fr/ocaml/job/precheck/972/ .

gasche · 2024-05-15T09:36:51Z

This needs a rebase, it would be useful to get an explicit approval from @gadmm, but also the CI reports a lot of failed tests on Windows:


    tests/backtrace/backtrace_systhreads.ml
    tests/backtrace/callstack.ml
    tests/c-api/test_c_thread_has_lock_systhread.ml
    tests/lib-channels/input_all.ml
    tests/lib-systhreads/boundscheck.ml
    tests/lib-systhreads/multicore_lifecycle.ml
    tests/lib-systhreads/test_c_thread_register.ml
    tests/lib-threads/backtrace_threads.ml
    tests/lib-threads/bank.ml
    tests/lib-threads/bufchan.ml
    tests/lib-threads/close.ml
    tests/lib-threads/fileio.ml
    tests/lib-threads/mutex_errors.ml
    tests/lib-threads/pr5325.ml
    tests/lib-threads/pr7638.ml
    tests/lib-threads/pr8857.ml
    tests/lib-threads/prodcons.ml
    tests/lib-threads/prodcons2.ml
    tests/lib-threads/sieve.ml
    tests/lib-threads/swapchan.ml
    tests/lib-threads/tls.ml
    tests/lib-threads/torture.ml
    tests/lib-threads/uncaught_exception_handler.ml
    tests/lib-unix/win-socketpair/test.ml
    tests/parallel/fib_threads.ml
    tests/parallel/multicore_systhreads.ml
    tests/parallel/test_c_thread_register.ml
    tests/regression/pr12948/test.ml
    tests/statmemprof/blocking_in_callback.ml
    tests/statmemprof/moved_while_blocking.ml
    tests/statmemprof/thread_exit_in_callback.ml

This probably needs some investigation if the results are consistent after the rebase and CI rerun.

gadmm reviewed Mar 13, 2024

View reviewed changes

gadmm suggested changes Apr 8, 2024

View reviewed changes

damiendoligez added 3 commits April 30, 2024 14:20

add regression test for ocaml#12948

ca943b8

interrupt tick thread without delay on domain shutdown

292f61c

take comments into account and update Changes

e587924

damiendoligez force-pushed the fix-slow-domain-join branch from a317498 to e587924 Compare April 30, 2024 14:24

fix wrong identifier

099750b

gasche added bug multicore runtime-system labels May 15, 2024

damiendoligez self-assigned this May 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #12948 (slow domain join) #13026

Fix #12948 (slow domain join) #13026

damiendoligez commented Mar 13, 2024

gadmm Mar 13, 2024 •

edited

gadmm left a comment

gadmm Apr 8, 2024

damiendoligez Apr 30, 2024

gadmm Apr 8, 2024

gadmm Apr 8, 2024

gadmm Apr 8, 2024

damiendoligez Apr 30, 2024

gadmm Apr 8, 2024

damiendoligez Apr 30, 2024

damiendoligez commented Apr 30, 2024 •

edited

damiendoligez commented Apr 30, 2024

gasche commented May 15, 2024

	int caml_plat_timedwait(caml_plat_cond, const struct timespec );
	/* return 0 on timeout */
	int caml_plat_timedwait(caml_plat_cond, const struct timespec );

-    (void) caml_plat_timedwait (&Tick_thread_control.cond, &deadline);
+    while(caml_plat_timedwait (&Tick_thread_control.cond, &deadline))
+    {
+      if (Tick_thread_control.state == Tick_stop) goto …;
+      /* In case of spurious wakeup, keep waiting */
+    };

		caml_plat_unlock (&Tick_thread_control.mu);
		caml_plat_signal (&Tick_thread_control.cond);

Fix #12948 (slow domain join) #13026

Are you sure you want to change the base?

Fix #12948 (slow domain join) #13026

Conversation

damiendoligez commented Mar 13, 2024

gadmm Mar 13, 2024 • edited

Choose a reason for hiding this comment

gadmm left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damiendoligez commented Apr 30, 2024 • edited

damiendoligez commented Apr 30, 2024

gasche commented May 15, 2024

gadmm Mar 13, 2024 •

edited

damiendoligez commented Apr 30, 2024 •

edited