Ocena kodu - zarządca zasobów

Odpowiedz Nowy wątek
2014-12-26 23:02
0

Cóż to wiele mówić - próbuję się późną godziną wgryźć w tworzenie klasy, która będzie threadsafe.

#include <functional>
#include <vector>
#include <memory>
#include <utility>
#include <mutex>
 
template<typename T>
class ResourceManager{
public:
    using ResourcePtr = std::unique_ptr<T>;
    using SafeResource = std::pair<ResourcePtr, std::mutex>;
    using Key = size_t;
    using Actions = struct{
        std::function<T*(const std::string &)> load;
        std::function<void(T *)> unload;
    };
public:
    ResourceManager(Actions actions):
        _actions(actions){}
    ResourceManager(const ResourceManager &) = delete;
public:
    Key load(const std::string &path){
        auto resource = _actions.load(path);
        return load(resource);
    }
 
    Key load(T *resource){
        std::lock_guard<std::mutex> guard(_mutex);
        auto resourcePtr = ResourcePtr(resource, _actions.unload);
        _resources.push_back(make_pair(resourcePtr, std::mutex()));
        return _resources.size()-1;
    }
 
    void unload(Key key){
        std::lock_guard<std::mutex> guard(_mutex);
        _resources.erase(_resources.begin()+key);
    }
 
    void operate(Key key, std::function<void(T &)> operation){
        auto safeResource = _resources.at(key);
        std::lock_guard<std::mutex> guard(safeResource.second);
        operation(*safeResource.first);
    }
 
    const T *get(Key key) const{
        auto safeResource = _resources.at(key);
        std::lock_guard<std::mutex> guard(safeResource.second);
        return safeResource.first;
    }
private:
    Actions _actions;
    std::vector<SafeResource> _resources;
    std::mutex _mutex;
};

Trochę nie wypada... ale co z tego, pff.

edytowany 3x, ostatnio: spartanPAGE, 2014-12-26 23:54
Ale czego nie wypada? Dobrze jet od czasu do czasu poczytać/podyskutować o kodzie gdzie problemem nie jest źle napisana pętla. - several 2014-12-27 12:21

Pozostało 580 znaków

2014-12-26 23:46
1

Wg mnie to w get nie powinieneś bezpośrednio lock odpalać.
Natomiast na poczet tego get trzeba stworzyć metodę zwracającą lock_guard poprzez &&


Wykonuję programy na zamówienie, pisać na Priv.
Asm/C/C++/Pascal/Delphi/Java/C#/PHP/JS oraz inne języki.
O właśnie! Właśnie tego działania oczekiwałem, ale nie potrafiłem zebrać tego w głowie - spartanPAGE 2014-12-26 23:48

Pozostało 580 znaków

2014-12-27 01:05
Wybitny Terrorysta
0
_13th_Dragon napisał(a):

Wg mnie to w get nie powinieneś bezpośrednio lock odpalać.
Natomiast na poczet tego get trzeba stworzyć metodę zwracającą lock_guard poprzez &&

Proszę o wybaczenie, jeśli pytanie jest nie na miejscu, ale dlaczego napisałeś &&? Co to znaczy? Zwracanie przez referencję wymaga jednego & - i zastanawiam się o co chodzi z podwójnym znakiem &?

2014-12-27 01:14
Krwawy Krawiec
0

To się nazywa rvalue reference i wprowadzone zostało w C++11. ;)

Pozostało 580 znaków

2014-12-27 01:25
0

Tak dla przyszłych pokoleń lub do dalszej oceny:
Po podliczeniu plusów i minusów oraz naprawieniu kilku spraw (m.i. nonmovable mutexes)... wychodzi taki kod:

#include <functional>
#include <vector>
#include <memory>
#include <mutex>
 
template<typename T>
class ResourceManager{
public:
    using Actions = struct{
        using LoadFunc = std::function<T*(const std::string &)>;
        using UnloadFunc = std::function<void(T *)>;
        LoadFunc load;
        UnloadFunc unload;
    };
    using ResourcePtr = std::unique_ptr<T, typename Actions::UnloadFunc>;
    struct MutexWrapper{
        std::mutex m;
        MutexWrapper() {}
        MutexWrapper(MutexWrapper const&) {}
        MutexWrapper& operator=(MutexWrapper const&) { return *this; }
        inline std::mutex &get(){ return m; }
    };
    using Key = size_t;
public:
    ResourceManager(Actions actions):
        _actions(actions){}
    ResourceManager(const ResourceManager &rhs) = delete;
public:
    Key load(const std::string &path){
        auto resource = _actions.load(path);
        return load(resource);
    }
 
