Meine Thread Klasse (bitte mal testen)

Plassy

Hi,

ich bin zur Zeit dabei mich mit der Problematik multithreading zu befassen.
Also habe ich mir eine thread-Klasse geschrieben.
Das Ganze soll so funktionieren:

Man hat einen thread der eine beliebige Anzahl von tasks ausführen kann
(natürlich hintereinander und nicht parallel... selbstverständlich).
Mit "enqueueTask(Task* task)" hängt man einen task an die Liste (eine single-linked-list) an.
Der thread arbeitet die tasks hintereinander nach dem FIFO prinzip ab und entfernt
den jeweiligen task dann gleich aus der Liste.
Dann kann man noch einstellen ob sich der thread beenden soll nachdem alle
tasks abgearbeitet sind oder ob Er auf Neue warten soll... und der ganze mist eben.

Der code sollte größtenteils selbsterklärend sein:

class Thread
{
public:
	class Task
	{
	friend class Thread;
	public:
		Task() : nextTask(0){ }
		virtual ~Task(){ }
		virtual void operator()() = 0;

	private:
		Task* nextTask;
	};

	Thread() :
		mThread(0)                            ,
		mFirstTask(0)                         ,
		mLastTask(0)                          ,
		mIsQueuing(false)                     ,
		mIsAllowedToQueue(true)               ,
		mStopAfterProcessingAllTasks(false)   ,
		mStopAfterProcessingCurrentTask(false),
		mRemoveAllTasksAfterStopping(false)   ,
		mThreadIsRunning(false)               ,
		mNumTasksQueued(0)                    ,
		mNumTasksRemoved(0)
	{
	}

	virtual ~Thread()
	{
		delete mThread;

		while(mFirstTask)
		{
			Task* temp = mFirstTask;
			mFirstTask = mFirstTask->nextTask;
			delete temp;
		}
	}

	virtual void taskStarts(Task* task){ }
	virtual void taskEnds  (Task* task){ }

	void enqueueTask(Task* task)
	{
		while(not mIsAllowedToQueue)
			/* waiting */;

		mIsQueuing = true;

		if(not mFirstTask)
		{
			mFirstTask = mLastTask = task;
		}
		else
		{
			mLastTask->nextTask = task;
			mLastTask           = task;
		}

		mIsQueuing = false;
		++mNumTasksQueued;
	}

	inline bool isRunning() const
	{
		return mThreadIsRunning;
	}

	inline void stopAfterProcessingAllTasks()
	{
		mStopAfterProcessingAllTasks = true;
	}

	inline void stopAfterProcessingCurrentTask()
	{
		mStopAfterProcessingCurrentTask = true;
	}

	inline void removeTasksAfterStopping(bool doRemove=true)
	{
		mRemoveAllTasksAfterStopping = doRemove;
	}

	inline unsigned int getNumTasks() const
	{
		while(not mIsAllowedToQueue)
			/* waiting */;

		return mNumTasksQueued - mNumTasksRemoved;
	}

	inline void start()
	{
		if(mThreadIsRunning)
			return;

		delete mThread;

		mThread = new boost::thread(
			boost::bind(&Thread::threadFunction, this)
		);
	}

private:
	void threadFunction()
	{
		mThreadIsRunning = true;

		while(not mStopAfterProcessingAllTasks and not mStopAfterProcessingCurrentTask)
		{
			while(not mStopAfterProcessingCurrentTask)
			{
				while(mIsQueuing)
					/* waiting */;

				mIsAllowedToQueue = false;

				if(not mFirstTask)
				{
					mIsAllowedToQueue = true;
					break;
				}

				Task* temp = mFirstTask;
				mFirstTask = mFirstTask->nextTask;
				++mNumTasksRemoved;

				mIsAllowedToQueue = true;

				taskStarts(temp);
				temp->operator()();
				taskEnds(temp);
				delete temp;
			}
		}

		if(mRemoveAllTasksAfterStopping)
		{
			// remove the rest of the tasks if the thread stops
			// before all tasks could be processed
			while(mFirstTask)
			{
				Task* temp = mFirstTask;
				mFirstTask = mFirstTask->nextTask;
				delete temp;

				++mNumTasksRemoved;
			}
		}

		mThreadIsRunning = false;
	}

private:
	boost::thread* mThread;
	Task*          mFirstTask;
	Task*          mLastTask;
	unsigned int   mNumTasksQueued;
	unsigned int   mNumTasksRemoved;
	volatile bool  mIsQueuing;
	volatile bool  mIsAllowedToQueue;
	volatile bool  mStopAfterProcessingAllTasks;
	volatile bool  mStopAfterProcessingCurrentTask;
	volatile bool  mRemoveAllTasksAfterStopping;
	volatile bool  mThreadIsRunning;
};

