hrev53305 adds 2 changesets to branch 'master'
old head: 359cf105940e3a7cf71b02f225203db0a77e6cac
new head: fd97a8c7fe51576b851bc00ea6a49b76d4434dcf
overview:
https://git.haiku-os.org/haiku/log/?qt=range&q=fd97a8c7fe51+%5E359cf105940e
----------------------------------------------------------------------------
f9c77b11edcf: app_server: Clean up MultiLocker::IsWriteLocked().
This function was (or at least now is) severely over-engineered:
it is designed to avoid calling find_thread(NULL) as little
as possible, and use stack addresses to determine if the current
thread is the one holding the write lock.
However, this is unneccessary, as find_thread(NULL) has been
optimized (on x86 and x86_64, at least) to be a single "mov"
from thread-local data, with no syscall. So that is probably
even faster than an integer divide and compare, allowing
this function to be simplified greatly.
fd97a8c7fe51: app_server: Use a rw_lock instead of 3 (!) semaphores for
MultiLocker.
Since every HWInterface is a MultiLocker, and every BBitmap requires
a server-side BitmapHWInterface, this saves 3 semaphores per BBitmap
instance (as well as a lot of semaphore-related overhead in calling
the kernel so often.)
[ Augustin Cavalier <waddlesplash@xxxxxxxxx> ]
----------------------------------------------------------------------------
2 files changed, 49 insertions(+), 230 deletions(-)
src/servers/app/MultiLocker.cpp | 252 ++++++------------------------------
src/servers/app/MultiLocker.h | 27 ++--
############################################################################
Commit: f9c77b11edcf7eee58dd866cb89364def0778110
URL: https://git.haiku-os.org/haiku/commit/?id=f9c77b11edcf
Author: Augustin Cavalier <waddlesplash@xxxxxxxxx>
Date: Sun Jul 28 03:06:24 2019 UTC
app_server: Clean up MultiLocker::IsWriteLocked().
This function was (or at least now is) severely over-engineered:
it is designed to avoid calling find_thread(NULL) as little
as possible, and use stack addresses to determine if the current
thread is the one holding the write lock.
However, this is unneccessary, as find_thread(NULL) has been
optimized (on x86 and x86_64, at least) to be a single "mov"
from thread-local data, with no syscall. So that is probably
even faster than an integer divide and compare, allowing
this function to be simplified greatly.
----------------------------------------------------------------------------
diff --git a/src/servers/app/MultiLocker.cpp b/src/servers/app/MultiLocker.cpp
index 979a360635..cc9efb52c9 100644
--- a/src/servers/app/MultiLocker.cpp
+++ b/src/servers/app/MultiLocker.cpp
@@ -35,8 +35,7 @@ MultiLocker::MultiLocker(const char* baseName)
#endif
fInit(B_NO_INIT),
fWriterNest(0),
- fWriterThread(-1),
- fWriterStackBase(0)
+ fWriterThread(-1)
{
// build the semaphores
#if !DEBUG
@@ -126,30 +125,8 @@ MultiLocker::InitCheck()
}
-/*!
- This function demonstrates a nice method of determining if the current
thread
- is the writer or not. The method involves caching the index of the
page in memory
- where the thread's stack is located. Each time a new writer acquires
the lock,
- its thread_id and stack_page are recorded. IsWriteLocked gets the
stack_page of the
- current thread and sees if it is a match. If the stack_page matches
you are guaranteed
- to have the matching thread. If the stack page doesn't match the more
traditional
- find_thread(NULL) method of matching the thread_ids is used.
-
- This technique is very useful when dealing with a lock that is acquired
in a nested fashion.
- It could be expanded to cache the information of the last thread in the
lock, and then if
- the same thread returns while there is no one in the lock, it could
save some time, if the
- same thread is likely to acquire the lock again and again.
- I should note another shortcut that could be implemented here
- If fWriterThread is set to -1 then there is no writer in the lock, and
we could
- return from this function much faster. However the function is
currently set up
- so all of the stack_base and thread_id info is determined here.
WriteLock passes
- in some variables so that if the lock is not held it does not have to
get the thread_id
- and stack base again. Instead this function returns that information.
So this shortcut
- would only move this information gathering outside of this function,
and I like it all
- contained.
-*/
bool
-MultiLocker::IsWriteLocked(addr_t* _stackBase, thread_id* _thread) const
+MultiLocker::IsWriteLocked() const
{
#if TIMING
bigtime_t start = system_time();
@@ -158,30 +135,8 @@ MultiLocker::IsWriteLocked(addr_t* _stackBase, thread_id*
_thread) const
// get a variable on the stack
bool writeLockHolder = false;
- if (fInit == B_OK) {
- // determine which page in memory this stack represents
- // this is managed by taking the address of the item on the
- // stack and dividing it by the size of the memory pages
- // if it is the same as the cached stack_page, there is a match
- addr_t stackBase = (addr_t)&writeLockHolder / B_PAGE_SIZE;
- thread_id thread = 0;
-
- if (fWriterStackBase == stackBase) {
- writeLockHolder = true;
- } else {
- // as there was no stack page match we resort to the
- // tried and true methods
- thread = find_thread(NULL);
- if (fWriterThread == thread)
- writeLockHolder = true;
- }
-
- // if someone wants this information, give it to them
- if (_stackBase != NULL)
- *_stackBase = stackBase;
- if (_thread != NULL)
- *_thread = thread;
- }
+ if (fInit == B_OK)
+ writeLockHolder = (find_thread(NULL) == fWriterThread);
#if TIMING
bigtime_t end = system_time();
@@ -248,10 +203,7 @@ MultiLocker::WriteLock()
bool locked = false;
if (fInit == B_OK) {
- addr_t stackBase = 0;
- thread_id thread = -1;
-
- if (IsWriteLocked(&stackBase, &thread)) {
+ if (IsWriteLocked()) {
// already the writer - increment the nesting count
fWriterNest++;
locked = true;
@@ -286,8 +238,7 @@ MultiLocker::WriteLock()
if (locked) {
ASSERT(fWriterThread == -1);
// record thread information
- fWriterThread = thread;
- fWriterStackBase = stackBase;
+ fWriterThread = find_thread(NULL);
}
}
}
@@ -369,7 +320,6 @@ MultiLocker::WriteUnlock()
if (unlocked) {
// clear the information
fWriterThread = -1;
- fWriterStackBase = 0;
// decrement and retrieve the lock count
if (atomic_add(&fLockCount, -1) > 1) {
@@ -434,10 +384,7 @@ MultiLocker::WriteLock()
if (fInit != B_OK)
debugger("lock not initialized");
- addr_t stackBase = 0;
- thread_id thread = -1;
-
- if (IsWriteLocked(&stackBase, &thread)) {
+ if (IsWriteLocked()) {
if (fWriterNest < 0)
debugger("WriteLock() - negative writer nest count");
@@ -456,8 +403,7 @@ MultiLocker::WriteLock()
locked = status == B_OK;
if (locked) {
// record thread information
- fWriterThread = thread;
- fWriterStackBase = stackBase;
+ fWriterThread = find_thread(NULL);
}
}
@@ -501,7 +447,6 @@ MultiLocker::WriteUnlock()
} else {
// clear the information while still holding the lock
fWriterThread = -1;
- fWriterStackBase = 0;
unlocked = release_sem_etc(fLock, LARGE_NUMBER,
B_DO_NOT_RESCHEDULE) == B_OK;
}
diff --git a/src/servers/app/MultiLocker.h b/src/servers/app/MultiLocker.h
index eac9ea5ea6..54528f935c 100644
--- a/src/servers/app/MultiLocker.h
+++ b/src/servers/app/MultiLocker.h
@@ -56,9 +56,8 @@ public:
bool ReadUnlock();
bool WriteUnlock();
- // does the current thread hold a write lock ?
- bool IsWriteLocked(addr_t
*stackBase = NULL,
-
thread_id *thread = NULL) const;
+ // does the current thread hold a write lock?
+ bool IsWriteLocked() const;
#if MULTI_LOCKER_DEBUG
// in DEBUG mode returns whether the lock is held
@@ -97,7 +96,6 @@ private:
status_t fInit;
int32 fWriterNest;
thread_id fWriterThread;
- addr_t fWriterStackBase;
#if MULTI_LOCKER_TIMING
uint32 rl_count;
############################################################################
Revision: hrev53305
Commit: fd97a8c7fe51576b851bc00ea6a49b76d4434dcf
URL: https://git.haiku-os.org/haiku/commit/?id=fd97a8c7fe51
Author: Augustin Cavalier <waddlesplash@xxxxxxxxx>
Date: Sun Jul 28 03:53:45 2019 UTC
app_server: Use a rw_lock instead of 3 (!) semaphores for MultiLocker.
Since every HWInterface is a MultiLocker, and every BBitmap requires
a server-side BitmapHWInterface, this saves 3 semaphores per BBitmap
instance (as well as a lot of semaphore-related overhead in calling
the kernel so often.)
----------------------------------------------------------------------------
diff --git a/src/servers/app/MultiLocker.cpp b/src/servers/app/MultiLocker.cpp
index cc9efb52c9..3e758f5ed4 100644
--- a/src/servers/app/MultiLocker.cpp
+++ b/src/servers/app/MultiLocker.cpp
@@ -28,39 +28,20 @@ MultiLocker::MultiLocker(const char* baseName)
#if DEBUG
fDebugArray(NULL),
fMaxThreads(0),
-#else
- fReadCount(0),
- fWriteCount(0),
- fLockCount(0),
-#endif
- fInit(B_NO_INIT),
fWriterNest(0),
- fWriterThread(-1)
+ fWriterThread(-1),
+#endif
+ fInit(B_NO_INIT)
{
- // build the semaphores
#if !DEBUG
- if (baseName) {
- char name[128];
- snprintf(name, sizeof(name), "%s-%s", baseName, "ReadSem");
- fReadSem = create_sem(0, name);
- snprintf(name, sizeof(name), "%s-%s", baseName, "WriteSem");
- fWriteSem = create_sem(0, name);
- snprintf(name, sizeof(name), "%s-%s", baseName, "WriterLock");
- fWriterLock = create_sem(0, name);
- } else {
- fReadSem = create_sem(0, "MultiLocker_ReadSem");
- fWriteSem = create_sem(0, "MultiLocker_WriteSem");
- fWriterLock = create_sem(0, "MultiLocker_WriterLock");
- }
-
- if (fReadSem >= 0 && fWriteSem >=0 && fWriterLock >= 0)
- fInit = B_OK;
+ rw_lock_init(&fLock, baseName != NULL ? baseName : "some MultiLocker");
+ fInit = B_OK;
#else
+ // we are in debug mode!
fLock = create_sem(LARGE_NUMBER, baseName != NULL ? baseName :
"MultiLocker");
if (fLock >= 0)
fInit = B_OK;
- // we are in debug mode!
// create the reader tracking list
// the array needs to be large enough to hold all possible threads
system_info sys;
@@ -93,10 +74,7 @@ MultiLocker::~MultiLocker()
fInit = B_NO_INIT;
#if !DEBUG
- // delete the semaphores
- delete_sem(fReadSem);
- delete_sem(fWriteSem);
- delete_sem(fWriterLock);
+ rw_lock_destroy(&fLock);
#else
delete_sem(fLock);
free(fDebugArray);
@@ -125,6 +103,10 @@ MultiLocker::InitCheck()
}
+#if !DEBUG
+// #pragma mark - Standard versions
+
+
bool
MultiLocker::IsWriteLocked() const
{
@@ -132,11 +114,10 @@ MultiLocker::IsWriteLocked() const
bigtime_t start = system_time();
#endif
- // get a variable on the stack
bool writeLockHolder = false;
if (fInit == B_OK)
- writeLockHolder = (find_thread(NULL) == fWriterThread);
+ writeLockHolder = (find_thread(NULL) == fLock.holder);
#if TIMING
bigtime_t end = system_time();
@@ -148,10 +129,6 @@ MultiLocker::IsWriteLocked() const
}
-#if !DEBUG
-// #pragma mark - Standard versions
-
-
bool
MultiLocker::ReadLock()
{
@@ -159,29 +136,7 @@ MultiLocker::ReadLock()
bigtime_t start = system_time();
#endif
- bool locked = false;
-
- // the lock must be initialized
- if (fInit == B_OK) {
- if (IsWriteLocked()) {
- // the writer simply increments the nesting
- fWriterNest++;
- locked = true;
- } else {
- // increment and retrieve the current count of readers
- int32 currentCount = atomic_add(&fReadCount, 1);
- if (currentCount < 0) {
- // a writer holds the lock so wait for fReadSem
to be released
- status_t status;
- do {
- status = acquire_sem(fReadSem);
- } while (status == B_INTERRUPTED);
-
- locked = status == B_OK;
- } else
- locked = true;
- }
- }
+ bool locked = (rw_lock_read_lock(&fLock) == B_OK);
#if TIMING
bigtime_t end = system_time();
@@ -200,49 +155,7 @@ MultiLocker::WriteLock()
bigtime_t start = system_time();
#endif
- bool locked = false;
-
- if (fInit == B_OK) {
- if (IsWriteLocked()) {
- // already the writer - increment the nesting count
- fWriterNest++;
- locked = true;
- } else {
- // new writer acquiring the lock
- if (atomic_add(&fLockCount, 1) >= 1) {
- // another writer in the lock - acquire the
semaphore
- status_t status;
- do {
- status = acquire_sem(fWriterLock);
- } while (status == B_INTERRUPTED);
-
- locked = status == B_OK;
- } else
- locked = true;
-
- if (locked) {
- // new holder of the lock
-
- // decrement fReadCount by a very large number
- // this will cause new readers to block on
fReadSem
- int32 readers = atomic_add(&fReadCount,
-LARGE_NUMBER);
- if (readers > 0) {
- // readers hold the lock - acquire
fWriteSem
- status_t status;
- do {
- status =
acquire_sem_etc(fWriteSem, readers, 0, 0);
- } while (status == B_INTERRUPTED);
-
- locked = status == B_OK;
- }
- if (locked) {
- ASSERT(fWriterThread == -1);
- // record thread information
- fWriterThread = find_thread(NULL);
- }
- }
- }
- }
+ bool locked = (rw_lock_write_lock(&fLock) == B_OK);
#if TIMING
bigtime_t end = system_time();
@@ -261,21 +174,7 @@ MultiLocker::ReadUnlock()
bigtime_t start = system_time();
#endif
- bool unlocked = false;
-
- if (IsWriteLocked()) {
- // writers simply decrement the nesting count
- fWriterNest--;
- unlocked = true;
- } else {
- // decrement and retrieve the read counter
- int32 current_count = atomic_add(&fReadCount, -1);
- if (current_count < 0) {
- // a writer is waiting for the lock so release fWriteSem
- unlocked = release_sem_etc(fWriteSem, 1,
B_DO_NOT_RESCHEDULE) == B_OK;
- } else
- unlocked = true;
- }
+ bool unlocked = (rw_lock_read_unlock(&fLock) == B_OK);
#if TIMING
bigtime_t end = system_time();
@@ -294,43 +193,7 @@ MultiLocker::WriteUnlock()
bigtime_t start = system_time();
#endif
- bool unlocked = false;
-
- if (IsWriteLocked()) {
- // if this is a nested lock simply decrement the nest count
- if (fWriterNest > 0) {
- fWriterNest--;
- unlocked = true;
- } else {
- // writer finally unlocking
-
- // increment fReadCount by a large number
- // this will let new readers acquire the read lock
- // retrieve the number of current waiters
- int32 readersWaiting = atomic_add(&fReadCount,
LARGE_NUMBER)
- + LARGE_NUMBER;
-
- if (readersWaiting > 0) {
- // readers are waiting to acquire the lock
- unlocked = release_sem_etc(fReadSem,
readersWaiting,
- B_DO_NOT_RESCHEDULE) == B_OK;
- } else
- unlocked = true;
-
- if (unlocked) {
- // clear the information
- fWriterThread = -1;
-
- // decrement and retrieve the lock count
- if (atomic_add(&fLockCount, -1) > 1) {
- // other writers are waiting so release
fWriterLock
- unlocked = release_sem_etc(fWriterLock,
1,
- B_DO_NOT_RESCHEDULE) == B_OK;
- }
- }
- }
- } else
- debugger("Non-writer attempting to WriteUnlock()");
+ bool unlocked = (rw_lock_write_unlock(&fLock) == B_OK);
#if TIMING
bigtime_t end = system_time();
@@ -346,6 +209,28 @@ MultiLocker::WriteUnlock()
// #pragma mark - Debug versions
+bool
+MultiLocker::IsWriteLocked() const
+{
+#if TIMING
+ bigtime_t start = system_time();
+#endif
+
+ bool writeLockHolder = false;
+
+ if (fInit == B_OK)
+ writeLockHolder = (find_thread(NULL) == fWriterThread);
+
+#if TIMING
+ bigtime_t end = system_time();
+ islock_time += (end - start);
+ islock_count++;
+#endif
+
+ return writeLockHolder;
+}
+
+
bool
MultiLocker::ReadLock()
{
diff --git a/src/servers/app/MultiLocker.h b/src/servers/app/MultiLocker.h
index 54528f935c..382f47d241 100644
--- a/src/servers/app/MultiLocker.h
+++ b/src/servers/app/MultiLocker.h
@@ -21,6 +21,7 @@
#include <OS.h>
+#include <locks.h>
#define MULTI_LOCKER_TIMING 0
@@ -71,7 +72,9 @@ private:
MultiLocker& operator=(const MultiLocker&
other);
// not
implemented
-#if MULTI_LOCKER_DEBUG
+#if !MULTI_LOCKER_DEBUG
+ rw_lock fLock;
+#else
// functions for managing the DEBUG reader array
void _RegisterThread();
void _UnregisterThread();
@@ -79,23 +82,11 @@ private:
sem_id fLock;
int32* fDebugArray;
int32 fMaxThreads;
-#else
- // readers adjust count and block on fReadSem when a
writer
- // hold the lock
- int32 fReadCount;
- sem_id fReadSem;
- // writers adjust the count and block on fWriteSem
- // when readers hold the lock
- int32 fWriteCount;
- sem_id fWriteSem;
- // writers must acquire fWriterLock when acquiring a
write lock
- int32 fLockCount;
- sem_id fWriterLock;
+ int32 fWriterNest;
+ thread_id fWriterThread;
#endif // MULTI_LOCKER_DEBUG
status_t fInit;
- int32 fWriterNest;
- thread_id fWriterThread;
#if MULTI_LOCKER_TIMING
uint32 rl_count;
