std::mt19937

singletonPattern schrieb:

Ist das schlechter Stil und wenn ja warum?

deshalb: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Ri-singleton
"Singletons are basically complicated global objects in disguise."

Außerdem ist zumindest mal die Member-Variable

std::random_device m_rd;

völlig überflüssig - du willst doch den Generator nur einmalig seeden und brauchst diese Variable nicht im State!

TyRoXx

singletonPattern schrieb:

Ist das schlechter Stil und wenn ja warum?

Die Methoden sind aus jedem Thread aufrufbar, aber nicht thread-safe.

Funktionen, die den Generator benutzen, sind völlig unnötigerweise nicht-deterministisch. Nachvollziehbarkeit und Testbarkeit gehen ohne Grund flöten.

wachs

Hab mir das nochmals angeschaut. Ich denke ich nimm den generator rein in den Thread.

Anderseits habe ich mir noch ein paar Gedanken zum zentralen Generator gemacht. Was meint ihr könnte man den in etwa so machen? Das sollte mMn threadsicher sein.

std::mutex mtx;
int rnd_value2(int min,int max,unsigned long const &cseed){
	static __declspec(thread) std::mt19937 *re=nullptr;
	if(!re)re=new std::mt19937(cseed);
	std::uniform_int_distribution<int>ui(min,max);

	std::lock_guard<std::mutex> mtx_lock(mtx);
	return ui(*re);
}

wachs

Was ist denn das jetzt wieder für eine Krücke? Guck dir doch nur mal das Interface an. Du musst immer 3 Dinge (min, max, seed) übergeben, ob der dritte aber benutzt wird oder nicht, hängt davon ab, ob zuvor schon einmal die Funktion aufgerufen wurde?! Das führt doch nur dazu, dass man einfach immer seed=0 setzt in der Hoffnung, dass der generator schon existiert.

Wenn du eh einen mutex verwendest, dann gib doch den Generator von außen in den Thread rein. Wenn du thread-local Variablen hast, warum dann mutex?

wachs

@wob das stimmt doch nicht

Ab der ersten Instanzierung von re ist die Instanzierung so nicht mehr beeinflussbar, da es static und auch eine if() vorhanden ist.

Als Krücke kann man es schon ansehen, aber es ist grundlegend das Prinzip das mich interessiert. Und so funktioniert das Bestens.

Was stimmt nicht von dem, was ich geschrieben hatte?

Ab der ersten Instanzierung von re ist die Instanzierung so nicht mehr beeinflussbar, da es static und auch eine if() vorhanden ist.

Das war ja gerade meine Kritik. Von außen sieht man es nicht. Daher muss man entweder jedes Mal einen sinnvollen Seed mitgeben oder alternativ irgendwo speichern, ob man schon irgendwann mal die Funktion aufgerufen hat. Man wird dann gerne sagen "ach, wurde bestimmt schon mal initialisiert, ich übergebe als Seed einfach 0", denn jedes Mal einen neuen Seed zu ermitteln wäre ja komplett bescheuert.

Und ein weiteres Problem: Du gibst den Speicher nie frei und kannst es auch nicht.

=> Es funktioniert also alles andere als bestens.

wachs

Ach so sorry, hab da was falsch verstanden.

Ich hab das Prinzip vorher schon in eine Klasse umgebaut, und eine Belastungsprobe mit 20 Threads a je 1 mio. Zufallszahlen mit unterschiedlichen min/max Werten gemacht. Funkst Bestens.

Das war auch der Grund warum ich random_device rausgeschmissen habe. Ich finde den so Performancelastig, dass ich den seed-Wert nun selber eingebe. (Dafür gibt es viele Möglichkeiten um einen neuen Seed zu wählen, hash, clock etc.)

In der Klasse kann auf Wunsch Seed problemlos jederzeit geändert werden. Es wird dazu keine neue Instanz erzeugt. operator()() oder operator()(min,max) spucken die Zufallswerte aus. Sobald das Programm aus dem letzten Scope austritt, wir auch der Destruktor der Klasse aufgerufen.

Scheint mir für den ersten Moment eine gute Sache zu sein.

wachs

hustbaer

Damit dass du den Generator in die Hilfsfunktion verschiebst, trennst du bloss 'was auf was mMn. nicht aufgetrennt werden sollte. Nämlich die Entscheidung welcher Generator verwendet wird von dem Code der den Generator eben verwendet.

Gleichzeitig vermischt du zwei Dinge die mMn. nicht vermischt werden sollten, nämllich wie der Generator geseedet wird mit allen Stellen wo er verwendet wird.

Softwaredesignmässig sind das zwei schlimme Dinge.
Rein technisch betrachtet ist es natürlich "OK", in dem Sinn dass es funktionieren wird.

ps: Was das Freigeben des Speichers angeht, das ist wirklich ein Problem. Wäre es nen normales Singleton, könnte man es vernachlässigen. Aber eine geleakte Instanz pro Thread, das kann sich schnell summieren. Zumal MT19937 Generatoren ja nicht gerade ganz klein sind (IIRC ~4 KB pro Instanz).

wachs

Da kann ich dir nur recht geben, dass es Designmässig nicht der Hit ist.

Geseedet wird bei der Instanzierung, also im Thread selber. Diese könnte indivuduelle, für jede Thread umgeändert werden, wie die min + max auch. Wenn Singleton genutzt werden würde, währe wahrscheinlich eine einheitliche Seed über alle Threads vorhanden.

Ich wollte das einfach nur mal ausprobieren, rein aus Neugier. Für mich kommt diese Variante sowieso nicht in Frage.

Gruss wachs