From c01f349e6d41177d7be185b217eac237bcee621f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Axel=20D=C3=B6rfler?= Date: Fri, 26 Jan 2007 12:42:18 +0000 Subject: [PATCH] Cleanup: * Got rid of unused BLooper members * renamed fTaskID to fThread * Removed private and deprecated AddLooper()/RemoveLooper()/... stuff; BLooper is now directly calling BLooperList methods. * Got rid of extensive and useless comments * Made a few TODOs more clear * Merged InitData() and InitData(...) to _InitData(...) * BLooper::Team() now uses BPrivate::current_team(), sTeamID is gone now. git-svn-id: file:///srv/svn/repos/haiku/haiku/trunk@19966 a95241bf-73f2-0310-859d-f6bbb57e9c96 --- headers/os/app/Looper.h | 40 +--- src/kits/app/Application.cpp | 13 +- src/kits/app/Looper.cpp | 345 +++++++++------------------------- src/kits/interface/Window.cpp | 27 +-- 4 files changed, 113 insertions(+), 312 deletions(-) diff --git a/headers/os/app/Looper.h b/headers/os/app/Looper.h index 1b632a3bff..bf57e1a62d 100644 --- a/headers/os/app/Looper.h +++ b/headers/os/app/Looper.h @@ -1,5 +1,5 @@ /* - * Copyright 2001-2006, Haiku Inc. All Rights Reserved. + * Copyright 2001-2007, Haiku Inc. All Rights Reserved. * Distributed under the terms of the MIT License. * * Authors: @@ -41,9 +41,9 @@ public: status_t PostMessage(uint32 command); status_t PostMessage(BMessage* message); status_t PostMessage(uint32 command, BHandler* handler, - BHandler* reply_to = NULL); + BHandler* replyTo = NULL); status_t PostMessage(BMessage* message, BHandler* handler, - BHandler* reply_to = NULL); + BHandler* replyTo = NULL); virtual void DispatchMessage(BMessage* message, BHandler* handler); virtual void MessageReceived(BMessage* msg); @@ -72,7 +72,7 @@ public: status_t LockWithTimeout(bigtime_t timeout); thread_id Thread() const; team_id Team() const; - static BLooper* LooperForThread(thread_id tid); + static BLooper* LooperForThread(thread_id thread); // Loop debugging thread_id LockingThread() const; @@ -92,7 +92,7 @@ public: virtual void SetCommonFilterList(BList* filters); BList* CommonFilterList() const; - // Private or reserved --------------------------------------------------------- + // Private or reserved virtual status_t Perform(perform_code d, void* arg); protected: @@ -108,8 +108,6 @@ private: friend class BHandler; friend class BPrivate::BLooperList; friend port_id _get_looper_port_(const BLooper* ); - friend status_t _safe_get_server_token_(const BLooper* , int32* ); - friend team_id _find_cur_team_id_(); virtual void _ReservedLooper1(); virtual void _ReservedLooper2(); @@ -132,8 +130,7 @@ private: static status_t _LockComplete(BLooper* loop, int32 old, thread_id this_tid, sem_id sem, bigtime_t timeout); - void InitData(); - void InitData(const char* name, int32 prio, int32 capacity); + void _InitData(const char* name, int32 priority, int32 capacity); void AddMessage(BMessage* msg); void _AddMessagePriv(BMessage* msg); static status_t _task0_(void* arg); @@ -153,38 +150,21 @@ private: BHandler* resolve_specifier(BHandler* target, BMessage* msg); void UnlockFully(); - static uint32 sLooperID; - static team_id sTeamID; - -// DEPRECATED - static void AddLooper(BLooper* l); - static bool IsLooperValid(const BLooper* l); - static void RemoveLooper(BLooper* l); - static void GetLooperList(BList* list); - static BLooper* LooperForName(const char* name); - static BLooper* LooperForPort(port_id port); - - uint32 fLooperID; BMessageQueue* fQueue; BMessage* fLastMessage; port_id fMsgPort; - long fAtomicCount; + int32 fAtomicCount; sem_id fLockSem; - long fOwnerCount; + int32 fOwnerCount; thread_id fOwner; - thread_id fTaskID; - uint32 _unused1; + thread_id fThread; int32 fInitPriority; BHandler* fPreferred; BList fHandlers; BList* fCommonFilters; bool fTerminating; bool fRunCalled; - thread_id fCachedPid; - size_t fCachedStack; - void* fMsgBuffer; - size_t fMsgBufferSize; - uint32 _reserved[6]; + uint32 _reserved[12]; }; #endif // _LOOPER_H diff --git a/src/kits/app/Application.cpp b/src/kits/app/Application.cpp index 050a5d0197..c94c1519e1 100644 --- a/src/kits/app/Application.cpp +++ b/src/kits/app/Application.cpp @@ -1,10 +1,11 @@ /* - * Copyright 2001-2006, Haiku. + * Copyright 2001-2007, Haiku. * Distributed under the terms of the MIT License. * * Authors: * Erik Jaesler (erik@cgsoftware.com) * Jerome Duval + * Axel Dörfler, axeld@pinc-software.de */ @@ -483,17 +484,13 @@ BApplication::Run() if (fRunCalled) debugger("BApplication::Run was already called. Can only be called once."); - // Note: We need a local variable too (for the return value), since - // fTaskID is cleared by Quit(). -// ToDo: actually, it's not clobbered there?! - thread_id thread = fTaskID = find_thread(NULL); - + fThread = find_thread(NULL); fRunCalled = true; task_looper(); delete fPulseRunner; - return thread; + return fThread; } @@ -514,7 +511,7 @@ BApplication::Quit() // Delete the object, if not running only. if (!fRunCalled) { delete this; - } else if (find_thread(NULL) != fTaskID) { + } else if (find_thread(NULL) != fThread) { // ToDo: why shouldn't we set fTerminating to true directly in this case? // We are not the looper thread. // We push a _QUIT_ into the queue. diff --git a/src/kits/app/Looper.cpp b/src/kits/app/Looper.cpp index 4012fe6f41..3bcd6be8f4 100644 --- a/src/kits/app/Looper.cpp +++ b/src/kits/app/Looper.cpp @@ -11,13 +11,23 @@ /*! BLooper class spawns a thread that runs a message loop. */ -/** - @note Although I'm implementing "by the book" for now, I would like to - refactor sLooperList and all of the functions that operate on it - into their own class in the BPrivate namespace. +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include + +#include +#include - Also considering adding the thread priority when archiving. - */ // debugging //#define DBG(x) x @@ -35,21 +45,6 @@ static BLocker sDebugPrintLocker("BLooper debug print"); }) */ -#include - -#include -#include -#include -#include -#include -#include -#include - -#include -#include -#include -#include - #define FILTER_LIST_BLOCK_SIZE 5 #define DATA_BLOCK_SIZE 5 @@ -62,9 +57,6 @@ using BPrivate::BLooperList; port_id _get_looper_port_(const BLooper* looper); -uint32 BLooper::sLooperID = (uint32)B_ERROR; -team_id BLooper::sTeamID = (team_id)B_ERROR; - enum { BLOOPER_PROCESS_INTERNALLY = 0, BLOOPER_HANDLER_BY_INDEX @@ -113,7 +105,7 @@ struct _loop_data_ { BLooper::BLooper(const char* name, int32 priority, int32 port_capacity) : BHandler(name) { - InitData(name, priority, port_capacity); + _InitData(name, priority, port_capacity); } @@ -136,11 +128,12 @@ BLooper::~BLooper() if (fMsgPort >= 0) close_port(fMsgPort); - BMessage *msg; // Clear the queue so our call to IsMessageWaiting() below doesn't give // us bogus info - while ((msg = fQueue->NextMessage()) != NULL) { - delete msg; // msg will automagically post generic reply + BMessage *message; + while ((message = fQueue->NextMessage()) != NULL) { + delete message; + // msg will automagically post generic reply } do { @@ -166,7 +159,7 @@ BLooper::~BLooper() } Unlock(); - RemoveLooper(this); + gLooperList.RemoveLooper(this); delete_sem(fLockSem); } @@ -179,7 +172,7 @@ BLooper::BLooper(BMessage *data) || portCapacity < 0) portCapacity = B_LOOPER_PORT_DEFAULT_CAPACITY; - InitData(Name(), B_NORMAL_PRIORITY, portCapacity); + _InitData(Name(), B_NORMAL_PRIORITY, portCapacity); } @@ -205,6 +198,8 @@ BLooper::Archive(BMessage *data, bool deep) const if (status == B_OK) status = data->AddInt32("_port_cap", info.capacity); + // TODO: what about the thread priority? + return status; } @@ -245,18 +240,7 @@ void BLooper::DispatchMessage(BMessage *message, BHandler *handler) { PRINT(("BLooper::DispatchMessage(%.4s)\n", (char*)&message->what)); - /** @note - Initially, DispatchMessage() was locking the looper, calling the - filtering API, determining whether to use fPreferred or not, and - deleting the message. A look at the BeBook, however, reveals that - all this function does is handle its own B_QUIT_REQUESTED messages - and pass everything else to handler->MessageReceived(). Clearly the - rest must be happening in task_looper(). This makes a lot of sense - because otherwise every derived class would have to figure out when - to use fPreferred, handle the locking and filtering and delete the - message. Even if the BeBook didn't say as much, it would make total - sense to hoist that functionality out of here and into task_looper(). - */ + switch (message->what) { case _QUIT_: // Can't call Quit() to do this, because of the slight chance @@ -286,10 +270,7 @@ BLooper::DispatchMessage(BMessage *message, BHandler *handler) void BLooper::MessageReceived(BMessage *msg) { - // TODO: verify - // The BeBook says this "simply calls the inherited function. ...the BLooper - // implementation does nothing of importance." Which is not the same as - // saying it does nothing. Investigate. + // TODO: implement scripting support BHandler::MessageReceived(msg); } @@ -325,17 +306,6 @@ BLooper::IsMessageWaiting() const if (!fQueue->IsEmpty()) return true; -/** - @note: What we're doing here differs slightly from the R5 implementation. - It appears that they probably return count != 0, which gives an - incorrect true result when port_buffer_size_etc() would block -- - which indicates that the port's buffer is empty, so we should return - false. Since we don't actually care about what the error is, we - just return count > 0. This has some interesting consequences in - that we will correctly return 'false' if the port is empty - (B_WOULD_BLOCK), whereas R5 will return true. We call that a bug - where I come from. ;) - */ int32 count; do { count = port_buffer_size_etc(fMsgPort, B_RELATIVE_TIMEOUT, 0); @@ -436,12 +406,12 @@ BLooper::Run() if (fRunCalled) { // Not allowed to call Run() more than once debugger("can't call BLooper::Run twice!"); - return fTaskID; + return fThread; } - fTaskID = spawn_thread(_task0_, Name(), fInitPriority, this); - if (fTaskID < B_OK) - return fTaskID; + fThread = spawn_thread(_task0_, Name(), fInitPriority, this); + if (fThread < B_OK) + return fThread; if (fMsgPort < B_OK) return fMsgPort; @@ -449,11 +419,11 @@ BLooper::Run() fRunCalled = true; Unlock(); - status_t err = resume_thread(fTaskID); + status_t err = resume_thread(fThread); if (err < B_OK) return err; - return fTaskID; + return fThread; } @@ -479,7 +449,7 @@ BLooper::Quit() PRINT((" Run() has not been called yet\n")); fTerminating = true; delete this; - } else if (find_thread(NULL) == fTaskID) { + } else if (find_thread(NULL) == fThread) { PRINT((" We are the looper thread\n")); fTerminating = true; delete this; @@ -569,16 +539,7 @@ PRINT(("BLooper::Unlock() done\n")); bool BLooper::IsLocked() const { - // We have to lock the list for the call to IsLooperValid(). Has the side - // effect of not letting the looper get deleted while we're here. - BObjectLocker ListLock(gLooperList); - - if (!ListLock.IsLocked()) { - // If we can't lock the list, our semaphore is probably toast - return false; - } - - if (!IsLooperValid(this)) { + if (!gLooperList.IsLooperValid(this)) { // The looper is gone, so of course it's not locked return false; } @@ -598,25 +559,21 @@ BLooper::LockWithTimeout(bigtime_t timeout) thread_id BLooper::Thread() const { - return fTaskID; + return fThread; } team_id BLooper::Team() const { - return sTeamID; + return BPrivate::current_team(); } BLooper* -BLooper::LooperForThread(thread_id tid) +BLooper::LooperForThread(thread_id thread) { - BObjectLocker ListLock(gLooperList); - if (ListLock.IsLocked()) - return gLooperList.LooperForThread(tid); - - return NULL; + return gLooperList.LooperForThread(thread); } @@ -856,7 +813,7 @@ BLooper::BLooper(int32 priority, port_id port, const char* name) { // This must be a legacy constructor fMsgPort = port; - InitData(name, priority, B_LOOPER_PORT_DEFAULT_CAPACITY); + _InitData(name, priority, B_LOOPER_PORT_DEFAULT_CAPACITY); } @@ -868,7 +825,7 @@ BLooper::_PostMessage(BMessage *msg, BHandler *handler, if (!listLocker.IsLocked()) return B_ERROR; - if (!IsLooperValid(this)) + if (!gLooperList.IsLooperValid(this)) return B_BAD_VALUE; // Does handler belong to this looper? @@ -884,120 +841,66 @@ BLooper::_PostMessage(BMessage *msg, BHandler *handler, } -status_t -BLooper::_Lock(BLooper* loop, port_id port, bigtime_t timeout) -{ -PRINT(("BLooper::_Lock(%p, %lx)\n", loop, port)); -/** - @note The assumption I'm under here is that since we can get the port of - the BLooper directly from the BLooper itself, the port parameter is - for identifying BLoopers by port_id when a pointer to the BLooper in - question is not available. So this function has two modes: - o When loop != NULL, use it directly - o When loop == NULL and port is valid, use the port_id to get - the looper - I scoured the docs to find out what constitutes a valid port_id to - no avail. Since create_port uses the standard error values in its - returned port_id, I'll assume that anything less than zero is a safe - bet as an *invalid* port_id. I'm guessing that, like thread and - semaphore ids, anything >= zero is valid. So, the short version of - this reads: if you don't want to find by port_id, make port = -1. +/*! + Locks a looper either by port or using a direct pointer to the looper. - Another assumption I'm making is that Lock() and LockWithTimeout() - are covers for this function. If it turns out that we don't really - need this function, I may refactor this code into LockWithTimeout() - and have Lock() call it instead. This function could then be - removed. - */ + \param looper looper to lock, if not NULL + \param port port to identify the looper in case \a looper is NULL + \param timeout timeout for acquiring the lock +*/ +status_t +BLooper::_Lock(BLooper* looper, port_id port, bigtime_t timeout) +{ + PRINT(("BLooper::_Lock(%p, %lx)\n", looper, port)); // Check params (loop, port) - if (!loop && port < 0) - { -PRINT(("BLooper::_Lock() done 1\n")); + if (looper == NULL && port < 0) { + PRINT(("BLooper::_Lock() done 1\n")); return B_BAD_VALUE; } - // forward declared so I can use BAutolock on sLooperListLock - thread_id curThread; + thread_id currentThread = find_thread(NULL); + int32 oldCount; sem_id sem; -/** - @note We lock the looper list at the start of the lock operation to - prevent the looper getting removed from the list while we're - doing list operations. Also ensures that the looper doesn't - get deleted here (since ~BLooper() has to lock the list as - well to remove itself). - */ - int32 oldCount; { BObjectLocker ListLock(gLooperList); if (!ListLock.IsLocked()) - { - // If we can't lock, the semaphore is probably - // gone, which leaves us in no-man's land -PRINT(("BLooper::_Lock() done 2\n")); + return B_BAD_VALUE; + + // Look up looper by port_id, if necessary + if (looper == NULL) { + looper = gLooperList.LooperForPort(port); + if (looper == NULL) { + PRINT(("BLooper::_Lock() done 3\n")); + return B_BAD_VALUE; + } + } else if (!gLooperList.IsLooperValid(looper)) { + // Check looper validity + PRINT(("BLooper::_Lock() done 4\n")); return B_BAD_VALUE; } - - // Look up looper by port_id, if necessary - if (!loop) - { - loop = LooperForPort(port); - if (!loop) - { -PRINT(("BLooper::_Lock() done 3\n")); - return B_BAD_VALUE; - } - } - else - { - // Check looper validity - if (!IsLooperValid(loop)) - { -PRINT(("BLooper::_Lock() done 4\n")); - return B_BAD_VALUE; - } - } - - // Is the looper trying to lock itself? - // Check for nested lock attempt - curThread = find_thread(NULL); - if (curThread == loop->fOwner) - { - // Bump fOwnerCount - ++loop->fOwnerCount; -PRINT(("BLooper::_Lock() done 5: fOwnerCount: %ld\n", loop->fOwnerCount)); + + // Check for nested lock attempt + if (currentThread == looper->fOwner) { + ++looper->fOwnerCount; + PRINT(("BLooper::_Lock() done 5: fOwnerCount: %ld\n", loop->fOwnerCount)); return B_OK; } - - // Something external to the looper is attempting to lock - // Cache the semaphore - sem = loop->fLockSem; - - // Validate the semaphore - if (sem < 0) - { -PRINT(("BLooper::_Lock() done 6\n")); + + // Cache the semaphore, so that we can safely access it after having + // unlocked the looper list + sem = looper->fLockSem; + if (sem < 0) { + PRINT(("BLooper::_Lock() done 6\n")); return B_BAD_VALUE; } - - // Bump the requested lock count (using fAtomicCount for this) - oldCount = atomic_add(&loop->fAtomicCount, 1); - // sLooperListLock automatically released here + // Bump the requested lock count (using fAtomicCount for this) + oldCount = atomic_add(&looper->fAtomicCount, 1); } -/** - @note We have to operate with the looper list unlocked during semaphore - acquisition so that the rest of the application doesn't have to - wait for this lock to happen. This is why we cached fLockSem - earlier -- with the list unlocked, the looper might get deleted - right out from under us. This is also why we use a raw semaphore - instead of the easier-to-deal-with BLocker; you can't cache a - BLocker. - */ - // acquire the lock for real - return _LockComplete(loop, oldCount, curThread, sem, timeout); + return _LockComplete(looper, oldCount, currentThread, sem, timeout); } @@ -1009,25 +912,24 @@ BLooper::_LockComplete(BLooper *looper, int32 oldCount, thread_id thread, sem_id #if DEBUG < 1 if (oldCount > 0) { #endif - do { + do { err = acquire_sem_etc(sem, 1, B_RELATIVE_TIMEOUT, timeout); } while (err == B_INTERRUPTED); #if DEBUG < 1 } -#endif +#endif if (err == B_OK) { - // Assign current thread to fOwner looper->fOwner = thread; - // Reset fOwnerCount to 1 looper->fOwnerCount = 1; } -PRINT(("BLooper::_LockComplete() done: %lx\n", err)); + + PRINT(("BLooper::_LockComplete() done: %lx\n", err)); return err; } void -BLooper::InitData() +BLooper::_InitData(const char *name, int32 priority, int32 portCapacity) { fOwner = B_ERROR; fRunCalled = false; @@ -1035,24 +937,11 @@ BLooper::InitData() fCommonFilters = NULL; fLastMessage = NULL; fPreferred = NULL; - fTaskID = B_ERROR; + fThread = B_ERROR; fTerminating = false; fMsgPort = -1; fAtomicCount = 0; - if (sTeamID == -1) { - thread_info info; - get_thread_info(find_thread(NULL), &info); - sTeamID = info.team; - } -} - - -void -BLooper::InitData(const char *name, int32 priority, int32 portCapacity) -{ - InitData(); - if (name == NULL) name = "anonymous looper"; @@ -1069,8 +958,7 @@ BLooper::InitData(const char *name, int32 priority, int32 portCapacity) fInitPriority = priority; - BObjectLocker ListLock(gLooperList); - AddLooper(this); + gLooperList.AddLooper(this); AddHandler(this); } @@ -1325,7 +1213,7 @@ BLooper::_QuitRequested(BMessage *msg) || (msg->FindBool("_shutdown_", &shutdown) == B_OK && shutdown)) { BMessage replyMsg(B_REPLY); replyMsg.AddBool("result", isQuitting); - replyMsg.AddInt32("thread", fTaskID); + replyMsg.AddInt32("thread", fThread); msg->SendReply(&replyMsg); } @@ -1515,61 +1403,6 @@ BLooper::UnlockFully() } -void -BLooper::AddLooper(BLooper *looper) -{ - if (gLooperList.IsLocked()) - gLooperList.AddLooper(looper); -} - - -bool -BLooper::IsLooperValid(const BLooper *looper) -{ - if (gLooperList.IsLocked()) - return gLooperList.IsLooperValid(looper); - - return false; -} - - -void -BLooper::RemoveLooper(BLooper *looper) -{ - if (gLooperList.IsLocked()) - gLooperList.RemoveLooper(looper); -} - - -void -BLooper::GetLooperList(BList* list) -{ - BObjectLocker ListLock(gLooperList); - if (ListLock.IsLocked()) - gLooperList.GetLooperList(list); -} - - -BLooper * -BLooper::LooperForName(const char* name) -{ - if (gLooperList.IsLocked()) - return gLooperList.LooperForName(name); - - return NULL; -} - - -BLooper * -BLooper::LooperForPort(port_id port) -{ - if (gLooperList.IsLocked()) - return gLooperList.LooperForPort(port); - - return NULL; -} - - // #pragma mark - diff --git a/src/kits/interface/Window.cpp b/src/kits/interface/Window.cpp index 0e0921f88b..890e494fcf 100644 --- a/src/kits/interface/Window.cpp +++ b/src/kits/interface/Window.cpp @@ -1066,7 +1066,7 @@ FrameMoved(origin); case _UPDATE_: { -//bigtime_t now = system_time(); + //bigtime_t now = system_time(); STRACE(("info:BWindow handling _UPDATE_.\n")); BRect updateRect; @@ -1088,14 +1088,14 @@ FrameMoved(origin); if (origin != fFrame.LeftTop()) { // TODO: remove code duplicatation with // B_WINDOW_MOVED case... -//printf("window position was not up to date\n"); + //printf("window position was not up to date\n"); fFrame.OffsetTo(origin); FrameMoved(origin); } if (width != fFrame.Width() || height != fFrame.Height()) { // TODO: remove code duplicatation with // B_WINDOW_RESIZED case... -//printf("window size was not up to date\n"); + //printf("window size was not up to date\n"); fFrame.right = fFrame.left + width; fFrame.bottom = fFrame.top + height; @@ -1136,7 +1136,7 @@ FrameMoved(origin); fLink->Flush(); fInTransaction = false; -//printf("BWindow(%s) - UPDATE took %lld usecs\n", Title(), system_time() - now); + //printf("BWindow(%s) - UPDATE took %lld usecs\n", Title(), system_time() - now); break; } @@ -2194,7 +2194,7 @@ BWindow::Show() if (fLink->SenderPort() < B_OK) { // We don't have valid app_server connection; there is no point // in starting our looper - fTaskID = B_ERROR; + fThread = B_ERROR; return; } else Run(); @@ -2517,18 +2517,11 @@ BWindow::task_looper() if (IsLocked()) debugger("window must not be locked!"); - // loop: As long as we are not terminating. while (!fTerminating) { - // TODO: timeout determination algo - // Read from message port (how do we determine what the timeout is?) - BMessage* msg = MessageFromPort(); - // Did we get a message? - if (msg) { - // Add to queue - fQueue->AddMessage(msg); - } else - continue; + BMessage* msg = MessageFromPort(); + if (msg) + _AddMessagePriv(msg); // Get message count from port int32 msgCount = port_count(fMsgPort); @@ -2538,11 +2531,9 @@ BWindow::task_looper() msg = MessageFromPort(0); // Add messages to queue if (msg) - fQueue->AddMessage(msg); + _AddMessagePriv(msg); } - // loop: As long as there are messages in the queue and the port is - // empty... and we are not terminating, of course. bool dispatchNextMessage = true; while (!fTerminating && dispatchNextMessage) { // Get next message from queue (assign to fLastMessage)