Skip to content

Simplify wait in slices logic. NFC. #15739

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Jan 20, 2022
Merged

Simplify wait in slices logic. NFC. #15739

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
59aac0a
Simplify/unify wait in slices logic. NFC.
kleisauke Nov 23, 2021
c0b7882
Revert unnecessary changes
kleisauke Dec 10, 2021
21c0823
Revert hoisting of `self` changes
kleisauke Dec 10, 2021
e9692af
Revert small doc changes
kleisauke Dec 10, 2021
2a52598
Drop const for changed lines
kleisauke Dec 10, 2021
4b8a024
Prefer snake_case for changed lines
kleisauke Dec 10, 2021
961e320
Revert `emscripten_wait_for_call_v` change
kleisauke Dec 10, 2021
4784dfa
Consolidate vars
kleisauke Dec 10, 2021
02ba5d6
s/max_ms_to_sleep/max_ms_slice_to_sleep/g
kleisauke Jan 13, 2022
c8d47d0
Avoid call to `emscripten_futex_wait` when `msecsToSleep <= 0`
kleisauke Jan 13, 2022
40064d6
Resolve test failures
kleisauke Jan 15, 2022
70bfc3b
Split ternary operation over 2 lines
kleisauke Jan 17, 2022
ecbd5fa
Revert non-NFC change
kleisauke Jan 20, 2022
7d74c53
Incorporate review comment
kleisauke Jan 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions system/lib/libc/musl/src/thread/__timedwait.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
#include <math.h>
#include <emscripten/threading.h>
#include <emscripten/emscripten.h>
#include "pthread_impl.h"
#else
#include "futex.h"
#endif
Expand Down Expand Up @@ -61,6 +60,10 @@ int __timedwait_cp(volatile int *addr, int val,
#ifdef __EMSCRIPTEN__
double msecsToSleep = top ? (top->tv_sec * 1000 + top->tv_nsec / 1000000.0) : INFINITY;
int is_runtime_thread = emscripten_is_main_runtime_thread();

// Main runtime thread may need to run proxied calls, so sleep in very small slices to be responsive.
double max_ms_slice_to_sleep = is_runtime_thread ? 1 : 100;

// cp suffix in the function name means "cancellation point", so this wait can be cancelled
// by the users unless current threads cancelability is set to PTHREAD_CANCEL_DISABLE
// which may be either done by the user of __timedwait() function.
Expand All @@ -77,16 +80,17 @@ int __timedwait_cp(volatile int *addr, int val,
}
// Assist other threads by executing proxied operations that are effectively singlethreaded.
if (is_runtime_thread) emscripten_main_thread_process_queued_calls();
// Must wait in slices in case this thread is cancelled in between.
double waitMsecs = sleepUntilTime - emscripten_get_now();
if (waitMsecs <= 0) {

msecsToSleep = sleepUntilTime - emscripten_get_now();
if (msecsToSleep <= 0) {
r = ETIMEDOUT;
break;
}
if (waitMsecs > 100) waitMsecs = 100; // non-main threads can sleep in longer slices.
if (is_runtime_thread && waitMsecs > 1) waitMsecs = 1; // the runtime thread may need to run proxied calls, so sleep in very small slices to be responsive.
r = -emscripten_futex_wait((void*)addr, val, waitMsecs);
} while(r == ETIMEDOUT);
// Must wait in slices in case this thread is cancelled in between.
if (msecsToSleep > max_ms_slice_to_sleep)
msecsToSleep = max_ms_slice_to_sleep;
r = -emscripten_futex_wait((void*)addr, val, msecsToSleep);
} while (r == ETIMEDOUT);
} else {
// Can wait in one go.
r = -emscripten_futex_wait((void*)addr, val, msecsToSleep);
Expand Down
12 changes: 7 additions & 5 deletions system/lib/libc/musl/src/thread/__wait.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,12 @@ void __wait(volatile int *addr, volatile int *waiters, int val, int priv)
if (waiters) a_inc(waiters);
#ifdef __EMSCRIPTEN__
int is_runtime_thread = emscripten_is_main_runtime_thread();

// Main runtime thread may need to run proxied calls, so sleep in very small slices to be responsive.
double max_ms_slice_to_sleep = is_runtime_thread ? 1 : 100;

while (*addr==val) {
if (is_runtime_thread || pthread_self()->cancelasync == PTHREAD_CANCEL_ASYNCHRONOUS) {
// Must wait in slices in case this thread is cancelled in between.
int e;
do {
if (pthread_self()->cancel) {
Expand All @@ -27,10 +30,9 @@ void __wait(volatile int *addr, volatile int *waiters, int val, int priv)
}
// Assist other threads by executing proxied operations that are effectively singlethreaded.
if (is_runtime_thread) emscripten_main_thread_process_queued_calls();
// Main thread waits in _very_ small slices so that it stays responsive to assist proxied
// pthread calls.
e = emscripten_futex_wait((void*)addr, val, is_runtime_thread ? 1 : 100);
} while(e == -ETIMEDOUT);
// Must wait in slices in case this thread is cancelled in between.
e = emscripten_futex_wait((void*)addr, val, max_ms_slice_to_sleep);
} while (e == -ETIMEDOUT);
} else {
// Can wait in one go.
emscripten_futex_wait((void*)addr, val, INFINITY);
Expand Down
5 changes: 2 additions & 3 deletions system/lib/libc/musl/src/thread/pthread_barrier_wait.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,12 +93,11 @@ int pthread_barrier_wait(pthread_barrier_t *b)
if (is_runtime_thread) {
int e;
do {
// Main thread waits in _very_ small slices so that it stays responsive to assist proxied
// pthread calls.
// Main runtime thread may need to run proxied calls, so sleep in very small slices to be responsive.
e = emscripten_futex_wait(&inst->finished, 1, 1);
// Assist other threads by executing proxied operations that are effectively singlethreaded.
emscripten_main_thread_process_queued_calls();
} while(e == -ETIMEDOUT);
} while (e == -ETIMEDOUT);
} else {
// Can wait in one go.
emscripten_futex_wait(&inst->finished, 1, INFINITY);
Expand Down
27 changes: 10 additions & 17 deletions system/lib/pthread/library_pthread.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,12 +84,7 @@ int pthread_mutexattr_setprioceiling(pthread_mutexattr_t *attr, int prioceiling)
static uint32_t dummyZeroAddress = 0;

void emscripten_thread_sleep(double msecs) {
double now = emscripten_get_now();
double target = now + msecs;

__pthread_testcancel(); // pthreads spec: sleep is a cancellation point, so must test if this
// thread is cancelled during the sleep.
emscripten_current_thread_process_queued_calls();
double target = emscripten_get_now() + msecs;

// If we have less than this many msecs left to wait, busy spin that instead.
double min_ms_slice_to_sleep = 0.1;
Expand All @@ -99,23 +94,21 @@ void emscripten_thread_sleep(double msecs) {

emscripten_conditional_set_current_thread_status(
EM_THREAD_STATUS_RUNNING, EM_THREAD_STATUS_SLEEPING);
now = emscripten_get_now();
while (now < target) {

double ms_to_sleep;
do {
// Keep processing the main loop of the calling thread.
__pthread_testcancel(); // pthreads spec: sleep is a cancellation point, so must test if this
// thread is cancelled during the sleep.
emscripten_current_thread_process_queued_calls();

now = emscripten_get_now();
double ms_to_sleep = target - now;
if (ms_to_sleep > max_ms_slice_to_sleep) {
ms_to_sleep = target - emscripten_get_now();
if (ms_to_sleep < min_ms_slice_to_sleep)
continue;
if (ms_to_sleep > max_ms_slice_to_sleep)
ms_to_sleep = max_ms_slice_to_sleep;
}
if (ms_to_sleep >= min_ms_slice_to_sleep) {
emscripten_futex_wait(&dummyZeroAddress, 0, ms_to_sleep);
}
now = emscripten_get_now();
};
emscripten_futex_wait(&dummyZeroAddress, 0, ms_to_sleep);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The loop condition here seems a little odd to me. It doesn't seem to take into account that time has elapsed during emscripten_futex_wait. Perhaps adding ms_to_sleep = target - now; as the very last thing line of the loop would fix it? But then we are kind of re-using the single ms_to_sleep to mean two different things: How long to sleep for this loop cylcle, and the total remain time to sleep.. Maybe we should keep those things logically separate for readability?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit 7d74c53 takes the elapsed time during emscripten_futex_wait into account.

} while (ms_to_sleep > 0);

emscripten_conditional_set_current_thread_status(
EM_THREAD_STATUS_SLEEPING, EM_THREAD_STATUS_RUNNING);
Expand Down