threadsafe?

[gamer8o4]

ich hab immer probleme zu erkennen, ob eine klasse die ich geschrieben hat zu 100% threadsafe ist. zum beispiel habe ich eine timerklasse geschrieben, dessen code ich hier auch einmal reingeschrieben habe.
Habt ihr irgentwelche allgemeinen tipps, wie man code der nicht threadsafe ist, erkennen, finden, verbessern kann. Und ist zum beispiel meine timer-klasse wirklich threadsafe?

#ifndef DELTA_CORE_timers_H
#define DELTA_CORE_timers_H

#include "types.h"
#include "threadingmodels.h"
#include "uncopyable.h"

#include <time.h>

namespace delta{
namespace core{

	// simple timer-class based in <time.h>
	// -> precision: miliseconds (1sec == 1000ms)
	// -> uncopyable
	template<
		template<class,class> class ThreadingModel = DELTA_CORE_THREADS_MODEL_DEFAULT,
		class Mutex = DELTA_CORE_THREADS_MUTEX_DEFAULT
	>
	class timer 
		: public ThreadingModel<timer<ThreadingModel,Mutex>, Mutex>,
		  public uncopyable
	{
	private: // data
		bool running_;
		bool resetted;
		core::u32 begin, end;

	public: // construction
		timer(){
			running_ = false;
			resetted = true;
		}
		//~timer();

	public: // functions

		// start/continue the timer
		void start(){
			lock lock_(this);

			if(running_ == false) {
				if(resetted == true)
					begin = (core::u32) clock();
				else
					begin -= end - (core::u32) clock();
				running_ = true;
			}
		}

		// stop the timer
		void stop(){
			lock lock_(this);

			if(running_ == false) {
				end = (core::u32) clock();
				running_ = false;
			}
		}

		// reset the elapsed timer (timer will go on if it is running)
		void reset(){
			lock lock_(this);

			bool were_running = running_;

			if(were_running) stop();

			resetted = true;
			begin = end = 0;

			if(were_running) start();
		}

		// test if the timer is running
		bool running() const{
			lock lock_(this);
			return running_;
		}

		// get the elapsed time in miliseconds (1sec == 1000ms)
		core::u32 get_time(){
			lock lock_(this);
			if(running_ == true) return ((core::u32) clock() - begin) /*/ CLOCKS_PER_SEC*/;
			return end - begin;
		}

		// test if a specific time has elapsed (miliseconds: 1sec == 1000ms)
		bool is_over(core::u32 ms) const{
			lock lock_(this);
			return ms >= get_time();
		}

	};// c: timer

};// ns: core
};// ns: delta

[Eqalon]

gamer8o4 schrieb:

ich hab immer probleme zu erkennen, ob eine klasse die ich geschrieben hat zu 100% threadsafe ist.

Beschreib doch mal was Threadsafe für deine Klasse bedeutet. Und nicht nur sowas wie: "Sie kann von mehreren Threads verwendet werden."

Die Klasse sieht schon bald sehr Deadlocksafe aus. Und einige Sachen schauen sogar ohne Threads falsch aus, z.B. stop.

[gamer8o4]

threadsafe heißt für mich, dass mehere threads geichzeitig ein objekt der klasse ohne komplikationen zur gleichen zeit verwenden können. dabei wäre dann noch die frage, ob die funktionen in ctime trotzdem noch von anderen klassen verwendet werden können. (eigentlich ja schon, sind ja system calls und die sind threadsafe)

was ist denn an der stoppfunktion falsch?

[Gast]

Müsste es nicht running_ == true lauten, dort?

[Eqalon]

gamer8o4 schrieb:

threadsafe heißt für mich, dass mehere threads geichzeitig ein objekt der klasse ohne komplikationen zur gleichen zeit verwenden können. dabei wäre dann noch die frage, ob die funktionen in ctime trotzdem noch von anderen klassen verwendet werden können. (eigentlich ja schon, sind ja system calls und die sind threadsafe)

Stellt sich die Frage, was das für einen Sinn bei dem Timer hat? Weißt du nicht welcher Thread den Timer wann startet und stoppt? Ist es dann nicht sinnlos irgendwas messen zu wollen, wenn du nicht mal weißt ob der timer läuft?

[gamer8o4]

hat nen sinn! der timer könnte ja auch aufzeichen wie lange das programm schon läuft. dann weiß natürlich welcher thread startet und stoppt, aber nicht welche threads alles über is_over und get_time darauf zugreifen

