Refactor RegistryKeyList

This is in preparation for page patching. The old code kept dynamic
(programatically created) and serialized keys separate. This had the
potential to reorder the key list during the application of a patch.

This is separate from the prp-patches branch to facillitate review.

Sources/Plasma/NucleusLib/pnKeyedObject/hsKeyedObject.cpp

@@ -107,6 +107,10 @@ plKey hsKeyedObject::RegisterAs(plFixedKeyId fixedKey)
     if (key == nil)
     {
         key = hsgResMgr::ResMgr()->NewKey(meUoid, this);
+
+        //  the key list "helpfully" assigns us an object id.
+        // we don't want one for fixed keys however (initialization order might bite us in the ass)
+        static_cast<plKeyImp*>(key)->SetObjectID(0);
     }
     else
     {

Sources/Plasma/PubUtilLib/plResMgr/plKeyFinder.cpp

@@ -438,15 +438,9 @@ plKey plKeyFinder::IFindSceneNodeKey(plRegistryPageNode* page) const
     plRegistryKeyList* keyList = page->IGetKeyList(CLASS_INDEX_SCOPED(plSceneNode));
     if (keyList)
     {
-        if (keyList->fStaticKeys.size() == 1)
+        if (keyList->fKeys.size() == 1)
         {
-            return plKey::Make((plKeyData*)keyList->fStaticKeys[0]);
-        }
-        else if (keyList->fDynamicKeys.size() == 1) // happens during export
-        {
-            plRegistryKeyList::DynSet::const_iterator it = keyList->fDynamicKeys.begin();
-            plKeyImp* keyImp = *it;
-            return plKey::Make(keyImp);
+            return plKey::Make((plKeyData*)keyList->fKeys[0]);
         }
     }
 
@@ -459,9 +453,9 @@ plKey plKeyFinder::IFindSceneNodeKey(plRegistryPageNode* page) const
     // Get the list of all sceneNodes
     plKey retVal(nil);
     keyList = page->IGetKeyList(CLASS_INDEX_SCOPED(plSceneNode));
-    if (keyList && keyList->fStaticKeys.size() == 1)
+    if (keyList && keyList->fKeys.size() == 1)
     {
-        retVal = plKey::Make((plKeyData*)keyList->fStaticKeys[0]);
+        retVal = plKey::Make((plKeyData*)keyList->fKeys[0]);
     }
     // If we just loaded up all the keys for this page, then we
     // may have a bunch of keys with a refcount of 0. For any of 

Sources/Plasma/PubUtilLib/plResMgr/plRegistryKeyList.cpp

@@ -39,83 +39,44 @@ You can contact Cyan Worlds, Inc. by email legal@cyan.com
       Mead, WA   99021
 
 *==LICENSE==*/
-#include "plRegistryKeyList.h"
-#include "plRegistryHelpers.h"
-#include "hsStream.h"
+
 #include <algorithm>
 
-plRegistryKeyList::plRegistryKeyList(uint16_t classType)
-{
-    fClassType = classType;
-    fReffedStaticKeys = 0;
-    fLocked = 0;
-    fFlags = 0;
-}
+#include "HeadSpin.h"
+#include "hsStream.h"
+
+#include "pnKeyedObject/plKeyImp.h"
+#include "plRegistryHelpers.h"
+#include "plRegistryKeyList.h"
 
 plRegistryKeyList::~plRegistryKeyList()
 {
-    hsAssert(fLocked == 0, "Key list still locked on delete");
-
-    for (int i = 0; i < fStaticKeys.size(); i++)
-    {
-        plKeyImp* keyImp = fStaticKeys[i];
-        if (!keyImp->ObjectIsLoaded())
-            delete keyImp;
-    }
+    std::for_each(fKeys.begin(), fKeys.end(),
+        [] (plKeyImp* key) { if (!key->ObjectIsLoaded()) delete key; }
+    );
 }
 
-// Special dummy key that lets us set the return value of the GetName call.
-// Makes it easier to do STL searches.
-class plSearchKeyImp : public plKeyImp
+plKeyImp* plRegistryKeyList::FindKey(const plString& keyName) const
 {
-public:
-    plString fSearchKeyName;
-    const plString& GetName() const { return fSearchKeyName; }
-};
-
-plKeyImp* plRegistryKeyList::FindKey(const plString& keyName)
-{
-    static plSearchKeyImp searchKey;
-    searchKey.fSearchKeyName = keyName;
-
-    // Search the static key list
-    if (fFlags & kStaticUnsorted)
-    {
-        // We're unsorted, brute force it.  May do a separate search table in the
-        // future if this is a bottlneck
-        for (int i = 0; i < fStaticKeys.size(); i++)
-        {
-            plKeyImp* curKey = fStaticKeys[i];
-            if (curKey && !keyName.Compare(curKey->GetName(), plString::kCaseInsensitive))
-                return curKey;
-        }
-    }
+    auto it = std::find_if(fKeys.begin(), fKeys.end(),
+        [&] (plKeyImp* key) { return key->GetName().CompareI(keyName) == 0; }
+    );
+    if (it != fKeys.end())
+        return *it;
     else
-    {
-        // We're sorted, do a fast lookup
-        StaticVec::const_iterator it = std::lower_bound(fStaticKeys.begin(), fStaticKeys.end(), &searchKey, KeySorter());
-        if (it != fStaticKeys.end() && !keyName.Compare((*it)->GetName(), plString::kCaseInsensitive))
-            return *it;
-    }
-
-    // Search the dynamic key list
-    DynSet::const_iterator dynIt = fDynamicKeys.find(&searchKey);
-    if (dynIt != fDynamicKeys.end())
-        return *dynIt;
-
-    return nil;
+        return nullptr;
 }
 
-plKeyImp* plRegistryKeyList::FindKey(const plUoid& uoid)
+plKeyImp* plRegistryKeyList::FindKey(const plUoid& uoid) const
 {
     uint32_t objectID = uoid.GetObjectID();
 
-    // Key is dynamic or doesn't know it's index.  Do a find by name.
+    // Key is dynamic or doesn't know its index.  Do a find by name.
     if (objectID == 0)
         return FindKey(uoid.GetObjectName());
 
     // Direct lookup
-    if (objectID <= fStaticKeys.size())
+    if (objectID <= fKeys.size())
     {
 #ifdef PLASMA_EXTERNAL_RELEASE
         return fStaticKeys[objectID-1];
@@ -123,8 +84,8 @@ plKeyImp* plRegistryKeyList::FindKey(const plUoid& uoid)
         // If this is an internal release, our objectIDs might not match
         // because of local data. Verify that we have the right key by
         // name, and if it's wrong, do the slower find-by-name.
-        plKeyImp *keyImp = fStaticKeys[objectID-1];
-        if (keyImp->GetName().Compare(uoid.GetObjectName(), plString::kCaseInsensitive) != 0)
+        plKeyImp *keyImp = fKeys[objectID-1];
+        if (keyImp->GetName().CompareI(uoid.GetObjectName()) != 0)
             return FindKey(uoid.GetObjectName());
         else
             return keyImp;
@@ -135,29 +96,17 @@ plKeyImp* plRegistryKeyList::FindKey(const plUoid& uoid)
     // because no one was using them. No worries. The resManager will catch this and 
     // reload our keys, then try again.
 
-    return nil;
-}
-
-void plRegistryKeyList::ILock()
-{
-    fLocked++;
-}
-
-void plRegistryKeyList::IUnlock()
-{
-    fLocked--;
-    if (fLocked == 0)
-        IRepack();
+    return nullptr;
 }
 
 bool plRegistryKeyList::IterateKeys(plRegistryKeyIterator* iterator)
 {
     ILock();
 
-    for (int i = 0; i < fStaticKeys.size(); i++)
+    for (auto it = fKeys.begin(); it != fKeys.end(); ++it)
     {
-        plKeyImp* keyImp = fStaticKeys[i];
-        if (keyImp != nil)
+        plKeyImp* keyImp = *it;
+        if (keyImp)
         {
             if (!iterator->EatKey(plKey::Make(keyImp)))
             {
@@ -167,18 +116,6 @@ bool plRegistryKeyList::IterateKeys(plRegistryKeyIterator* iterator)
         }
     }
 
-    DynSet::const_iterator it;
-    for (it = fDynamicKeys.begin(); it != fDynamicKeys.end(); it++)
-    {
-        plKeyImp* keyImp = *it;
-        hsAssert(keyImp, "Shouldn't ever have a nil dynamic key");
-        if (!iterator->EatKey(plKey::Make(keyImp)))
-        {
-            IUnlock();
-            return false;
-        }
-    }
-
     IUnlock();
     return true;
 }
@@ -188,23 +125,30 @@ void plRegistryKeyList::AddKey(plKeyImp* key, LoadStatus& loadStatusChange)
     loadStatusChange = kNoChange;
 
     hsAssert(fLocked == 0, "Don't currently support adding keys while locked");
-    if (fLocked == 0 && key != nil)
+    if (fLocked == 0 && key)
     {
-        // If this is the first key added, we just became loaded
-        if (fDynamicKeys.empty())
-            loadStatusChange = kDynLoaded;
+        hsAssert(std::find(fKeys.begin(), fKeys.end(), key) == fKeys.end(), "Key already added");
 
-        hsAssert(fDynamicKeys.find(key) == fDynamicKeys.end(), "Key already added");
-        fDynamicKeys.insert(key);
-    }
-}
+        // first key to be added?
+        if (fKeys.empty())
+            loadStatusChange = kTypeLoaded;
 
-void plRegistryKeyList::SetKeyUsed(plKeyImp* key)
-{
-    // If this is a static key, mark that we used it.  Otherwise, just ignore it.
-    uint32_t id = key->GetUoid().GetObjectID();
-    if (id > 0)
-        fReffedStaticKeys++;
+        // Objects that already have an object ID will be respected.
+        // Totally new keys will not have one, but keys from other sources (patches) will.
+        if (key->GetUoid().GetObjectID() == 0)
+        {
+            fKeys.push_back(key);
+            key->SetObjectID(fKeys.size());
+        }
+        else
+        {
+            uint32_t id = key->GetUoid().GetObjectID();
+            if (fKeys.size() < id)
+                fKeys.resize(id);
+            fKeys[id - 1] = key;
+        }
+        ++fReffedKeys;
+    }
 }
 
 bool plRegistryKeyList::SetKeyUnused(plKeyImp* key, LoadStatus& loadStatusChange)
@@ -219,122 +163,50 @@ bool plRegistryKeyList::SetKeyUnused(plKeyImp* key, LoadStatus& loadStatusChange
         return true;
     }
 
-    // Check if it's a static key
     uint32_t id = key->GetUoid().GetObjectID();
-    hsAssert(id <= fStaticKeys.size(), "Bad static key id");
-    if (id != 0 && id <= fStaticKeys.size())
-    {
-        fReffedStaticKeys--;
-        if (fLocked == 0)
-            IRepack();
-
-        // That was our last used static key, we're static unloaded
-        if (fReffedStaticKeys == 0)
-            loadStatusChange = kStaticUnloaded;
-
-        return true;
-    }
+    hsAssert(id <= fKeys.size(), "Bad static key id");
 
-    // Try to find it in the dynamic key list
-    DynSet::iterator dynIt = fDynamicKeys.find(key);
-    if (dynIt != fDynamicKeys.end())
+    // Fixed Keys will have id == 0. Let's just make sure we have it before we toss it.
+    if (id == 0)
     {
-        hsAssert(fLocked == 0, "Don't currently support removing dynamic keys while locked");
-        if (fLocked == 0)
+        hsAssert(key->GetUoid().GetLocation() == plLocation::kGlobalFixedLoc, "key id == 0 but not fixed?");
+        if (!FindKey(key->GetName()))
         {
-            fDynamicKeys.erase(dynIt);
-            delete key;
-
-            // That was our last dynamic key, notify of dynamic unloaded
-            if (fDynamicKeys.empty())
-                loadStatusChange = kDynUnloaded;
-
-            return true;
+            hsAssert(false, "Couldn't find fixed key!");
+            return false;
         }
-        return false;
-    }
-
-    hsAssert(0, "Couldn't find this key, what is it?");
-    return false;
-}
-
-//// IRepack /////////////////////////////////////////////////////////////////
-//  Frees the memory for our static key array if none of them are loaded
-void plRegistryKeyList::IRepack()
-{
-    if (fReffedStaticKeys == 0 && !fStaticKeys.empty())
-    {
-
-        for (int i = 0; i < fStaticKeys.size(); i++)
-            delete fStaticKeys[i];
-
-        fStaticKeys.clear();
-    }
-}
-
-void plRegistryKeyList::PrepForWrite()
-{
-    // If we have any static keys already, we were read in.  To keep from
-    // invalidating old key indexes any new keys have to go on the end, hence we're
-    // unsorted now.
-    if (!fStaticKeys.empty())
-        fFlags |= kStaticUnsorted;
-
-    // If a dynamic keys doesn't have an object assigned to it, we're not writing
-    // it out.  Figure out how many valid keys we have.
-    int numDynKeys = 0;
-    DynSet::const_iterator cIt;
-    for (cIt = fDynamicKeys.begin(); cIt != fDynamicKeys.end(); cIt++)
-    {
-        plKeyImp* key = *cIt;
-        // We're only going to write out keys that have objects
-        if (key->ObjectIsLoaded())
-            numDynKeys++;
     }
+    else if (id > fKeys.size())
+        return false;
 
-    // Start our new object id's after any already created ones
-    uint32_t objectID = fStaticKeys.size()+1;
-    // Make room for our new keys
-    fStaticKeys.resize(fStaticKeys.size()+numDynKeys);
-
-    DynSet::iterator it = fDynamicKeys.begin();
-    while (it != fDynamicKeys.end())
-    {
-        plKeyImp* key = *it;
-        it++;
-
-        // If we're gonna use this key, tag it with it's object id and move it to the static array.
-        if (key->ObjectIsLoaded())
-        {
-            key->SetObjectID(objectID);
-            fStaticKeys[objectID-1] = key;
-
-            objectID++;
-            fReffedStaticKeys++;
-            fDynamicKeys.erase(key);
-        }
-    }
+    // Got that key, decrement the key counter
+    --fReffedKeys;
+    if (fReffedKeys == 0)
+        loadStatusChange = kTypeUnloaded;
+    return true;
 }
 
 void plRegistryKeyList::Read(hsStream* s)
 {
     uint32_t keyListLen = s->ReadLE32();
-    if (!fStaticKeys.empty())
+    if (!fKeys.empty())
     {
         s->Skip(keyListLen);
         return;
     }
 
-    fFlags = s->ReadByte();
+    // deprecated flags. used to indicate alphabetically sorted keys for some "optimization"
+    // that really appeared to do nothing. no loss.
+    s->ReadByte();
 
     uint32_t numKeys = s->ReadLE32();
-    fStaticKeys.resize(numKeys);
+    fKeys.reserve(numKeys);
 
-    for (int i = 0; i < numKeys; i++)
+    for (uint32_t i = 0; i < numKeys; ++i)
     {
         plKeyImp* newKey = new plKeyImp;
         newKey->Read(s);
-        fStaticKeys[i] = newKey;
+        fKeys.push_back(newKey);
     }
 }
 