Ich habe Das jetzt so implementiert, dass es ohne mutexes und system-lock Aufrufe auskommt...
Oder besser ich denke es. Ich weiß nämlich nicht wirklich ob Das funzt, da ich keinen
multi-processor/core PC besitze und das Ganze nur unter normalem fake-multithreading testen kann.

Ich hatte auch noch vor ne Version mit mutexes zu basteln und dann mal die performance zu vergleichen.

Wäre nicht schlecht wenn Das mal jemand testen oder ein paar Kommentare (konstruktive ;)) zu dem Design abgeben könnte.

mfg
Plassy

PS: Die Klasse wird so benutzt:

class WritingTask: public Threads::Thread::Task
{
	virtual void operator()()
	{
		std::cout << "hello World" << std::endl;
	}
};

int main(int argc, char **argv)
{
	Threads::Thread thread;

	for(int i=0; i<100000; ++i)
	{
		thread.enqueueTask(new WritingTask1());
	}

	thread.start();
	/* do bla bla blub */
}

----------------------------------------------------

delete-Aufruf geändert

WIEDER EIN TROLL

TROLLE BITTE NICHT FÜTTERN!
DANKE!

*D*Evil

Kennst du std::queue? Wäre was für dich ^^

if(mThread)
            delete mThread;

oft gesehen und dennoch überflüssig:

delete mThread;

und fertig delete 0 führt zu keinem Fehler ... usw. also da kann man einiges optimieren

Simon2

Plassy schrieb:

...Ich habe Das jetzt so implementiert, dass es ohne mutexes und system-lock Aufrufe auskommt...

Das wird lustig !

Gruß,

Simon2.

Plassy

(D)Evil schrieb:

Kennst du std::queue? Wäre was für dich ^^
if(mThread)
            delete mThread;
oft gesehen und dennoch überflüssig:
delete mThread;
und fertig delete 0 führt zu keinem Fehler ... usw. also da kann man einiges optimieren

zu std::queue:
hab ich auch schon dran gedacht. Meiner meinung nach war das problem aber zu simpel um einen stl-container reinzustopfen. Aber gut, das könnte ich natürlich noch ändern.

zu delete 0:
Huh?? Hab ich nicht gewusst

Plassy

Simon2 schrieb:

Plassy schrieb:

...Ich habe Das jetzt so implementiert, dass es ohne mutexes und system-lock Aufrufe auskommt...

Das wird lustig !

Gruß,

Simon2.

Deswegen ist es ja auch in einer Klasse gekapselt.

BorisDieKlinge

Ich würde mich nie Trauen ne Klasse von mir hier rein stellen;) Nach der kritik schieb man nur noch depries

hustbaer

Kann den Thread bitte wer löschen damit nicht noch irgendwer auf die Idee kommt den greislichen Horror da oben zu verwenden?

Plassy

@hustaer

Plassy schrieb:

...oder ein paar Kommentare (konstruktive ;)) zu dem Design abgeben könnte.

hustbaer

Was willst du von mir? Du setzt dich über sämtliche Regeln zum Thema Threading hinweg weil du dich in deiner unendlichen Selbstüberschätzung für reichlich schlau hältst, und postest gemeingefährlich falschen Code. Glaubst du nicht wenn es so einfach ginge wäre da vor dir schon jemand draufgekommen? Ja, wäre, und nein, so einfach geht es nicht.

Konstruktive Kritik: die ganzen "volatile bool" sind alle Blödsinn, das funktioniert so nicht, das führt nur zu bösen race conditions (aka. data races) und im endeffekt dazu dass das ding irgendwann abstürzt oder sonstwie gröber Blödsinn baut.

inline unsigned int getNumTasks() const
    {
        while(not mIsAllowedToQueue)
            /* waiting */;

        return mNumTasksQueued - mNumTasksRemoved;
    }

solange er nicht die erlaubnis zum queuen hat gibt er was zurück. und was macht er sonst?

wieso ist eigentlixch not blau. mein VS 2003 kennt kein not.

ich sehs. ein ; hinter dem kommentar.

hustbaer

not ist auch kein Keyword sondern ein define:

#define not !

Sollte sich in <iso646.h> befinden.

camper

hustbaer schrieb:

not ist auch kein Keyword sondern ein define:
#define not !
Sollte sich in <iso646.h> befinden.

not ist ein keyword - Visual C++ hält sich in diesem Punkt nur nicht an den Standard. Es ist in keinem Falle eine schlechte Idee, <ciso646> einzubinden für den Fall der Fälle.

Plassy

hustbaer schrieb:

Was willst du von mir? Du setzt dich über sämtliche Regeln zum Thema Threading hinweg weil du dich in deiner unendlichen Selbstüberschätzung für reichlich schlau hältst, und postest gemeingefährlich falschen Code. Glaubst du nicht wenn es so einfach ginge wäre da vor dir schon jemand draufgekommen? Ja, wäre, und nein, so einfach geht es nicht.

lol du bist ja ein fröhlicher Geselle.