außerdem kann ich über das template threading_model ganz einfach alle mutexe und so "abschalten", also mache ich es lieber alles threadsafe, ob ichs brauche kann ich dann später auch noch entscheiden...

das mit der stoppfunktion gucke ich gleich nochmal.an. muss die klasse sowieso noch testen...

[volkard]

Ich würde running und resetted und end ganz weglassen.
Schnittstellen-einfache Klassen laufen auch meistens schneller.
(Heißt es überhaupt "resetted", es heißt ja auch nicht "setted", sondern "set".)
Und so Kommentare wie "//construction" sind unsinnig, die stören nur den Lesefluss.
Wenn der Kommentar sagen muss "// start/continue the timer", dann wäre es doch besser, start() und continue() anzubieten.
Der Kommentar "// stop the timer" ist auch nutzlos.
reset() halte ich für nutzlos. Soll man sich doch einen neuen Timer anlegen.

uncopyable ist nicht nötig. Ist aber aucgh kein Fehler. Ich selber bin da auch sehr restriktiv und mache erstmal alles uncopyable und mache es erst auf, wenn in einer Anwendung echt Bedarf besteht.

Reicht es, die mixins private zu erben? Als Attribute wären sie doch auch private?

#ifndef DELTA_CORE_timers_H
#define DELTA_CORE_timers_H

#include "types.h"
#include "threadingmodels.h"
#include "uncopyable.h"

#include <time.h>

namespace delta{
namespace core{
	// simple timer-class based on <time.h>
	// -> resolution: miliseconds
	// -> precision: at least 1/18 sec
	// -> max runtime: 49.7 days (2^^32/1000)
	//    -> please lol at //http:support.microsoft.com/kb/216641
	static_assert(CLOCKS_PER_SEC==1000,"above comment lies");
	template<
		template<class,class> class ThreadingModel = DELTA_CORE_THREADS_MODEL_DEFAULT,
		class Mutex = DELTA_CORE_THREADS_MUTEX_DEFAULT
	>
	class timer 
		: private ThreadingModel<timer<ThreadingModel,Mutex>, Mutex>,
		  private uncopyable
	{
	private:
		core::u32 elapsed;
#ifndef NDEBUG
		bool runnung;
#endif
	public:
		timer(){
			elapsed=0;
#ifndef NDEBUG
			runnung=false;
#endif
		}
		void start(){
			lock lock_(this);
#ifndef NDEBUG
			assert(running==false);
			runnung=true;
#endif
			elapsed -=(core::u32) clock();
		}
		void continue(){
			start();
		}
		void stop(){
			lock lock_(this);
#ifndef NDEBUG
			assert(running==true);
			runnung=false;
#endif
			elapsed +=(core::u32) clock();
		}
		core::u32 get_time(){
			//nur aufrufen, wenn der timer gestoppt ist. 
			lock lock_(this);
#ifndef NDEBUG
			assert(running==false);
#endif
			return elapsed;
		}
		bool is_over(core::u32 ms) const{
			//nur aufrufen, wenn der timer gestoppt ist. 
			lock lock_(this);
#ifndef NDEBUG
			assert(running==true);
#endif
			return ms >= elapsed +(core::u32) clock();
		}

	};// c: timer

};// ns: core
};// ns: delta

Nicht erschrecken. Ohne die checks ist es nur als im Wesentlichen als Code

class timer{
	private:
		core::u32 elapsed;
	public:
		timer(){
			elapsed=0;
		}
		void start(){
			lock lock_(this);
			elapsed -=(core::u32) clock();
		}
		void continue(){
			start();
		}
		void stop(){
			lock lock_(this);
			elapsed +=(core::u32) clock();
		}
		core::u32 get_time(){
			lock lock_(this);
			return elapsed;
		}
		bool is_over(core::u32 ms) const{
			lock lock_(this);
			return ms >= elapsed +(core::u32) clock();
		}
	};// c: timer

Das sieht doch recht schnuckelig aus. Es geht absolut nichts über Schnuckeligkeit beim Design von Klassen!

Zur eigentlichen Frage:
Du benutzt clock und sonst nix.
Also erst schauen, ob clock() threadsicher ist.
http://en.cppreference.com/w/cpp/chrono/c/clock
Hmm.

Falls nicht, muss jeder Aufruf von clock mit lock geschützt werden.
Oder

delta::core::u32 delta::core::clock(){
   static mutex m;//noch mit den Threading-Model-Sachen verzieren
   lock(m);
   return (delta::core::u32)::clock();
}

