[wip] Allow callbacks to be registered for GVL related events #119
Conversation
casperisfine
force-pushed
the
mvh-jb-gvl-callbacks
branch
from
b01459b to
e0c541b
Compare
|
@eightbitraptor FYI, I added a quick API to remove hooks (seems like it segfaults..., not too sure why) |
casperisfine
force-pushed
the
mvh-jb-gvl-callbacks
branch
from
1d72486 to
e5ef64f
Compare
casperisfine
force-pushed
the
mvh-jb-gvl-callbacks
branch
3 times, most recently
from
d1ede76 to
60cf306
Compare
casperisfine
force-pushed
the
mvh-jb-gvl-callbacks
branch
from
60cf306 to
cd2222b
Compare
casperisfine
requested review from
tenderlove,
XrXr,
peterzhu2118 and
eightbitraptor
|
👋 This is still mostly a prototype (e.g. no windows implementation) but I'd love to get some early feedback on it. e.g. Is there some big no-nos, etc. And wether the general approach is sound. |
thread_pthread.c
Outdated
| hook->event = event; | ||
|
|
||
| if (pthread_rwlock_wrlock(&rb_gvl_hooks_rw_lock)) { | ||
| // TODO: better way to deal with error? |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh! That's great info! Thanks!
| #define RUBY_INTERNAL_EVENT_SWITCH 0x00040000 /**< Thread switched. */ | ||
| #define RUBY_EVENT_SWITCH 0x00040000 /**< @old{RUBY_INTERNAL_EVENT_SWITCH} */ | ||
| /*0x00080000 */ | ||
| #define RUBY_INTERNAL_EVENT_NEWOBJ 0x00100000 /**< Object allocated. */ | ||
| #define RUBY_INTERNAL_EVENT_FREEOBJ 0x00200000 /**< Object swept. */ | ||
| #define RUBY_INTERNAL_EVENT_GC_START 0x00400000 /**< GC started. */ | ||
| #define RUBY_INTERNAL_EVENT_GC_END_MARK 0x00800000 /**< GC ended mark phase. */ | ||
| #define RUBY_INTERNAL_EVENT_GC_END_SWEEP 0x01000000 /**< GC ended sweep phase. */ | ||
| #define RUBY_INTERNAL_EVENT_GC_ENTER 0x02000000 /**< `gc_enter()` is called. */ | ||
| #define RUBY_INTERNAL_EVENT_GC_EXIT 0x04000000 /**< `gc_exit()` is called. */ | ||
| #define RUBY_INTERNAL_EVENT_OBJSPACE_MASK 0x07f00000 /**< Bitmask of GC events. */ | ||
| #define RUBY_INTERNAL_EVENT_GVL_ACQUIRE_ENTER 0x10000000 /** `gvl_acquire() is called */ | ||
| #define RUBY_INTERNAL_EVENT_GVL_ACQUIRE_EXIT 0x20000000 /** `gvl_acquire() is exiting successfully */ | ||
| #define RUBY_INTERNAL_EVENT_GVL_RELEASE 0x40000000 /** `gvl_release() is called */ | ||
| #define RUBY_INTERNAL_EVENT_GVL_MASK 0x70000000 /**< Bitmask of GVL events. */ | ||
| #define RUBY_INTERNAL_EVENT_MASK 0xffff0000 /**< Bitmask of internal events. */ |
There was a problem hiding this comment.
Maybe it's just me, but I find using hex for bit flags and bit masks is much harder to read than using left shifts for bit flags and &ing bit flags for bit masks. 😅
There was a problem hiding this comment.
I don't think it's just you :) I just figured I'd match the existing pattern.
| @@ -101,6 +101,78 @@ | |||
| # endif | |||
| #endif | |||
|
|
|||
| static gvl_hook_t * rb_gvl_hooks = NULL; | |||
| static pthread_rwlock_t rb_gvl_hooks_rw_lock = PTHREAD_RWLOCK_INITIALIZER; | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
I don't have bandwidth to give a thorough review, so I'm going to recuse myself from the reviewers' list. |
casperisfine
force-pushed
the
mvh-jb-gvl-callbacks
branch
from
c96f517 to
4346b16
Compare
casperisfine
force-pushed
the
mvh-jb-gvl-callbacks
branch
from
4346b16 to
d27dafe
Compare
casperisfine
force-pushed
the
mvh-jb-gvl-callbacks
branch
from
d27dafe to
fed36e0
Compare
Quick & dirty proof of concept for the GVL instrumentation API https://bugs.ruby-lang.org/issues/18339
To be continued later this week, it's full of shortcuts for now.