@@ -343,17 +215,13 @@ void plRegistryKeyList::Write(hsStream* s)
     // Save space for the length of our data
     uint32_t beginPos = s->GetPosition();
     s->WriteLE32(0);
-    s->WriteByte(fFlags);
+    s->WriteByte(0); // Deprecated flags
 
-    int numKeys = fStaticKeys.size();
-    s->WriteLE32(numKeys);
+    s->WriteLE32(fKeys.size());
 
-    // Write out all our keys (anything in dynamic is unused, so just ignore those)
-    for (int i = 0; i < numKeys; i++)
-    {
-        plKeyImp* key = fStaticKeys[i];
-        key->Write(s);
-    }
+    // Write out all our keys
+    for (auto it = fKeys.begin(); it != fKeys.end(); ++it)
+        (*it)->Write(s);
 
     // Go back to the start and write the length of our data
     uint32_t endPos = s->GetPosition();

Sources/Plasma/PubUtilLib/plResMgr/plRegistryKeyList.h

@@ -42,22 +42,13 @@ You can contact Cyan Worlds, Inc. by email legal@cyan.com
 #ifndef plRegistryKeyList_h_inc
 #define plRegistryKeyList_h_inc
 
-#include "HeadSpin.h"
-#include "pnKeyedObject/plKeyImp.h"
 #include <vector>