Nehmen wir an, es sei threadsicher.

Dann erstmal defensiv JEDE Methode mit lock verzieren. Außer dem Konstruktor, ok. Das haste gemacht. Und dann ist die Klasser threadsicher. Ganz von alleine.

Und dann überlegen, ob es nötig ist. Was kann maximal passieren?

Bei Deinem Ansatz mit 25000 Attributen, die irgendwie zusammenarbeiten, gab es keine Chance. Alles zumachen.

Mit nur "elapsed" (hatte ich eigentlich so aus Performance-Überlegungen erfunden, als es noch keine Mehrkerner gab), wenn elapsed ein atomic-type ist, muss man da noch locken?
Ich würde festlegen, daß nur der Thread, der einen timer startet, ihn auch stoppen darf. (sichern mit NDEBUG, GetThreadID und assert).
Ja, man muss locken, weil der Status "running" sich parallel ändern kann.
Muß man noch locken, wenn man statt elapsed ein begin und end hat?
Hmm, nee, oder?

Es ist scheiße, dem Anwender freizustellen, ob er

timer t;
t.start();//nicht nötig, da er auch automatisch startet

oder

timer t;//startet automatisch

schreibt.
Das führt nur zu Ungemach und bald zu einem Großnotstand. Das muß man festlegen, ob der timer nach Erzeugung läuft oder nicht.

Entsprechend

timer t;
t.start();
t.start();

Was soll das? Wer als Anwender sowas schreibt, macht auch Speicherlöcher.
Nicht mit if absegnen, sondern mit assert bestrafen. Früh bestrafen, hart bestrafen. Nur so lernt er es! Der "er" bin meistens ich. Und das hat nichts mit Masochismus zu tun. Als Maso würde ich viel mehr boost benutzen und niemals using namespace std. Davon gibt es genug hier, die große Mehrheit sogar. Seit Jahren benutze ich (bis auf wirklich sehr seltene Ausnahmen) keinen Debugger mehr. Tut einfach nicht not.

[knivil]

#ifndef NDEBUG
            assert(running==true);
#endif

Das nenn ich ifndef-Obsfucation und wird von mir vermieden. Wer soll denn so einen Schrott lesen und warten?

ob ichs brauche kann ich dann später auch noch entscheiden

Allso Bullshit-Design!

template<
        template<class,class> class ThreadingModel = DELTA_CORE_THREADS_MODEL_DEFAULT,
        class Mutex = DELTA_CORE_THREADS_MUTEX_DEFAULT
    >
    class timer
        : public ThreadingModel<timer<ThreadingModel,Mutex>, Mutex>,
          public uncopyable

Omfg, dachte das sollte 'ne Timerklasse werden ... keine Uebug in Metatemplatespagethiiiieeee.

Sorry, ich sehe keinen Sinn fuer all diese Zeilen. Mein Vorschlag, einfach struct timespec ein paar ordentliche Operationen wie now, <, ==, + oder - spendiert wie beispielsweise hier: http://ideone.com/FmqaSG . OPerator - habe ich jetzt nicht gemacht, weils eher aus dem Kopf geschrieben wurde.

[gamer8o4]

das war mal ne antwort, besten dank, habs kapiert :))

[volkard]

knivil schrieb:

#ifndef NDEBUG
            assert(running==true);
#endif

Das nenn ich ifndef-Obsfucation und wird von mir vermieden. Wer soll denn so einen Schrott lesen und warten?

Im Kontext war es hier so besser. Der Code war genau für gamer8o4 geschrieben. Das ist kein Produktiv-Code.
Im Produktiv-Code steht bei mir niemals #ifdef oder #ifndef. Es gibt andere Wege. Aber ich will ehrlich nicht, daß gamer8o4 zur Zeit diese Nebensächlichkeiten perfektioniert.

[Gast]

Das mit dem "generalisierte Policy-Bibliothek nur für sich selbst"-schreiben birgt immer das Risiko, dass man das ganze nicht gut durchtestet, weil man eben ja eigentlich nur seinen eigenen Code als Benutzer hat, wobei dennoch der negative Punkt der schlechteren Lesbarkeit bleibt. Ich finde, dort ist ein guter Ansatz, erst solche Template-Policy-Basisklassen einzubauen, wenn man sie wirklich braucht. Sowas ist ja eigentlich schnell nachgerüstet.

[otze]

und mir ist gerade aufgefallen, dass clock() für einen timer beim threading mit die unglücklichste wahl ist. Volkards link zur docu sagts eindeutig: clock() muss bei mehreren threads nicht wall-time zurückgeben. Und auf meinem System tut es das auch nicht.

