Skip to content
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.

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

[wip] Allow callbacks to be registered for GVL related events #119

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from
Prev Previous commit
Next Next commit
Make rb_gvl hook API thread safe
  • Loading branch information
byroot committed Jan 25, 2022
commit 4d48cfe3944f019c9cb4dae4de4895f798f7a4e2
61 changes: 43 additions & 18 deletions thread_pthread.c
Original file line number Diff line number Diff line change
@@ -102,48 +102,69 @@
#endif

static gvl_hook_t * rb_gvl_hooks = NULL;
static pthread_rwlock_t rb_gvl_hooks_rw_lock = PTHREAD_RWLOCK_INITIALIZER;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder whether we should use read-write locks or just plain locks. Read-write locks will probably improve performance slightly if there are GVL hooks, but it means that the hooks must be careful to ensure their code is thread-safe. Do we care so much about being as performant as possible when there are GVL hooks vs. the convenience of ensuring there can't be race conditions?

Copy link
Author

Choose a reason for hiding this comment

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

it means that the hooks must be careful to ensure their code is thread-safe

Yeah, I think that's an implied part of the contract since some of the hooks are executed outside of the GVL anyway.

I'm trying first and foremost to minimize the overhead when no hook is registered, so that people don't using it aren't impacted, hence the ATOMIC uses and the if (rb_gvl_hooks) without locking.

But I'd also like to keep the overhead reasonably low when some hooks are registered so that it would be a viable API in production (for monitoring etc). Hence the read-write locks, so that hopefully we don't create too much contention.


gvl_hook_t *
rb_gvl_event_new(void *callback, uint32_t event) {
gvl_hook_t *hook = ALLOC_N(gvl_hook_t, 1);
hook->callback = callback;
hook->event = event;

if(!rb_gvl_hooks) {
rb_gvl_hooks = hook;
} else {
hook->next = rb_gvl_hooks;
rb_gvl_hooks = hook;
if (pthread_rwlock_wrlock(&rb_gvl_hooks_rw_lock)) {
// TODO: better way to deal with error?
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the other pthread functions in Ruby hard crash with rb_bug. The documentation says that it can only error pthread_rwlock_wrlock can return is

EDEADLK The current thread already owns the read-write lock for writing or reading.

This shouldn't ever happen I think, so maybe rb_bug should be used here too.

Copy link
Author

Choose a reason for hiding this comment

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

Oh! That's great info! Thanks!

ruby_xfree(hook);
return NULL;
}

hook->next = rb_gvl_hooks;
ATOMIC_PTR_EXCHANGE(rb_gvl_hooks, hook);

pthread_rwlock_unlock(&rb_gvl_hooks_rw_lock);
return hook;
}

bool
rb_gvl_event_delete(gvl_hook_t * hook) {
if (pthread_rwlock_wrlock(&rb_gvl_hooks_rw_lock)) {
// TODO: better way to deal with error?
return FALSE;
}

bool success = FALSE;

if (rb_gvl_hooks == hook) {
rb_gvl_hooks = hook->next;
ruby_xfree(hook);
return TRUE;
ATOMIC_PTR_EXCHANGE(rb_gvl_hooks, hook->next);
success = TRUE;
} else {
gvl_hook_t *h = rb_gvl_hooks;

do {
if (h->next == hook) {
h->next = hook->next;
success = TRUE;
}
} while ((h = h->next));
}

gvl_hook_t *h = rb_gvl_hooks;

do {
if (h->next == hook) {
h->next = hook->next;
ruby_xfree(hook);
return TRUE;
}
} while ((h = h->next));
pthread_rwlock_unlock(&rb_gvl_hooks_rw_lock);

return FALSE;
if (success) {
ruby_xfree(hook);
}
return success;
}

void
rb_gvl_execute_hooks(uint32_t event) {
if (!rb_gvl_hooks) {
return;
}

if (pthread_rwlock_rdlock(&rb_gvl_hooks_rw_lock)) {
// TODO: better way to deal with error?
return;
}

gvl_hook_t *h = rb_gvl_hooks;
struct gvl_hook_event_args args = {};

@@ -152,6 +173,8 @@ rb_gvl_execute_hooks(uint32_t event) {
(*h->callback)(event, args);
}
} while((h = h->next));

pthread_rwlock_unlock(&rb_gvl_hooks_rw_lock);
}

enum rtimer_state {
@@ -697,6 +720,8 @@ static void native_thread_init(rb_thread_t *th);
void
Init_native_thread(rb_thread_t *th)
{
pthread_rwlock_init(&rb_gvl_hooks_rw_lock, NULL);

#if defined(HAVE_PTHREAD_CONDATTR_SETCLOCK)
if (condattr_monotonic) {
int r = pthread_condattr_init(condattr_monotonic);