-#include <set>
 
+class plKeyImp;
 class plRegistryKeyIterator;
-
-class KeySorter
-{
-public:
-    bool operator() (plKeyImp* k1, plKeyImp* k2) const
-    {
-        hsAssert(k1 && k2, "Should have valid keys here");
-        return k1->GetName().Compare(k2->GetName(), plString::kCaseInsensitive) < 0;
-    }
-};
+class hsStream;
+class plString;
+class plUoid;
 
 //
 //  List of keys for a single class type.
@@ -68,60 +59,41 @@ protected:
     friend class plKeyFinder;
 
     uint16_t fClassType;
-    // Lock counter for iterating. If this is >0, don't do any ops that
-    // can change key positions in the array (instead, just leave holes)
-    uint16_t fLocked;
+    uint32_t fLocked;
+    uint32_t fReffedKeys;
 
-    enum Flags { kStaticUnsorted = 0x1 };
-    uint8_t fFlags;
-
-    // Static keys are one's we read off disk.  These don't change and are
-    // assumed to be already sorted when they're read in.
-    typedef std::vector<plKeyImp*> StaticVec;
-    StaticVec fStaticKeys;
-    uint32_t fReffedStaticKeys;   // Number of static keys that are loaded
-
-    // Dynamic keys are anything created at runtime.  They are put in the
-    // correct sorted position when they are added
-    typedef std::set<plKeyImp*, KeySorter> DynSet;
-    DynSet fDynamicKeys;
+    std::vector<plKeyImp*> fKeys;
 
     plRegistryKeyList() {}
 
