From e653c3c234c782c77c901919dab71f7922f9fd8c Mon Sep 17 00:00:00 2001 From: James Dong Date: Thu, 7 May 2009 11:33:37 -0700 Subject: [PATCH] OC2 oscl registry race condition The idea is to only initialize the singleton table for once and only once and keep it around forever. As a result, the lock for locking the whole singleton table is not needed and thus removed in this change. This approach works only if the client program follows the protocol for using the singleton: it must call initialize() first and then call the rest methods in this class. - changed the pointer to a singletontable to be a static object so that it is initialized statically - removed an extra lock on the whole singletontable, since we don't need it - moved initialize and cleanup to the header file for optimization. actually, these two methods can be removed. I put the removal of these two methods as a FIXME. --- oscl/oscl/osclbase/src/oscl_singleton.cpp | 104 ++++-------------------------- oscl/oscl/osclbase/src/oscl_singleton.h | 12 ++-- 2 files changed, 19 insertions(+), 97 deletions(-) diff --git a/oscl/oscl/osclbase/src/oscl_singleton.cpp b/oscl/oscl/osclbase/src/oscl_singleton.cpp index 310b02e2..003b4031 100644 --- a/oscl/oscl/osclbase/src/oscl_singleton.cpp +++ b/oscl/oscl/osclbase/src/oscl_singleton.cpp @@ -25,75 +25,17 @@ #include "oscl_lock_base.h" #include "oscl_base_alloc.h" - -OsclSingletonRegistry::SingletonTable* OsclSingletonRegistry::iSingletonTable = NULL; - - -OSCL_EXPORT_REF void OsclSingletonRegistry::initialize(Oscl_DefAlloc &alloc, int32 &aError) -{ - aError = 0; - //Allocate the registry on the first init call - //Note: there's some chance of thread contention here, since - //thread lock isn't available until after this step. - if (!iSingletonTable) - { - OsclAny* table = alloc.allocate(sizeof(SingletonTable)); - if (table) - iSingletonTable = new(table) SingletonTable(); - else - { - aError = EPVErrorBaseOutOfMemory; - return; - } - } - - //increment the ref count on each init. - iSingletonTable->iTableLock.Lock(); - iSingletonTable->iRefCount++; - iSingletonTable->iTableLock.Unlock(); -} - -OSCL_EXPORT_REF void OsclSingletonRegistry::cleanup(Oscl_DefAlloc &alloc, int32 &aError) -{ - aError = 0; - if (!iSingletonTable) - { - aError = EPVErrorBaseNotInstalled;//no table! - return; - } - - //decrement the ref count and cleanup when it reaches zero. - iSingletonTable->iTableLock.Lock(); - iSingletonTable->iRefCount--; - if (iSingletonTable->iRefCount == 0) - { - //cleanup - iSingletonTable->iTableLock.Unlock(); - iSingletonTable->~SingletonTable(); - alloc.deallocate(iSingletonTable); - iSingletonTable = NULL; - } - else - { - iSingletonTable->iTableLock.Unlock(); - } -} +// static allocation of the sSingletonTable object and keep it forever +OsclSingletonRegistry::SingletonTable OsclSingletonRegistry::sSingletonTable; OSCL_EXPORT_REF OsclAny* OsclSingletonRegistry::getInstance(uint32 ID, int32 &aError) { OSCL_ASSERT(ID < OSCL_SINGLETON_ID_LAST); aError = 0; - if (!iSingletonTable) - { - aError = EPVErrorBaseNotInstalled;//no table! - return NULL; - } - - iSingletonTable->iSingletonLocks[ID].Lock(); - OsclAny* value = iSingletonTable->iSingletons[ID]; - iSingletonTable->iSingletonLocks[ID].Unlock(); - + sSingletonTable.iSingletonLocks[ID].Lock(); + OsclAny* value = sSingletonTable.iSingletons[ID]; + sSingletonTable.iSingletonLocks[ID].Unlock(); return value; } @@ -102,15 +44,9 @@ OSCL_EXPORT_REF void OsclSingletonRegistry::registerInstance(OsclAny* ptr, uint3 OSCL_ASSERT(ID < OSCL_SINGLETON_ID_LAST); aError = 0; - if (!iSingletonTable) - { - aError = EPVErrorBaseNotInstalled;//no table! - return; - } - - iSingletonTable->iSingletonLocks[ID].Lock(); - iSingletonTable->iSingletons[ID] = ptr; - iSingletonTable->iSingletonLocks[ID].Unlock(); + sSingletonTable.iSingletonLocks[ID].Lock(); + sSingletonTable.iSingletons[ID] = ptr; + sSingletonTable.iSingletonLocks[ID].Unlock(); } OSCL_EXPORT_REF OsclAny* OsclSingletonRegistry::lockAndGetInstance(uint32 ID, int32 &aError) @@ -118,16 +54,9 @@ OSCL_EXPORT_REF OsclAny* OsclSingletonRegistry::lockAndGetInstance(uint32 ID, in OSCL_ASSERT(ID < OSCL_SINGLETON_ID_LAST); aError = 0; - - if (!iSingletonTable) - { - aError = EPVErrorBaseNotInstalled;//no table! - return NULL; - } - - iSingletonTable->iSingletonLocks[ID].Lock(); - OsclAny* value = iSingletonTable->iSingletons[ID]; - //leave it locked. + sSingletonTable.iSingletonLocks[ID].Lock(); + OsclAny* value = sSingletonTable.iSingletons[ID]; + //leave this table entry locked return value; } @@ -138,16 +67,9 @@ OSCL_EXPORT_REF void OsclSingletonRegistry::registerInstanceAndUnlock(OsclAny* p aError = 0; - if (!iSingletonTable) - { - aError = EPVErrorBaseNotInstalled;//no table! - return; - } - //assume it's already locked. - - iSingletonTable->iSingletons[ID] = ptr; - iSingletonTable->iSingletonLocks[ID].Unlock(); + sSingletonTable.iSingletons[ID] = ptr; + sSingletonTable.iSingletonLocks[ID].Unlock(); } #endif //OSCL_HAS_SINGLETON_SUPPORT diff --git a/oscl/oscl/osclbase/src/oscl_singleton.h b/oscl/oscl/osclbase/src/oscl_singleton.h index ec3a2933..8e797081 100644 --- a/oscl/oscl/osclbase/src/oscl_singleton.h +++ b/oscl/oscl/osclbase/src/oscl_singleton.h @@ -113,26 +113,26 @@ class OsclSingletonRegistry typedef registry_type* registry_pointer_type; private: - OSCL_IMPORT_REF static void initialize(Oscl_DefAlloc &alloc, int32 &error); - OSCL_IMPORT_REF static void cleanup(Oscl_DefAlloc &alloc, int32 &error); + // FIXME: + // these methods are obsolete and can be removed + OSCL_IMPORT_REF static void initialize(Oscl_DefAlloc &alloc, int32 &error) { error = 0; } + OSCL_IMPORT_REF static void cleanup(Oscl_DefAlloc &alloc, int32 &error) { error = 0; } friend class OsclBase; private: class SingletonTable { public: - SingletonTable(): iRefCount(0) + SingletonTable() { for (uint32 i = 0;i < OSCL_SINGLETON_ID_LAST;i++) iSingletons[i] = NULL; } - _OsclBasicLock iTableLock; - uint32 iRefCount; OsclAny* iSingletons[OSCL_SINGLETON_ID_LAST]; _OsclBasicLock iSingletonLocks[OSCL_SINGLETON_ID_LAST]; }; //The singleton table is a global variable. - static SingletonTable* iSingletonTable; + static SingletonTable sSingletonTable; }; template < class T, uint32 ID, class Registry = OsclSingletonRegistry > class OsclSingleton -- 2.11.0