From 78c0c33229ac807bd12926aaeffb7ea8d7d4d7b1 Mon Sep 17 00:00:00 2001 From: Todd White Date: Wed, 12 Aug 2026 23:44:24 -0400 Subject: [PATCH] Let selector table readers share the lock The selector table is read far more often than it is written: every sel_getName, and every sel_registerName on a name that is already known, reads it and changes nothing. Both took the same exclusive lock as registration, so readers serialised against each other. The lock becomes a read-write lock. On Windows that is a slim reader/writer lock in place of the critical section, whose exclusive case is a mutex; elsewhere it is a pthreads read-write lock. Slim locks need Windows Vista. objc_register_selector_copy read the table through sel_getName while holding the lock, which worked only because the lock was recursive. It reads through sel_getNameNonUnique instead, which takes no lock and is what register_selector_locked already uses for the same purpose. Both answer the name held by the type list, so the value is unchanged. Test/TypedSelectorRegistration.m registers a typed selector whose untyped form is already known, which is the path that reads the table under the registration lock. It times out against the recursive call and passes without it. Windows, 24 threads, ns per call: sel_registerName on a known name 23871 to 2986, sel_getName 17273 to 2173. Eight threads gain between 1.5 and 3.3 times, and one thread is unchanged. The critical section is what collapses; acquiring and releasing one 24 threads at once costs 17093 ns against 2184 for the shared case of a slim lock. On glibc it is close to a wash: sel_registerName at 24 threads goes from 2654 to 2073 ns, because the lock is held there across a hash lookup long enough for readers to overlap, while sel_getName holds it across a single vector index and does not improve. A pthreads read acquire costs what the mutex cost, both being one atomic read-modify-write on one shared line. --- Test/CMakeLists.txt | 1 + Test/TypedSelectorRegistration.m | 26 ++++++++++++++ lock.h | 61 ++++++++++++++++++++++++++++++++ selector_table.cc | 31 ++++++++++------ 4 files changed, 109 insertions(+), 10 deletions(-) create mode 100644 Test/TypedSelectorRegistration.m diff --git a/Test/CMakeLists.txt b/Test/CMakeLists.txt index b88dda39..2f070b88 100644 --- a/Test/CMakeLists.txt +++ b/Test/CMakeLists.txt @@ -52,6 +52,7 @@ set(TESTS hash_table_delete.c hash_test.c setSuperclass.m + TypedSelectorRegistration.m UnexpectedException.m ) diff --git a/Test/TypedSelectorRegistration.m b/Test/TypedSelectorRegistration.m new file mode 100644 index 00000000..e0423ecd --- /dev/null +++ b/Test/TypedSelectorRegistration.m @@ -0,0 +1,26 @@ +#include "Test.h" +#include + +// Registering a typed selector whose untyped form is already known reads the +// selector table while the registration holds the table's lock. Both +// registrations answer the same name, and the typed one keeps its types. +int main(void) +{ + SEL untyped = sel_registerName("probeMethodWithValue:"); + assert(strcmp(sel_getName(untyped), "probeMethodWithValue:") == 0); + assert(sel_getType_np(untyped) == NULL); + + SEL typed = sel_registerTypedName_np("probeMethodWithValue:", "v@:i"); + assert(strcmp(sel_getName(typed), "probeMethodWithValue:") == 0); + assert(strcmp(sel_getType_np(typed), "v@:i") == 0); + // One copy of the name is kept, shared with the untyped selector. + assert(sel_getName(typed) == sel_getName(untyped)); + + // The same pair in the other order. + SEL typedFirst = sel_registerTypedName_np("otherProbeMethod:", "v@:d"); + SEL untypedSecond = sel_registerName("otherProbeMethod:"); + assert(strcmp(sel_getName(untypedSecond), "otherProbeMethod:") == 0); + assert(strcmp(sel_getType_np(typedFirst), "v@:d") == 0); + + return 0; +} diff --git a/lock.h b/lock.h index 071f2dce..d76ee2b2 100644 --- a/lock.h +++ b/lock.h @@ -13,6 +13,15 @@ typedef CRITICAL_SECTION mutex_t; # define LOCK(x) EnterCriticalSection(x) # define UNLOCK(x) LeaveCriticalSection(x) # define DESTROY_LOCK(x) DeleteCriticalSection(x) +// A slim reader/writer lock needs Windows Vista or later. Its exclusive mode +// is a mutex, so a writer sees the same behaviour as a critical section. +typedef SRWLOCK rwlock_t; +# define INIT_RWLOCK(x) InitializeSRWLock(&(x)) +# define RDLOCK(x) AcquireSRWLockShared(x) +# define RDUNLOCK(x) ReleaseSRWLockShared(x) +# define WRLOCK(x) AcquireSRWLockExclusive(x) +# define WRUNLOCK(x) ReleaseSRWLockExclusive(x) +# define DESTROY_RWLOCK(x) (void)(x) #else # include @@ -40,6 +49,14 @@ static inline void init_recursive_mutex(pthread_mutex_t *x) # define LOCK(x) pthread_mutex_lock(x) # define UNLOCK(x) pthread_mutex_unlock(x) # define DESTROY_LOCK(x) pthread_mutex_destroy(x) + +typedef pthread_rwlock_t rwlock_t; +# define INIT_RWLOCK(x) pthread_rwlock_init(&(x), NULL) +# define RDLOCK(x) pthread_rwlock_rdlock(x) +# define RDUNLOCK(x) pthread_rwlock_unlock(x) +# define WRLOCK(x) pthread_rwlock_wrlock(x) +# define WRUNLOCK(x) pthread_rwlock_unlock(x) +# define DESTROY_RWLOCK(x) pthread_rwlock_destroy(x) #endif __attribute__((unused)) static void objc_release_lock(void *x) @@ -113,6 +130,50 @@ class RecursiveMutex UNLOCK(&mutex); } }; + +/** + * A lock that many readers may hold at once, or one writer exclusively. It is + * not recursive: a thread that holds it must not acquire it again. + */ +class ReadWriteLock +{ + /// The underlying lock + rwlock_t rwlock; + + public: + /** + * Explicit initialisation of the underlying lock, so that this can be a + * global. + */ + void init() + { + INIT_RWLOCK(rwlock); + } + + /// Acquire the lock for writing. + void lock() + { + WRLOCK(&rwlock); + } + + /// Release the lock after writing. + void unlock() + { + WRUNLOCK(&rwlock); + } + + /// Acquire the lock for reading. + void lock_shared() + { + RDLOCK(&rwlock); + } + + /// Release the lock after reading. + void unlock_shared() + { + RDUNLOCK(&rwlock); + } +}; #endif #endif // __LIBOBJC_LOCK_H_INCLUDED__ diff --git a/selector_table.cc b/selector_table.cc index 5fc68af8..bc57ecee 100644 --- a/selector_table.cc +++ b/selector_table.cc @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include "class.h" @@ -96,13 +97,18 @@ struct TypeList : public std::forward_list std::vector *selector_list; /** - * Lock protecting the selector table. + * Lock protecting the selector table. Registration is the only writer, so + * lookups share it. It is not recursive: anything called with it held must + * use the _locked form. */ -RecursiveMutex selector_table_lock; +ReadWriteLock selector_table_lock; -/// Type to use as a lock guard +/// Type to use as a lock guard for registration using LockGuard = std::lock_guard; +/// Type to use as a lock guard for lookup +using ReadGuard = std::shared_lock; + inline TypeList *selLookup_locked(uint32_t idx) { if (idx >= selector_list->size()) @@ -114,7 +120,7 @@ inline TypeList *selLookup_locked(uint32_t idx) inline TypeList *selLookup(uint32_t idx) { - LockGuard g{selector_table_lock}; + ReadGuard g{selector_table_lock}; return selLookup_locked(idx); } @@ -367,14 +373,19 @@ extern "C" PRIVATE void init_selector_tables() selector_table_lock.init(); } -static SEL selector_lookup(const char *name, const char *types) +static SEL selector_lookup_locked(const char *name, const char *types) { UnregisteredSelector sel = {name, types}; - LockGuard g{selector_table_lock}; auto result = selector_table->find(sel); return (result == selector_table->end()) ? nullptr : *result; } +static SEL selector_lookup(const char *name, const char *types) +{ + ReadGuard g{selector_table_lock}; + return selector_lookup_locked(name, types); +} + static inline void add_selector_to_table(SEL aSel) { // Store the name at the head of the list. @@ -404,7 +415,7 @@ static inline void register_selector_locked(SEL aSel) objc_resize_dtables(selector_list->size()); return; } - SEL untyped = selector_lookup(aSel->name, 0); + SEL untyped = selector_lookup_locked(aSel->name, 0); // If this has a type encoding, store the untyped version too. if (untyped == nullptr) { @@ -476,7 +487,7 @@ SEL objc_register_selector_copy(UnregisteredSelector &aSel, BOOL copyArgs) // registration; see objc_register_selector above and gnustep/libobjc2#391. LOCK_RUNTIME_FOR_SCOPE(); LockGuard g{selector_table_lock}; - copy = selector_lookup(aSel.name, aSel.types); + copy = selector_lookup_locked(aSel.name, aSel.types); if (nullptr != copy && selector_identical(aSel, copy)) { return copy; @@ -488,10 +499,10 @@ SEL objc_register_selector_copy(UnregisteredSelector &aSel, BOOL copyArgs) copy->types = (nullptr == aSel.types) ? nullptr : aSel.types; if (copyArgs) { - SEL untyped = selector_lookup(aSel.name, 0); + SEL untyped = selector_lookup_locked(aSel.name, 0); if (untyped != nullptr) { - copy->name = sel_getName(untyped); + copy->name = sel_getNameNonUnique(untyped); } else {