-    void ILock();
-    void IUnlock();
-
     void IRepack();
+    void ILock() { ++fLocked; }
+    void IUnlock() { --fLocked; }
 
 public:
-    plRegistryKeyList(uint16_t classType);
+    enum LoadStatus
+    {
+        kNoChange,
+        kTypeLoaded,
+        kTypeUnloaded
+    };
+
+    plRegistryKeyList(uint16_t classType)
+        : fClassType(classType), fReffedKeys(0), fLocked(0)
+    { }
     ~plRegistryKeyList();
 
     uint16_t GetClassType() const { return fClassType; }
 
-    // Find a key by name (case-insensitive)
-    plKeyImp* FindKey(const plString& keyName);
-    // Find a key by uoid index.
-    plKeyImp* FindKey(const plUoid& uoid);
+    plKeyImp* FindKey(const plString& keyName) const;
+    plKeyImp* FindKey(const plUoid& uoid) const;
 
     bool IterateKeys(plRegistryKeyIterator* iterator);
 
-    // Changes in our load status that can be caused by loading or unloading a key
-    enum LoadStatus
-    {
-        kNoChange,
-        kDynLoaded,
-        kDynUnloaded,
-        kStaticUnloaded,
-    };
     void AddKey(plKeyImp* key, LoadStatus& loadStatusChange);
