Designfrage: Pool



  • Hm... okay, ich habe das jetzt so umgeändert:

    template <typename HTy_>
    class pool {
    public:
    	typedef typename std::list<HTy_>::iterator iterator;
    
    	template <typename Fnc_>
    	pool(unsigned poolsize, const Fnc_& fnc)
    	{
    		while(--poolsize) unused_.push(fnc());
    	}
    
    	const iterator get() {
    		mutex lock;
    		used_.push_front(unused_.front());
    		unused_.pop();
    		return used.begin();
    	}
    
    	void putback(iterator) {
    		mutex lock;
    		unused_.push(*iterator);
    		used_.erase(iterator);
    	}
    
    private:
    	std::queue<HTy_> unused_;
    	std::list<HTy_> used_;
    };
    

    Hoffentlich geht das gut... 😞



  • was soll "mutex lock;" sein?



  • Na so ein Lock-Teil, das ich anderswo definiert habe. Wäre aber Windows-spezifisch.



  • .. habe meine Antwort versucht klarer zu schreiben.. 🙂

    Edit:
    Andersrum: Du musst pro zu lockenden Bereich (also einen Bereich auf welchen nur 1 Thread gleichzeitig zugreifen kann) ein Mutex Objekt haben. Bedeutet, dein Objekt lock muss ein Member sein!

    Simon



  • Deshalb besitzt mutex ja im Innern ein statisches Objekt. 😕 Der Konstruktor is blockierend, fals dieses Objekt im "locked"-mode steht.



  • Aha.. aber Du willst ja nicht alle Pool Instanzen auch gleich mitlocken, wenn Du bei einer bestimmten Pool Instanz ein Objekt rausholst.

    Edit:
    Typeischerweise geht das in etwa so:
    1. Es gibt ein Mutex Objekt (CriticalSection, Mutex, etc.) pro zu schützenden "Bereich", in deinem Fall ein Objekt vom Typ pool.
    2. Jede Methode die den schützenswerten Bereich "anfassen" möchte, versucht dieses Mutex Objekt zu locken und wird es danach auch wieder freigeben. Das wird oft mittels RAII, z.B. einer Lock Klasse gemacht, welche als Konstruktor Argument eben dieses Mutex Objekt nimmt.



  • So habe ich es in etwa gemacht:

    template <typename HTy_>
    class pool {
    public:
        typedef typename std::list<HTy_>::iterator iterator;
    
        template <typename Fnc_>
        pool(unsigned poolsize, const Fnc_& fnc)
        {
            while(--poolsize) unused_.push(fnc());
        }
    
        const iterator get() {
            mutex lck(lock_);
            used_.push_front(unused_.front());
            unused_.pop();
            return used.begin();
        }
    
        void putback(iterator) {
            mutex lck(lock_);
            unused_.push(*iterator);
            used_.erase(iterator);
        }
    
    private:
        mutex lock_;
        std::queue<HTy_> unused_;
        std::list<HTy_> used_;
    };
    

    Jetzt in Ordnung?



  • Ohne genau zu wissen was dein "mutex" ist, kann das keiner sagen, aber das was hier nach nem Copyctor aussieht ist dann wohl falsch.



  • > Ohne genau zu wissen was dein "mutex" ist,

    Da nicht alle Leute Win32-API können, bringt es nichts, den Code zu posten.

    > aber das was hier nach nem Copyctor aussieht ist dann wohl falsch.

    Nö. Der Lock wird kopiert und blockiert dann, wenn gerade jemand anderes dort arbeitet. Wenn der andere fertig ist, hört der Copy-Ctor auf zu warten und blockiert nun selber und returned. Der Dtor von der Kopie hebt die Blockade dann wieder auf.



  • Ad aCTa schrieb:

    > Ohne genau zu wissen was dein "mutex" ist,

    Da nicht alle Leute Win32-API können, bringt es nichts, den Code zu posten.

    dann können wir auch nix machen



  • Mir egal, aber für dieses Forum ist das jetzt nicht treffend:

    class mutex {
    public:
    	mutex()
               : event_(CreateEvent(0, 0, 0, 0)) {
    		SetEvent(event_); // signaled
    	}
    
    	mutex(const mutex& m)
    		: event_(m.event_) {
    		WaitForSingleObject(event_, INFINITE);
    		ResetEvent(event_); // Nicht signaled
    	}
    
    	~mutex() { SetEvent(event_); } // signaled
    
    private:
    	HANDLE event_;
    };
    


  • Ad aCTa schrieb:

    Jetzt in Ordnung?

    Nein, du darfst keinen iterator auf den pool nach aussen geben.
    Davon abgesehen hast du hier ja keinen relevanten Code, also kann man nichts sagen. Aber der Ansatz sieht etwas schief aus. Vorallem der mutex Code und der Ctor.

    Weiters musst du mit Proxys der Resourcen arbeiten, du willst ja nicht dauernd kopieren - vorallem sind nicht alle Resourcen kopierbar...

    Die used Liste brauchst du im Prinzip auch nur wenn du garbage collection machen willst.

    Vielleicht waere etwas mehr Code schon sinnvoll, denn da ist nichts worueber wir sinnvoll reden koennten, mit Ausnahme ob man eine used-Liste braucht.



  • > Nein, du darfst keinen iterator auf den pool nach aussen geben.

    Warum?

    > Davon abgesehen hast du hier ja keinen relevanten Code, also kann man nichts sagen.

    Anwendungsfall wäre:

    typedef std::shared_ptr<socket> socket_ptr;
    pool<socket_ptr> socket_pool(5, create_socket); // create_socket ist eine Funktion
    // ...
    for(;;) {
       pool<socket_ptr>::iterator accept_socket = socket_pool.get(); // freien Socket bekommen
       accept(*accept_socket); // Verbindung aufnehmen
       service(*accpet_socket); // Verbindung bearbeiten bis zum disconnect
       socket_pool.putback(accept_socket); // Socket zurücklegen
    }
    

    Zugegeben, das Dereferenzieren sieht nicht schön aus, aber wo ist das Problem mit einem Iterator?

    > Weiters musst du mit Proxys der Resourcen arbeiten, du willst ja nicht dauernd kopieren

    Ich habe sowohl Wrapper um die Handles alsauch shared_ptr.



  • was soll dein mutex jetzt machen außer ewig warten im copyctor?
    schau dir mal das an
    http://msdn.microsoft.com/en-us/library/ms686927(VS.85).aspx
    und so geht ein scoped lock
    http://www.boost.org/doc/libs/1_35_0/doc/html/boost/interprocess/scoped_lock.html

    Ad aCTa schrieb:

    > Nein, du darfst keinen iterator auf den pool nach aussen geben.

    Warum?

    weil man damit alles möglich ein deinem pool machen kann, am mutex vorbei usw..



  • > was soll dein mutex jetzt machen außer ewig warten im copyctor?

    Das Event umschalten (non-signaled), damit ein weiterer Lock jetzt warten muss. Davor wartet er solange, bis ein anderer Thread das Event auf signaled stellt. (Genau dann wenn der Dtor eines anderen Locks aufgerufen wurde.) Was soll daran nicht stimmen?

    > weil man damit alles möglich ein deinem pool machen kann, am mutex vorbei usw..

    Was will man denn machen? Der Iterator ist konstant, der Pool kann durch ihn nicht verändert werden. Er hat nur Zugriff auf dieses eine Element, und das gehört jetzt dem Thread und muss bis zum putback nicht mehr synchronisiert werden.

    Edit: Ja, der Sinn der used-Liste ist schon etwas fraglich. Ich hatte Angst vor Leaks, weil ich dem Nutzer nun das Handle anvertraue und hoffe, dass er es auch wieder in den Pool zurückpackt. Ich könnte das natürlich so machen:

    struct noch_ein_wrapper {
       noch_ein_wrapper(const socket_ptr& ptr, pool<socket_ptr>& p)
          : socket_pool(p), socket(ptr) {
       }
    
       ~noch_ein_wrapper() { socket_pool.putback(ptr); }
    
       pool<socket_ptr>& socket_pool;
       socket_ptr socket;
    };
    

    Aber das wäre jetzt schon der 3. Wrapper um das eigentliche Handle...



  • 1. CloseHandle(..) wird nicht aufgerufen bei deiner mutex Implemenation.
    2. Benutzt doch die extra dafür vorgesehenen Mechanismen, entweder boost oder direkt Win32 API Critical Section, Mutex, Semaphore, ...

    Hier eine Implementation mit Critical Sections um das Prinzip zu Skizieren:

    class CriticalSection
    {
    public:
        CriticalSection()
        {
          InitializeCriticalSection(&_cs);
        }
        ~CriticalSection()
        {
          DeleteCriticalSection(&_cs);
        }
        void enter() const
        {
          EnterCriticalSection(&_cs);
        }
        void leave() const
        {
          LeaveCriticalSection(&_cs);
        }
    
    private:
        CriticalSection(const CriticalSection&);
        CriticalSection& operator=(const CriticalSection&);
    
        mutable CRITICAL_SECTION _cs;
    };
    
    class CriticalSectionLock
    {
    public:
        explicit CriticalSectionLock(const CriticalSection& cs)
        : _cs(cs)
        {
          _cs.enter();
        }
        ~CriticalSectionLock()
        {
          _cs.leave();
        }
    
    private:
        CriticalSectionLock(const CriticalSectionLock&);
        CriticalSectionLock& operator=(const CriticalSectionLock&);
    
        const CriticalSection& _cs;
    };
    

    Simon

    PS:

    Da nicht alle Leute Win32-API können, bringt es nichts, den Code zu posten.

    Glaub mir, da wissen ne Menge Leute ganz anderes nicht und es würde nie etwas bringen irgendetwas zu posten.. aber ist ja offensichtlich nicht so...



  • Ad aCTa schrieb:

    > weil man damit alles möglich ein deinem pool machen kann, am mutex vorbei usw..

    Was will man denn machen? Der Iterator ist konstant, der Pool kann durch ihn nicht verändert werden. Er hat nur Zugriff auf dieses eine Element, und das gehört jetzt dem Thread und muss bis zum putback nicht mehr synchronisiert werden.

    Dieses const bringt bei dem iterator garnichts.

    int t=0;
    int f(){return t++;}
    
    template <typename HTy_>
    class pool {
    public:
        typedef typename std::list<HTy_>::iterator iterator;
    
        template <typename Fnc_>
        pool(unsigned poolsize, const Fnc_& fnc)
        {
            while(--poolsize) unused_.push(fnc());
        }
    
        const iterator get() {
            used_.push_front(unused_.front());
            unused_.pop();
            return used_.begin();
        }
    
        void putback(iterator) {
            unused_.push(*iterator);
            used_.erase(iterator);
        }
    
    private:
        std::queue<HTy_> unused_;
        std::list<HTy_> used_;
    }; 
    
    int main()
    {
    	pool<int> p(3,f);
    	pool<int>::iterator ittmp = p.get();
    	pool<int>::iterator it = p.get();
    	std::cout<<*it;
    	it++;
    	std::cout<<*it;
    }
    


  • Ad aCTa schrieb:

    > Nein, du darfst keinen iterator auf den pool nach aussen geben.

    Warum?

    Weil ich dann einfach ein ++it; mache und boese Sachen machen kann. Weiters hast du ein Implementationsdetail im Interface - das ist meistens schlecht.

    Im Normalfall loest man sowas ueber Proxy Objekte. Dein iterator ist ja genauso ein Proxy objekt - nur dass er sich unnatuerlich anfuehlt und so fiese Sachen wie

    *i=some_other_resource;
    

    erlaubt.

    > Davon abgesehen hast du hier ja keinen relevanten Code, also kann man nichts sagen.

    Anwendungsfall wäre:

    Und der Code kommt dir nicht komisch vor mit dem iterator? Aber ja, in der Anwendung ist der pool trivial, du hast eine get und eine putback Methode. Das Spannende sind aber die konkreten Implementierungen.

    Weiters ist dein Ctor immernoch Fehlerhaft.
    Die used Liste kannst du in der Debug Variante ja fuehren. Dann fehlt natuerlich noch ein Dtor und eine schoene delete Funktion fuer die Resourcen.

    Dann gibts noch so kleinigkeiten wie zB der Pool sollte nicht kopierbar sein.

    Sprich du hast die Idee fuer einen Pool gehabt, das passt. Aber du musst alles zu Ende denken. Aber wenn du das Locking Fehlerfrei machst, die Poolklasse nicht kopierbar machst, die used Liste entfernst, den Fehler im Ctor fixt (uU einen dynamisch wachsenden Pool machst?), einen Dtor implementierst und die iteratoren entfernst - dann passt es so in etwa 🙂

    Und ueberdenke wie du Resourcen erstellen willst die nicht kopierbar sind.



  • > 1. CloseHandle(..) wird nicht aufgerufen bei deiner mutex Implemenation.

    Guter Punkt, das habe ich jetzt behoben. (Nutze Mutex der Windows API).

    > Weil ich dann einfach ein ++it; mache und boese Sachen machen kann.

    Das wusste ich noch garnicht! 😮 Dann ist klar, warum das nicht sicher ist...

    > Weiters hast du ein Implementationsdetail im Interface - das ist meistens schlecht.

    Ich weiß, der Code hier ist nur so zur Darstellung. Im echten Code sind sie getrennt. Ich schreibe es jetzt hier aber auch mal getrennt auf.

    Die used-Liste ist so gesehen sinnlos. Meine Idee war, dass ich aus der Queue einfach freie Handles bekomme (ohne große Schleifen mit Sachen wie if(iter->is_used()) ) und sie dann daraus entferne. Sobald der Client des pools fertig ist, soll Stack-Unwinding den Destruktor aufrufen, welcher das Handle in den Pool zurückschreibt. Der Pool speichert seine Handles in std::shared_ptr s, die dann auch von pool::get() zurückgegeben werden. Ich hätte gerne sowas:

    template <typename HTy_>
    std::shared_ptr<HTy_> pool<HTy_>::get() {
    	mutex lck(lock_);
    	std::shared_ptr<HTy> tmp(unused_.front().get(), std::bind(&pool<HTy_>::deleter, this, std::placeholders::_1)); // shared_ptr bekommen und Deleter einstellen
    	unused_.pop(); // aus Queue entfernen
    	return tmp; // Zack! (siehe unten)
    }
    
    // ...
    { // Scope
    	pool<socket> socket_pool(5, create_socket); // socket ist ein Handle-Wrapper
    	typedef std::shared_ptr<socket> socket_ptr;
    	socket_ptr socket = socket_pool.get();
    	// Service...
    } // Scope verlassen, der Deleter von shared_ptr soll das Handle in die Queue zurückschieben
    

    Der Deleter ist einfach sowas:

    template <typename HTy_>
    void pool<HTy_>::deleter(HTy* object) {
    	mutex lck(lock_);
    	unused_.push(std::shared_ptr<HTy>(object)); // neuen shared_ptr aus der Ressource erstellen + pushen
    }
    

    Oben steht "Zack!", und zwar weil ja schon der Destruktor des shared_ptr in der pool::get() samt Deleter aufgerufen wird und die Ressource in die Queue zurückschreibt, obwohl sie ja jetzt als "used" gilt. So kann das demnach nicht klappen. Wie kann ich das lösen? (Automatisches zurückschreiben in Queue)

    > die Poolklasse nicht kopierbar machst,

    Kommt danach, erst mal muss der Pool selbst funktionieren. 😉

    > den Fehler im Ctor fixt

    Fehler?



  • Ad aCTa schrieb:

    > den Fehler im Ctor fixt

    Fehler?

    while(--poolsize) unused_.push(fnc());
    

    Wieviele Objekte legst du an wenn poolsize zB 1 ist?

    ->

    while(poolsize--) unused_.push(fnc());
    

    PS:
    mir persoenlich gefaellt diese shared_ptr Variante nicht so toll weil ich diese Option lieber dem Clientcode ueberlasse, aber im Prinzip passt es so schon.


Anmelden zum Antworten