Also, ist die Klasse Thradsafe: nee, die geht sofort kaputt, wenn sie mit mehreren Threads verwendet wird.

Als nächster Punkt, und das ist durchaus nur meine Bescheidene Meinung: in vielen Fällen muss nicht eine Klasse THreadsafe gemacht werden, sodnern ihre Verwendung. Wer einfach nur überall extensiv lockt, und das dann auch noch, wie du, in einem Spiel macht, fliegt auf die Schnauze.

Kann man nicht statt eines scoped-locks so eine Art guard-Klasse verwenden und damit das locking aus den Klassen enternen?

Ich dachte an etwas, das in etwa wie folgt in der Verwendung aussieht:

Locker<Timer> timer;//erzeugt einen timer sowie einen mutex

//einmalige verwendung, macht lock(mutex); timer.get_time(); unlock(mutex);
timer->get_time();

{//mehrfache verwendung, spart lock-operationen
Lock<Timer> lock = timer->lock();//lock(mutex)
lock->get_time();//0 overhead
lock->get_time();//o overhead
}//hier unlock(mutex)

dann muss man sich nicht in allen Klassen mit Threading beschäftigen, sondern nur die Klassen, von denen man weiß, dass sie mit Objekten aus unterschiedlichen Threads hantieren müssen.

[volkard]

otze schrieb:

Ich dachte an etwas, das in etwa wie folgt in der Verwendung aussieht:

{//mehrfache verwendung, spart lock-operationen
Lock<Timer> lock = timer->lock();//lock(mutex)
lock->get_time();//0 overhead
lock->get_time();//o overhead
}//hier unlock(mutex)

dann muss man sich nicht in allen Klassen mit Threading beschäftigen, sondern nur die Klassen, von denen man weiß, dass sie mit Objekten aus unterschiedlichen Threads hantieren müssen.

Das ist generell eine sehr gute Idee.
Kack auf die Threading-Model-Policies! Nicht das Objekt weiß, wann gelockt werden muss, sondern der Anwender.
(Dazu kommt in Kürze nochwas extremes...)

[Gast]

Hey, den Ansatz finde ich cool. Muss ich mir für meine Problemstellung noch etwas einfallen lassen, weil ich bei mir eigentlich an vielen Stellen nur gewisse Interfaces zu IN einem Objekt habe (Also das Objekt bietet die Interfaces über einen Reflektions-Mechanismus an, das bedeutet noch lange nicht, dass es von diesen ableitet). Und so wird natürlich die Kopplung zwischen den involvierten Hilfsklassen und den Objekt wieder etwas vergrößert. Hrmm, hrmmm. Außerdem, wenn man den Lock dann in den Schnittstellen definiert, hat man virtuelle Aufrufe, selbst bei Objekten, die gar nicht gelockt werden brauchen. Hrmm, Hrmm.
Edit: Also bei mir muss grundsätzlich jedes Objekt eigentlich gelockt werden, weil alles parallel abläuft, von daher ist der Ansatz natürlich immer noch viel stringenter, aber der Vorteil mit der Performance erübrigt sich eigentlich. Bleibt natürlich noch der Vorteil, dass man so einfach über mehrere Operationen locken kann, dafür hatte ich bei einer speziellen Schnittstelle bei mir eben ein solches Lock vorgesehen, wie volkard es beschrieben hat. Und dann so lange nicht mehr damit gearbeitet, dass ich es bis eben wieder vergessen habe. Mal sehen, wieviel Mehrarbeit die erhöhte Kopplung kostet, wenn ich das überall durchziehe...

[gamer8o4]

Der Ansatz ist wirklich genail, was ich mir da an zeit, übersichtlichkeit performance,... spare
ich mach mich glaube mal direkt ran

[otze]

Der Nachteil ist, dass man sich recht einfach potentielle Deadlocks einfangen kann:

Locker<A> objA;
Locker<B> objB;
void threadFunc1(){
    Lock<A> locka = objA.lock();
    Lock<B> lockb = objB.lock();
}
void threadFunc2(){
    Lock<B> lockb = objB.lock();//oops, ich vertausche die beiden Zeilen...
    Lock<A> locka = objA.lock();

}

be carefull

[volkard]

gamer8o4 schrieb:

Der Ansatz ist wirklich genail, was ich mir da an zeit, übersichtlichkeit performance,... spare
ich mach mich glaube mal direkt ran