hustbaer schrieb:

Konstruktive Kritik: die ganzen "volatile bool" sind alle Blödsinn, das funktioniert so nicht, das führt nur zu bösen race conditions (aka. data races) und im endeffekt dazu dass das ding irgendwann abstürzt oder sonstwie gröber Blödsinn baut.

Aber trotzdm würde ich doch gerne etwas näher wissen warum das jetzt in diesem fall zu race-conditions führen kann.
Die ganzen bools waren eigendlich dazu gedacht Das zu verhindern.
In wie fern funktioniert Das jetzt nicht??

MfG
Plassy

Plassy schrieb:

Aber trotzdm würde ich doch gerne etwas näher wissen warum das jetzt in diesem fall zu race-conditions führen kann.
Die ganzen bools waren eigendlich dazu gedacht Das zu verhindern.
In wie fern funktioniert Das jetzt nicht??

Wärend einer der bool Variablen verändert wird, kann ein Threadwechsel auftreten. Dann der Wert in der Boolvariable undefiniert.

Außderdem. Wer sagt dir, dass nachdem du eine dieser while-Schleifen verlassen hast, nicht direkt der Threadwechsel statt findet. Dann laufen plözlich 2 Threads gleichzeitig.

So eine Klasse kann man in der Praxis gar nicht gebrauchen.

Plassy

ProgChild schrieb:

Wärend einer der bool Variablen verändert wird, kann ein Threadwechsel auftreten. Dann der Wert in der Boolvariable undefiniert.

Außderdem. Wer sagt dir, dass nachdem du eine dieser while-Schleifen verlassen hast, nicht direkt der Threadwechsel statt findet. Dann laufen plözlich 2 Threads gleichzeitig.

Danke :D. Das ist doch mal ne konstruktive antwort
Aber die Werte der bool variablen dürften doch eigendlich nie undefiniert sein.
Wenn ein thread die Variable schreibt und der andere thread die Variable liest bevor der erste thread zuende geschrieben hat, wäre das doch eigendlich völlig egal da ja nicht der genaue Wert abgefragt wird. Es wird ja nur abgefragt ob die Variable 0 oder nicht 0 ist. Ob am ende irgend ein Datenmüll drin steht dürfte dann doch egal sein, da es nicht 0 und somit "true" wäre. Oder gibts in diesen Fall noch was dazwischen?

Dein zweites Argument verstehe ich leider nicht so richtig.
Kannst du das vielleicht noch mal irgendwie anders formullieren?

MfG
Plassy

Plassy

nutzlos schrieb:

So eine Klasse kann man in der Praxis gar nicht gebrauchen.

Und wieso nicht...??
Ist Das jetzt auf die Funktion bezogen oder auf die Umsetzung?

Werner Salomon

Plassy schrieb:

nutzlos schrieb:

So eine Klasse kann man in der Praxis gar nicht gebrauchen.

Und wieso nicht...??
Ist Das jetzt auf die Funktion bezogen oder auf die Umsetzung?

Hallo Plassy,

Eine race-condition kann genau dann auftreten, wenn einer der Thread unmittelbar hinter der Schleife

while(not mIsAllowedToQueue) // bzw. while(mIsQueuing)
            /* waiting */;

unterbrochen wird und der andere Thread jetzt weiterläuft. Das Flag, was Die Sperrung des Bereichs markiert - also entweder 'mIsQueuing' bzw. 'mIsAllowedToQueue' - ist dann noch nicht gesetzt. Dann rennt der zweite Thread in den anscheinend geschützten Bereich, wird womöglich selber unterbrochen, dann kommt wieder der andere dran .. und die race-condition ist da.

.. es gibt noch einen Grund. Das betrifft die Art und Weise, wie Du wartest. Du tust das in der schon erwähnten Schleife, die lediglich auf die Änderung eines bool-Wertes reagiert. Das klappt i.A. wenn Dein Programm mit den zwei Threads das einzige - oder fast das einzige - auf dem Rechner ist. Wenn Du das Verfahren in mehr als einem Thread machst und/oder die CPU-Last hoch geht, dann kann es schon passieren, dass Deine CPU sich zu mehreren 10% nur noch in diesen Schleifen tollt. Auf die Performance des Gesamtsystems wirkt sich das furchtbar aus.

Schau Dir vielleicht mal dies hier an. Ich verwende da zwar nur einen int-Wert als Kommando aber grundsätzlich könnte man den Kommando-Typ auch durch ein boost::function<void> ersetzen. Dann wäre man völlig flexibel.

Gruß
Werner

Bashar

ProgChild schrieb:

Wärend einer der bool Variablen verändert wird, kann ein Threadwechsel auftreten. Dann der Wert in der Boolvariable undefiniert.

Aber nur theoretisch. Ein bool ist ein Byte, das schreibt man in einer Instruktion, da kann also kein Threadwechsel dazwischenfunken.