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.

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

[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
Draft

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

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c3fa0fd
[wip] Allow callbacks to be registered for GVL related events
eightbitraptor Jan 24, 2022
e0c541b
Basic unregister GVL callbacks API (thread unsafe)
byroot Jan 25, 2022
4d48cfe
Make rb_gvl hook API thread safe
byroot Jan 25, 2022
316c44d
Call the gvl hooks
byroot Jan 25, 2022
cd2222b
Move GVL instrumentation tests in their own files
byroot Jan 26, 2022
720262b
Make gvl->waiting atomic
byroot Jan 26, 2022
b867e19
Hard crash on pthread_rwlock errors
byroot Jan 27, 2022
1f599a6
Stub rb_gvl_event_* on win32
byroot Jan 27, 2022
ed89a1b
More pthread_rwlock error handling
byroot Jan 27, 2022
5c9e48c
Fix depend
byroot Jan 27, 2022
fed36e0
Fix callback type
byroot Jan 27, 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
Prev Previous commit
Fix callback type
  • Loading branch information
@byroot
byroot committed Jan 27, 2022
commit fed36e094a5cb42496e3ec1209737300dbfda7b6
2 changes: 1 addition & 1 deletion include/ruby/thread_native.h
View file
Original file line number Diff line number Diff line change
@@ -214,7 +214,7 @@ typedef struct gvl_hook {
struct gvl_hook *next;
} gvl_hook_t;

gvl_hook_t * rb_gvl_event_new(void *callback, rb_event_flag_t event);
gvl_hook_t * rb_gvl_event_new(rb_gvl_callback callback, rb_event_flag_t event);
bool rb_gvl_event_delete(gvl_hook_t * hook);
RBIMPL_SYMBOL_EXPORT_END()
#endif
2 changes: 1 addition & 1 deletion thread_pthread.c
View file
Original file line number Diff line number Diff line change
@@ -105,7 +105,7 @@ 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, rb_event_flag_t event) {
rb_gvl_event_new(rb_gvl_callback callback, rb_event_flag_t event) {
gvl_hook_t *hook = ALLOC_N(gvl_hook_t, 1);
hook->callback = callback;
hook->event = event;
2 changes: 1 addition & 1 deletion thread_win32.c
View file
Original file line number Diff line number Diff line change
@@ -36,7 +36,7 @@ static volatile DWORD ruby_native_thread_key = TLS_OUT_OF_INDEXES;
static int w32_wait_events(HANDLE *events, int count, DWORD timeout, rb_thread_t *th);

gvl_hook_t *
rb_gvl_event_new(void *callback, rb_event_flag_t event) {
rb_gvl_event_new(rb_gvl_callback callback, rb_event_flag_t event) {
// not implemented yet
}