-    void SetKeyUsed(plKeyImp* key);
+    void SetKeyUsed(plKeyImp* key) { ++fReffedKeys; }
     bool SetKeyUnused(plKeyImp* key, LoadStatus& loadStatusChange);
 
-    // Export time only.  Before we write to disk, assign all the static keys
-    // object ID's that they can use to do fast lookups at load time.
-    void PrepForWrite();
-
     void Read(hsStream* s);
     void Write(hsStream* s);
 };

Sources/Plasma/PubUtilLib/plResMgr/plRegistryNode.cpp

@@ -55,8 +55,7 @@ You can contact Cyan Worlds, Inc. by email legal@cyan.com
 plRegistryPageNode::plRegistryPageNode(const plFileName& path)
     : fValid(kPageCorrupt)
     , fPath(path)
-    , fDynLoadedTypes(0)
-    , fStaticLoadedTypes(0)
+    , fLoadedTypes(0)
     , fOpenRequests(0)
     , fIsNewPage(false)
 {
@@ -73,8 +72,7 @@ plRegistryPageNode::plRegistryPageNode(const plLocation& location, const plStrin
                                        const plString& page, const plFileName& dataPath)
     : fValid(kPageOk)
     , fPageInfo(location)
-    , fDynLoadedTypes(0)
-    , fStaticLoadedTypes(0)
+    , fLoadedTypes(0)
     , fOpenRequests(0)
     , fIsNewPage(true)
 {
@@ -182,7 +180,7 @@ void plRegistryPageNode::LoadKeys()
 
     stream->SetPosition(oldPos);
     CloseStream();
-    fStaticLoadedTypes = fKeyLists.size();
+    fLoadedTypes = fKeyLists.size();
 }
 
 void plRegistryPageNode::UnloadKeys()
@@ -195,8 +193,7 @@ void plRegistryPageNode::UnloadKeys()
     }
     fKeyLists.clear();
 