    Key load(T *resource){
        std::lock_guard<std::mutex> guard(_mutex);
        auto resourcePtr = ResourcePtr(resource, _actions.unload);
        _resources.push_back(std::move(resourcePtr));
        _mutexes.push_back(MutexWrapper());
        return _resources.size()-1;
    }
 
    void unload(Key key){
        std::lock_guard<std::mutex> guard(_mutex);
        _resources.erase(_resources.begin()+key);
        _mutexes.erase(_mutexes.begin()+key);
    }
 
    void operate(Key key, std::function<void(T&)> operation){
        std::lock_guard<std::mutex> guard(_mutexes.at(key).get());
        operation(*_resources.at(key));
    }
 
    void operate(Key key, std::function<void(const T&)> operation) const{
        std::lock_guard<std::mutex> guard(_mutexes.at(key).get());
        operation(*_resources.at(key));
    }
private:
    Actions _actions;
    std::vector<ResourcePtr> _resources;
    std::vector<MutexWrapper> _mutexes;
    std::mutex _mutex;
};

Postanowiłem zrezygnować z prostego get, ponieważ przy uwzględnieniu opieki nad obiektem całość zaczyna być nie tak ładna jak oryginalnie miała być.
Testy w drodze

edytowany 4x, ostatnio: spartanPAGE, 2014-12-27 01:31

Pozostało 580 znaków

2014-12-27 01:39

To jeszcze trochę dowalę:

  1. Po kiego ci - void operate(Key key, std::function<void(T&)> operation){- skoro ta z const robi dokładnie to samo? No chyba że ta z const będzie operować na const T &.
  2. Dwa vector'y, doprawdy? A wydawało mi się że o strukturach słyszałeś ;P przynajmniej pair<> użyj o ile nie chcesz użyć jakieś dedykowanej.
  3. Czemu nie masz destruktora który pilnuje wszystkich mutex'ów ?
  4. Przypomnij sobie zadanie o Pisarzach i Czytelnikach - tak jak zrobiłeś czytelnik może być tylko jeden - źle.
  5. Zastanów się nad zastąpieniem operate zwróceniem pewnego obiektu dla którego przeciążono operator-> zwracający T* (ewentualnie shared_ptr<t>) istnienie obiektu trzyma czytelnika.
    Jak na razie wystarczy ;P

Wykonuję programy na zamówienie, pisać na Priv.
Asm/C/C++/Pascal/Delphi/Java/C#/PHP/JS oraz inne języki.
1. Jest jest, poprawiłem; 2. Fakt, moja nieuwaga, "bawiłem się" troszkę - namieszała mi nieprzesuwalność obiektów, tworząc "wsteczną refraktoryzację"; 3. Przymykam je tylko na czas konkretnych operacji, muszę się nimi dodatkowo zająć w destruktorze? 4. Bardzo słuszna uwaga, nie pomyślałem o tym (w moim kodzie dla danego zasobu może wykonywać się w danym czasie tylko jedna operacja - nie ważne czy to pisanie czy czytanie); ale noc jeszcze młoda - spartanPAGE 2014-12-27 01:49
Nie ma co usprawiedliwiać się, przecież zaraz będzie poprawiona wersja, czyż nie? - _13th_Dragon 2014-12-27 01:54

Pozostało 580 znaków

2014-12-27 01:47
Mały Szczur
0

Super, dzięki! Dopiero zaczynam zabawę z wielowątkowością, więc na pewno się przyda ;-)

Pozostało 580 znaków

2014-12-27 03:09
0

Ponieważ standard C++14 nie jest jeszcze tak szeroko używany, a musiałem dorwać coś na wzór shared_mutex postanowiłem użyć boosta; Jeśli chodzi o zastąpienie operate - wydaje mi się, że wszystko mogę osiągnąć używając lambd

#pragma once
 
#include <boost/thread/locks.hpp>
#include <boost/thread/shared_mutex.hpp>
#include <boost/thread/mutex.hpp>
 
#include <functional>
#include <vector>
#include <memory>
#include <utility>
 
