atomic std::function



  • Mein Ziel ist es, einen Callback thread-safe zu implementieren; damit meine ich nicht den Callback, sondern den Functor an sich.

    Da ein Functor (mit std::function) relativ leichtgewichtig ist, wollte ich ihn ursprünglich in einem std::atomic verpacken, aber Pustekuchen! std::function ist nicht trivially-copyable.
    Kleine Versuche mit nem Mutex das hübsch zu gestalten machten mich nicht glücklich, also hab ich mal bei boost vorbei geschaut, und siehe da -> boost::atomic hat diese Voraussetzung nicht.

    Nun stellt sich mir die Frage, warum die std einen so dermaßen beschneidet!?

    Hab das Problem mal relativ übersichtlich veranschaulicht; welche Lösung hättet ihr hier? Ist atomic "the way to go" oder sollte ich das anders angehen?

    #include <iostream>
    #include <string>
    #include <thread>
    #include <functional>
    #include <chrono>
    #include <cassert>
    
    class Foo
    {
    public:
        using Callback = std::function<void()>;
    
        void setCallback(Callback _cb)
        {
            m_Callback = _cb;   // muss synchronisiert werden
        }
    
        void emit()
        {
            // ab hier ist ebenfalls synchronisation nötig
            assert(m_Callback);
            m_Callback();
        }
    
    private:
        Callback m_Callback;
    };
    
    void cb()
    {
        std::cout << "free function!";
    }
    
    int main()
    {
        Foo foo;
        foo.setCallback([]{ std::cout << "lambda!!"; });
        std::thread th{[&foo]{
            foo.setCallback(&::cb);
        }};
        std::this_thread::sleep_for(std::chrono::microseconds(1));
        foo.emit();
    }
    


  • xxx::atomic, egal ob xxx nun std oder boost ist, kann nur mit nicht-trivial kopierbaren Typen umgehen können, wenn ne Emulation verwendet wird die hinter der Bühne Locks verwenden (typischerweise in Form eines Lock Pools). Das ist keine Frage der Implementierungsqualität sondern das ist einfach so, geht nicht anders.

    Und ich würde eher annehmen dass boost::atomic das auch nicht wirklich unterstützt, sondern ledlglich der "is trivially copyable" Check fehlt. Die Möglichkeit "is trivially copyable" zuverlässig zu überprüfen gibt es schliesslich erst seit C++11 (oder sogar später?), und boost::atomic ist deutlich älter.

    Davon abgesehen kannst du den Funktor im Atomic sowieso nicht aufrufen - du müsstest ihn rauskopieren.

    Langer Rede kruzer Sinn: Du kannst genau so gut ne std::mutex + std::function nehmen, und die Kopie des std::function für den Aufruf machen während du die Mutex gelockt hast.

    Bzw. wenn der Funktor selbst Thread-Safe ist, dann könnte std::shared_ptrstd::function funktionieren. Wobei sich dann wieder das Problem ergibt wie du den std::shared_ptr thread-safe kopieren kannst. Was AFAIK nur boost::shared_ptr kann, und auch dort ist es - mangels Hardware-Support - mit Hilfe eines Lock-Pools implementiert. Wenn dein Funktor billig zu kopieren ist (=klein genug, und die std::function Implementierung "small object" optimiert ist) wird es also vermutlich billiger sein den Funktor einfach zu kopieren. Bei grossen, teuer zu kopierenden Funktoren wird die shared_ptr Variante schneller sein. Also z.B. wenn du Vektoren oder Strings by value in den Funktor capturest.

    Kleiner Tip am Rande: Du solltest emit() nicht so implementieren:

    void emit()
    {
        std::lock_guard<std::mutex> lock(m_mutex);
        m_callback();
    } // m_mutex unlocked here
    

    Das wäre nämlich Schritt 1 in der Ultimativen Bauanleitung für Deadlocks (tm).

    Besser wie schon oben beschrieben so:

    void emit()
    {
        std::function<...> callback;
        {
            std::lock_guard<std::mutex> lock(m_mutex);
            callback = m_callback;
        } // m_mutex unlocked here
    
        callback(); // m_mutex NOT locked while executing callback!
    }
    


  • ps: Ja, ist so, nur PODs werden unterstützt. Lies den ersten Punkt in dieser Liste: http://www.boost.org/doc/libs/1_65_0/doc/html/atomic/limitations.html

    Using non-POD-classes as template parameter to atomic<T> results in undefined behavior: This means that any class containing a constructor, destructor, virtual methods or access control specifications is not a valid argument in C++98. C++11 relaxes this slightly by allowing "trivial" classes containing only empty constructors. Advise: Use only POD types.



  • ok, danke für die Aufklärung. Das hab ich tatsächlich übersehen, dass das nicht erlaubt ist 😉

    Ja, das wäre auch meine Implementierung gewesen um den Deadlock zu vermeiden. Hab zwar auch schon einen Vorschlag für std::recursive_mutex erhalten, aber das wollte ich eigentlich vermeiden.



  • std::recursive_mutex bringt dir hier nix. Bzw. es löst nicht das wesentlich blödere der beiden Probleme. Nämlich dass du, wenn du nen Callback machst während du ne Mutex angezogen hast, ganz schnell bei den verfressenen Philosophen landest:
    https://en.wikipedia.org/wiki/Dining_philosophers_problem



  • ok, danke.

    Hab jetzt mal versucht eine allgemein gültige LockedValue Klasse zu schreiben. Ist das so ok oder sieht hier jemand Probleme?
    Die Klasse soll eben vornehmlich dazu dienen, für den oben genannten Anwendungsfall nicht händisch irgend ein mutex locken zu müssen.

    #pragma once
    
    template <class T>
    class LockedValue
    {
    private:
    	using Lock = std::scoped_lock<std::mutex>;
    
    public:
    	LockedValue() = default;
    
    	LockedValue(const LockedValue& _other) :
    		m_Value(_other.load())
    	{}
    
    	LockedValue(LockedValue& _other) :
    		LockedValue(static_cast<const LockedValue&>(_other))
    	{}
    
    	LockedValue(LockedValue&& _other) :
    		m_Value(_other._takeValue())
    	{}
    
    	template <class... Args>
    	LockedValue(Args&&... _args) :
    		m_Value(std::forward<Args>(_args)...)
    	{}
    
    	LockedValue& operator =(const LockedValue& _other)
    	{
    		auto value = _other.load();
    		store(std::move(value));
    		return *this;
    	}
    
    	LockedValue& operator =(LockedValue&& _other)
    	{
    		if (this != &_other)
    			store(_other._takeValue());
    		return *this;
    	}
    
    	template <class T_, typename = std::enable_if_t<std::is_convertible_v<T_, T>>>
    	LockedValue& operator =(T_&& _value)
    	{
    		store(std::forward<T_>(_value));
    		return *this;
    	}
    
    	T operator *() const
    	{
    		return load();
    	}
    
    	template <class T_>
    	void store(T_&& _value)
    	{
    		Lock lock(m_Mutex);
    		m_Value = std::forward<T_>(_value);
    	}
    
    	T load() const
    	{
    		Lock lock(m_Mutex);
    		return m_Value;
    	}
    
    private:
    	mutable std::mutex m_Mutex;
    	T m_Value;
    
    	T&& _takeValue()
    	{
    		Lock lock(m_Mutex);
    		return std::move(m_Value);
    	}
    };
    

    EDIT: vll im dtor noch ein lock einbauen?



  • Im dtor musst du nix locken. Der dtor darf sowieso immer nur laufen wenn sonst keine Zugriffe mehr laufen können, das muss der User der Klasse garantieren.

    Was ich auf jeden Fall ändern würde:
    - Nimm lieber U statt T_ . T und T_ , das is ja komplett unübersichtlich/verwirrend.
    - Mach den operator * weg - der verwirrt auch bloss. Wer load will soll einfach load schreiben.

    Was ich auch fragwürdig finde ist dass deine Rvalue-Overloads nicht thread-safe sind. Der Lock in _takeValue bringt halt net wirklich viel. Der eigentliche Zugriff auf m_Value passiert ja erst nachdem dieser Lock schon wieder freigegeben wurde.

    Die einzige Variante die mir einfällt wie man das thread-safe implementieren kann und gleichzeitig echten Move-Support bekommen wäre beide Locks anzuziehen, dann zu moven und dann beide wieder freizugeben. Dabei musst du allerdings wiederum sicherstellen dass es beim Locken der beiden Mutexen nicht zu nem Deadlock kommen kann.
    Also z.B. mit std::lock beide gleichzeitig locken bzw. std::scoped_lock (C++17) verwenden.



  • - Nimm lieber U statt T_. T und T_, das is ja komplett unübersichtlich/verwirrend.

    Ok, sehe ich ein

    - Mach den operator * weg - der verwirrt auch bloss. Wer load will soll einfach load schreiben.
    

    Ich mag den operator. Macht optional ja auch nicht anders. Wobei der da natürlich eine Referenz returned...

    Was ich auch fragwürdig finde ist dass deine Rvalue-Overloads nicht thread-safe sind. Der Lock in _takeValue bringt halt net wirklich viel. Der eigentliche Zugriff auf m_Value passiert ja erst nachdem dieser Lock schon wieder freigegeben wurde.

    Die einzige Variante die mir einfällt wie man das thread-safe implementieren kann und gleichzeitig echten Move-Support bekommen wäre beide Locks anzuziehen, dann zu moven und dann beide wieder freizugeben. Dabei musst du allerdings wiederum sicherstellen dass es beim Locken der beiden Mutexen nicht zu nem Deadlock kommen kann.
    Also z.B. mit std::lock beide gleichzeitig locken bzw. std::scoped_lock (C++17) verwenden.

    Hmm, da hast du recht. Hab ich nicht weit genug gedacht. Es müsste doch funktionieren, wenn _takeValue die value aus einer temporären value nimmt und nicht aus dem Member, oder?

    T _takeValue()
    {
        Lock lock(m_Mutex);
        auto tmp(std::move(m_Value));
        return tmp;
    }
    

    Damit hätte ich zwar einen doppelten move drin, wäre aber wohl hübscher als einen zweiten Mutex einzubauen. Nutze btw c++17



  • Du bräuchtest keinen zweiten Mutex, du müsstest bloss die Mutexen der beiden beteiligten Instanzen gleichzeitig locken. Ist aber egal, deine Variante, an die ich nicht gedacht hatte, ist eh eleganter. 👍 Und vermutlich nur unwesentlich langsamer. Da sag ich doch glatt: https://www.youtube.com/watch?v=xteKObnaA2c

    Wobei ich es so schreiben würde:

    T _takeValue()
    {
        Lock lock(m_Mutex);
        return std::move(m_Value);
    }
    

    ~~Mit NRVO sollte deine Variante zwar gleichwertig sein, aber ich meine mich zu erinnern dass das Vorhandensein eines Move-Ctors NRVO aushebelt.
    ~~
    EDIT: Hab mal kurz gegoogelt, und einige Stack-Overflow Antworten sagen 'was anderes, also dass NRVO auch "move elision" machen darf. Fände ich gut, aber ich meine eben mich zu erinnern das anders gehört zu haben. Hm.

    EDIT2: Ne, scheint wirklich so zu sein, also NRVO geht unabhängig davon ob es nen move-ctor gibt oder nicht. Gut so 🙂



  • hustbaer schrieb:

    EDIT2: Ne, scheint wirklich so zu sein, also NRVO geht unabhängig davon ob es nen move-ctor gibt oder nicht. Gut so 🙂

    Hat sich da mit c++17 nicht eh grundlegend was geändert?

    Jedenfalls vielen Dank für deine Mühen!



  • anti-freak schrieb:

    hustbaer schrieb:

    EDIT2: Ne, scheint wirklich so zu sein, also NRVO geht unabhängig davon ob es nen move-ctor gibt oder nicht. Gut so 🙂

    Hat sich da mit c++17 nicht eh grundlegend was geändert?

    Ja irgendwann in letzter Zeit wurde da irgendwas geändert. Frag ich aber nicht genau wann und genau was 😃
    (Ich glaube dass jetzt an bestimmten Stellen copy- und move-elision vorgeschrieben ist wo es früher optional war.)

    anti-freak schrieb:

    Jedenfalls vielen Dank für deine Mühen!

    Bitte. BTW: LockedValue halte ich für einen schlechten Namen. Ich fände SynchronizedValue besser.


Log in to reply