-    fDynLoadedTypes = 0;
-    fStaticLoadedTypes = 0;
+    fLoadedTypes = 0;
 }
 
 //// plWriteIterator /////////////////////////////////////////////////////////
@@ -235,8 +232,6 @@ void plRegistryPageNode::Write()
     for (it = fKeyLists.begin(); it != fKeyLists.end(); it++)
     {
         plRegistryKeyList* keyList = it->second;
-        keyList->PrepForWrite();
-
         int ver = plVersion::GetCreatableVersion(keyList->GetClassType());
         fPageInfo.AddClassVersion(keyList->GetClassType(), ver);
     }
@@ -358,8 +353,8 @@ void plRegistryPageNode::AddKey(plKeyImp* key)
     plRegistryKeyList::LoadStatus loadStatusChange;
     keys->AddKey(key, loadStatusChange);
 
-    if (loadStatusChange == plRegistryKeyList::kDynLoaded)
-        fDynLoadedTypes++;
+    if (loadStatusChange == plRegistryKeyList::kTypeLoaded)
+        ++fLoadedTypes;
 }
 
 void plRegistryPageNode::SetKeyUsed(plKeyImp* key)
@@ -381,10 +376,8 @@ bool plRegistryPageNode::SetKeyUnused(plKeyImp* key)
     bool removed = keys->SetKeyUnused(key, loadStatusChange);
 
     // If the key type just changed load status, update our load counts
-    if (loadStatusChange == plRegistryKeyList::kDynUnloaded)
-        fDynLoadedTypes--;
-    else if (loadStatusChange == plRegistryKeyList::kStaticUnloaded)
-        fStaticLoadedTypes--;
+    if (loadStatusChange == plRegistryKeyList::kTypeUnloaded)
+        --fLoadedTypes;
 
     return removed;
 }

Sources/Plasma/PubUtilLib/plResMgr/plRegistryNode.h

@@ -74,8 +74,7 @@ protected:
     // Map from class type to a list of keys of that type
     typedef std::map<uint16_t, plRegistryKeyList*> KeyMap;
     KeyMap fKeyLists;
-    int fDynLoadedTypes;    // The number of key types that have dynamic keys loaded
-    int fStaticLoadedTypes; // The number of key types that have all their keys loaded
+    uint32_t fLoadedTypes;      // The number of key types that have dynamic keys loaded
 
     PageCond    fValid;         // Condition of the page
     plFileName  fPath;          // Path to the page file
@@ -104,9 +103,9 @@ public:
     PageCond GetPageCondition() { return fValid; }
 
     // True if we have any static or dynamic keys loaded
-    bool IsLoaded() const     { return fDynLoadedTypes > 0 || fStaticLoadedTypes > 0; }
+    bool IsLoaded() const     { return fLoadedTypes > 0; }
     // True if all of our static keys are loaded
-    bool IsFullyLoaded() const    { return (fStaticLoadedTypes == fKeyLists.size() && !fKeyLists.empty()) || fIsNewPage; }
+    bool IsFullyLoaded() const    { return (fLoadedTypes == fKeyLists.size() && !fKeyLists.empty()) || fIsNewPage; }
 
     // Export time only.  If we want to reuse a page, load the keys we want then
     // call SetNewPage, so it will be considered a new page from now on.  That