template<typename T>
class ResourceManager{
public:
    using Actions = struct{
        using LoadFunc = std::function<T*(const std::string &)>;
        using UnloadFunc = std::function<void(T *)>;
        LoadFunc load;
        UnloadFunc unload;
    };
    struct MutexWrapper{
        using InternalType = boost::shared_mutex;
        InternalType m;
        MutexWrapper(){}
        MutexWrapper(const MutexWrapper &){}
        MutexWrapper& operator=(const MutexWrapper &){ return *this; }
        inline InternalType &get(){ return m; }
    };
    using ResourcePtr = std::unique_ptr<T, typename Actions::UnloadFunc>;
    using SafeResource = std::pair<ResourcePtr, MutexWrapper>;
    using Key = size_t;
public:
    ResourceManager(Actions actions):
        _actions(actions){}
    ResourceManager(const ResourceManager &) = delete;
public:
    Key load(const std::string &path){
        auto resource = _actions.load(path);
        return load(resource);
    }
 
    Key load(T *resource){
        boost::lock_guard<boost::mutex> guard(_mutex);
        auto resourcePtr = ResourcePtr(resource, _actions.unload);
        _resources.push_back(std::make_pair(std::move(resourcePtr), MutexWrapper()));
        return _resources.size()-1;
    }
 
    void unload(Key key){
        boost::lock_guard<boost::mutex> guard(_mutex);
        _resources.erase(_resources.begin() + key);
    }
 
    void operate(Key key, std::function<void(T&)> operation){
        auto &safeResource = _resources.at(key);
        boost::unique_lock<boost::shared_mutex> guard(safeResource.second.get());
        operation(*safeResource.first);
    }
 
    void operate(Key key, std::function<void(const T&)> operation) const{
        auto &safeResource = _resources.at(key);
        boost::shared_lock<boost::shared_mutex> guard(safeResource.second.get());
        operation(*safeResource.first);
    }
private:
    Actions _actions;
    std::vector<SafeResource> _resources;
    boost::mutex _mutex;
};

Pozostało 580 znaków

2014-12-27 03:31
1
  1. Wciąż nie widzę destruktora ;P
  2. operator= też wypada skasować
  3. Tak się zastanawiam może (schematycznie): template<typename T, typename Key=size_t> i użyć jednak mapy?

Wykonuję programy na zamówienie, pisać na Priv.
Asm/C/C++/Pascal/Delphi/Java/C#/PHP/JS oraz inne języki.

Pozostało 580 znaków

2014-12-27 03:52
0
  1. Wciąż nie widzę destruktora ;P

Nie koniecznie rozumiem co powinienem tam wsadzić - wszystko co dynamiczne jest wpakowane w unique_ptr

  1. operator= też wypada skasować

Z drugiej strony po usunięciu tego przy próbie kopiowania będzie krzyczeć unique_ptr, który się nie pozwoli się skopiować

  1. Tak się zastanawiam może (schematycznie): template<typename T, typename Key=size_t> i użyć jednak mapy?

Robię to do swojego małego projekciku, gdzie niezwykle ważny jest czas dostępu do elementów przy ich dużej ilości - O(1) to dosłowne błogosławieństwo.

Jak to mniej-więcej rozwijało się na papierze:

at. 1: ImageManager, push(string path), get(string path)
 
at. 2: class ImageManager, push(string name, string path), get(string name)
 
at. 3: class ImageManager, push(string path), get(string path_wo_extension)
 
at. 4: template<typename Key> class ImageManager, push(Key key, string path), get(key)
+usage: ImageManager<size_t>, push("something"_hash, "something.png"), get("something"_hash)
 
at. 5: template<typename T, typename Key> class ResourceManager, [...]
 
at. 6: template<typename T, typename Key> class AutoIncrementResourceManager, Key push(string path), get(Key)
 
at. 7: template<typename T>class ResourceManager, [...]

Ostatecznie wyszłoby, że wersja z mapą kurzyłaby się gdzieś w odmentach :P

czas chyba iść spać

edytowany 8x, ostatnio: spartanPAGE, 2014-12-27 03:55

Pozostało 580 znaków

2014-12-27 10:49
1

Nadal mi coś tu śmierdzi.
Chodzi o to że auto &safeResource = _resources.at(key); jest poza zasięgiem boost::mutex _mutex;
A to oznacza że może się okazać że pobierze jakieś śmiecie o ile się trafi na jednoczesne wykonanie z _resources.push_back(...
Brak destruktora oznacza dokładnie to samo, czyli możesz odczytać w _resources.at(key) jakieś śmieci.


Wykonuję programy na zamówienie, pisać na Priv.
Asm/C/C++/Pascal/Delphi/Java/C#/PHP/JS oraz inne języki.

Pozostało 580 znaków

Odpowiedz
Liczba odpowiedzi na stronę

1 użytkowników online, w tym zalogowanych: 0, gości: 1, botów: 0