Ocena kodu - zarządca zasobów

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.

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 &&

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 &?

0

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

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

1

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
0

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

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;
};
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?
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ć

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.

0
void operate(Key key, std::function<void(T&)> operation){
	_mutex.lock();
    auto &safeResource = _resources.at(key);
    boost::unique_lock<boost::shared_mutex> guard(safeResource.second.get());
    _mutex.unlock();
    operation(*safeResource.first);
}

void operate(Key key, std::function<void(const T&)> operation) const{
	_mutex.lock();
    auto &safeResource = _resources.at(key);
    boost::shared_lock<boost::shared_mutex> guard(safeResource.second.get());
    _mutex.unlock();
    operation(*safeResource.first);
}

i z tego co zrozumiałem*

~ResourceManager(){ boost::lock_guard<boost::mutex> guard(_mutex); }

Czy aby na pewno nic nie rzuci tutaj wyjątku? Jeśli tak, będę musiał temu zaradzić.

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