Beispiel, wo es wirklich rocken würde, wenn man logfile und << selber implementieren würde:

logfile<<"Hello"<<' '<<"world"<<endl;

Das lockt mindestens bei den vier Teilen viermal, bestimmt fünfmalm weil endl=='\n'<<flush, und wenn man Pech hat sogar pro Zeichen.

UND es vermischt die Ausgaben der Threads! cout muss doch gar nicht Threadsicher sein in dem Sinne, daß nix doppelt oder zu wenig ausgegeben wird. Verschwendete Rechenzeit. Denn das hülft gar genau gar nix, wenn ich tausend Web-Client-Threads dranhängen habe und die was loggen. Und teuer ist es auch noch.

Lock(logfile)<<"Hello"<<' '<<"world"<<endl;

und es gibt keine Vermischung. Und es ist viel billiger.

Und wenn ich single-Threaded unterwegs bin brauche ich das locken im Herzen von cout auch nicht.

[ghfjgfdjg]

otze schrieb:

Ich dachte an etwas, das in etwa wie folgt in der Verwendung aussieht:

Locker<Timer> timer;//erzeugt einen timer sowie einen mutex

//einmalige verwendung, macht lock(mutex); timer.get_time(); unlock(mutex);
timer->get_time();

{//mehrfache verwendung, spart lock-operationen
Lock<Timer> lock = timer->lock();//lock(mutex)
lock->get_time();//0 overhead
lock->get_time();//o overhead
}//hier unlock(mutex)

Man könnte auch sowas ähnliches machen, dass man einen einfachen Timer hat, der nur Funktionen anbietet, die kein lock brauchen und eine Methode die einen LockedTimer zurückgibt, der dann alle Methoden hat.

[volkard]

ghfjgfdjg schrieb:

Man könnte auch sowas ähnliches machen, dass man einen einfachen Timer hat, der nur Funktionen anbietet, die kein lock brauchen und eine Methode die einen LockedTimer zurückgibt, der dann alle Methoden hat.

Sowas ist mir sogar mal gelungen, ohne daß ich verschiedene Klassen anbieten musste.
Ist aber ein paar Tage her, hab die Details nicht mehr im Kopf.

Ein const-Objekt o. Jeder kann sich davon beliebig eine const-Referenz ziehen im Funktionsaufruf oder eine Zeiger. Das war für alle, die nicht locken müssen. Innendrin mußte das Objekt halt mutable benutzen.
Und mit Lock(o) kann man sich eine non-const-Referenz besorgen, die zu einer const-Ref im Aufruf konvertiert wird, wärend die non-const-ref noch lebt oder irgend sowas. Es war sehr verfrickelt, hat zwar geklappt, aber ich gab es schnell wieder auf wegen weil es total unintuitiv war.

Wie steht's eigentlich mit volatile?

uint32_t globalClock::get_time();
uint32_t globalClock::set_time() volatile;

Ich hab noch nie volatile bei Methoden gesehen.

[knivil]

den Ansatz finde ich cool ... Ansatz ist wirklich genail

Ohja, sehr spektakulaer ... Lockguards sind jetzt nicht gerade neu und Varianten gibt es viele.

UND es vermischt die Ausgaben der Threads! cout muss doch gar nicht Threadsicher sein in dem Sinne, daß nix doppelt oder zu wenig ausgegeben wird. Verschwendete Rechenzeit. Denn das hülft gar genau gar nix, wenn ich tausend Web-Client-Threads dranhängen habe und die was loggen. Und teuer ist es auch noch.

Im Vergleich mit Schreiben auf cout wahrscheinlich trotzdem billig. Hier ein Beispiel mit monitor<T>: http://channel9.msdn.com/Shows/Going+Deep/C-and-Beyond-2012-Herb-Sutter-Concurrency-and-Parallelism , geht in die Richtung von ghfjgfdjg's Vorschlag.

Der Nachteil ist, dass man sich recht einfach potentielle Deadlocks einfangen kann:

Immer wenn 2 Locks involviert sind, besteht diese Gefahr. Aber das ist auch nur Grundlagenkram.

Ich hab noch nie volatile bei Methoden gesehen.

http://www.drdobbs.com/cpp/volatile-the-multithreaded-programmers-b/184403766

Ansonsten kann man sich auch mal http://www.youtube.com/playlist?list=PL1835A90FC78FF8BE ansehen, bevor man multithreaded programmiert.

[Gast]

knivil schrieb:

...

Mit dem falschen Fuß aufgestanden?