Critical Sections bei Zugriff auf CMapWordToOb nötig?

Knaecke

Es gab zwar schon so einen ähnlichen Beitrag heute (Multithreading bei Arrays), aber ich mach trotzdem mal ein eigenes Thema auf.

Zuerst mal ein Stück Code:

bool CMyClass::SaveData(long id, void* myStruct, int sizeMyStruct)
{	
    CSingleLock lock(&m_Section, false);
    CByteArray *pArchive; // CBA-Pointer für Speicherzugriff auf Element in Map

    ...

    lock.Lock();
    m_Map.SetAt(id, new CByteArray); // neues CBA in Map anlegen
    m_Map.Lookup(id, pArchive);      // neues CBA dem Pointer zuweisen
    pArchive->SetSize(sizeMyStruct); // Größe für neues CBA setzen
    if(!memcpy(pArchive->GetData(),myStruct,sizeMyStruct)) // Inhalt aus Struktur ins CBA kopieren
        return false; // falls etwas schief läuft 
    lock.Unlock();

    ...

    return true;
}

Macht es nun Sinn, Zeilen 11+12 aus Performancegründen vom Locking auszunehmen und die Map wieder freizugeben? Oder wär das keine gute Idee?

Hinweis: Es kann nicht passieren, dass genau dieser Eintrag überschrieben wird, denn jedes Objekt, das in die Map in MyClass (= Singleton) schreiben will, hat seinen eigenen Key für die Map, allerdings könnten andere Objekte zu dem Zeitpunkt an andere Stellen der Map schreiben.

Und inwiefern arbeitet die Map? Ich weiß ja nicht, ob dann vielleicht gerade durch neue Einträge möglicherweise intern gerade die Pointer und Speicherbereiche vom BS umgeschaufelt werden. Oder kann sowas nicht passieren? In der MSDN steht zumindest, dass CMap nicht threadsicher ist, daher würde ich wohl fürs Einschließen von Zeilen 11+12 ins Locking plädieren.

Wie seht ihr das?

Knaecke

Hat hier keiner eine Idee?

hustbaer

Auf keinen Fall freigeben. Du darfst nicht lesen während ein anderer Thread schreiben könnte, und du darfst nicht schreiben während ein anderer Thread lesen oder schreiben könnte. Daher darfst du die CS nicht aufmachen.

Davon abgesehen macht man das normal eher so:

bool CMyClass::SaveData(long id, void* myStruct, int sizeMyStruct)
{    
    std::auto_ptr<CByteArray> pArchive(new CByteArray);

    ...

    pArchive->SetSize(sizeMyStruct);
    memcpy(pArchive->GetData(),myStruct,sizeMyStruct);

    {
       CSingleLock lock(&m_Section, true);
       m_Map.SetAt(id, pArchive.release());
    }

    ...

    return true;
}

Knaecke

Danke für den Hinweis. Mit der Reihenfolge vom Setzen, Kopieren und dann einfügen hast du natürlich völlig recht. Ist viel sinnvoller. So konnte ich noch einigen Code aus den CS rausnehmen. Weiß gar nicht, wie ich auf diese Methode gekommen bin, die muss ich wohl aus irgendeinem Tutorial oder so haben...

Der auto_ptr geht aber komischerweise nicht (der erzählt mir, auto_ptr wäre kein Member vom std, kann aber eigentlich nicht sein *schulterzuck*). Aber den brauch ich doch auch nicht wirklich oder? Weil erstens brauch ich mein pArchive noch mehrmals in meiner Funktion und zweitens wird der eh zum Ende der Methode wieder gelöscht oder nicht?

akari

Hallo

Der auto_ptr geht aber komischerweise nicht (der erzählt mir, auto_ptr wäre kein Member vom std, kann aber eigentlich nicht sein *schulterzuck*).

#include <memory>

Aber den brauch ich doch auch nicht wirklich oder?

Doch, denn auto_ptr wäre Exception-Save, dein manuelles delete nicht.

bis bald
akari

hustbaer

Jo, in m_Map.SetAt() kann es nötig sein Speicher anzufordern, und das kann daneben gehen. Und dann fliegt ein bad_alloc. Und dann würde pArchive nichtmehr freigegeben. Du kannst es aber so schreiben:

bool CMyClass::SaveData(long id, void* myStruct, int sizeMyStruct)
{    
    std::auto_ptr<CByteArray> archiveGuard(new CByteArray);
    CByteArray* pArchive(archiveGuard.get());

    ...

    pArchive->SetSize(sizeMyStruct);
    memcpy(pArchive->GetData(),myStruct,sizeMyStruct);

    {
       CSingleLock lock(&m_Section, true);
       m_Map.SetAt(id, pArchive);
       archiveGuard.release(); // ownership wurde an m_Map übertragen
    }

    ...
    pArchive->... // geht hier dann auch noch

    return true;
}

Genauer genommen war die erste Variante sogar falsch -- wenn aus SetAt eine Exception fliegt wäre pArchive auch nichtmehr freigegeben worden, und wäre auch nicht in m_Map drinnen -> Leak
Sorry, mein Fehler!

Knaecke

Jo da hab ich mich auch ein wenig gewundert, ob das da nicht schon einen Tick zu zeitig wäre, den Pointer schon beim SetAt freizugeben... naja aber ein Kollege hat spaßeshalber mal am Wochenende die MFC-Map mit der der STL verglichen und bei seinem Test rausgefunden, dass die STL wohl etwa 4x schneller war. Da werd ich wohl eh nochmal einiges umbauen...

Baust du auf, reißt du nieder, hast du Arbeit immer wieder

Aber das wird wahrscheinlich nix am Problem ändern, werd mich dann auch mal um den auto_ptr kümmern

hustbaer

Du kannst dir auch boost::shared_ptr (kann & darf man in Container stecken) bzw. die Boost pointer-container angucken.
Die erleichtern die ganze Ownership Sache doch erheblich.