Klassenfrage :)

Finnegan

Ich hab das c-array fbspiegel in ein std::vector geändert und alle Aufrufe natürlich angepasst; die Geschwindigkeit ist untragbar langsam gegenüber dem c-array, der Bildaufbau ist betrachtbar.

Es gibt keinen technischen Grund, weshalb einstd::vector langsamer als ein dynamisches C-Array sein sollte, zumal man im Zweifelsfall mit vector.data() auch direkten Pointerzugriff auf den Speicherbereich bekommen kann.

Eventuell verwendest du den vector auf eine ineffiziente Weise. vector.at() macht z.B. noch einen Range Check und wirft eine Exception bei einem Zugriff außerhalb des Bereichs. Wenn man damit Grafik mit vielen Millionen Pixeln machen will (und vielleicht sogar noch Animationen), dann kann das IMHO schon zu einer sichtbaren Verlangsamung führen.

Auch die Sanitizer (falls aktiviert) lassen den Code bestimmt nicht schneller werden.

Mach nochmal ein Commit auf Github und wenn ich die Tage wieder Zeit finde, kann ich mir das mal ansehen.

EL europ

@Finnegan
Ja ich hab fpspiegel im Konstruktor mit "fbspiegel.resize(bufferlenght)" initialisiert und in den Methoden mit fbspiegel.at() darauf zugegriffen. Nach den Ergebnissen hab ich es aber wieder rückgängig gemacht, und erst mal nach deinem Hinweis dem Apple ein AppleParas als Parameter gegen die Einzelwerte getauscht, so das die mandel.cpp etwas aufgeräumter aussieht.
mit den sanitizern läuft es mir immer in einen Fehler. Auch sanitize::leak zeigt das Apple mit ui->textFertig() Probleme macht; auch nachdem ich die sprintf()-Anweisung dort durch "Festwerte" ersetzt hatte.
Liegt es vielleicht daran das ich kein "this->" in den Methoden verwende?

Finnegan

@EL-europ sagte in Klassenfrage :

@Finnegan
Ja ich hab fpspiegel im Konstruktor mit "fbspiegel.resize(bufferlenght)" initialisiert und in den Methoden mit fbspiegel.at() darauf zugegriffen.

Das ist prinzipiell ja auch gut und richtig, besonders wenn du so viele Probleme mit Pufferüberläufen hast. Aber es geht halt ein wenig auf die Performance. Das ist für viele Anwendungen nicht relevant, aber bei einem Grafik-Buffer würde ich da vielleicht eine Ausnahme machen.

Dennoch sollte man bei Indexberechnungen sorgfältig darauf achten, dass die Buffer-Grenzen nicht über- oder unterschritten werden. Ich würde da wohl viel mit assert arbeiten (diese Prüfungen sind nur im Debug-Modus aktiv, bzw. wenn NDEBUG nicht definiert ist) und ansonsten z.B. fpspiegel[index] verwenden.

mit den sanitizern läuft es mir immer in einen Fehler. Auch sanitize::leak zeigt das Apple mit ui->textFertig() Probleme macht; auch nachdem ich die sprintf()-Anweisung dort durch "Festwerte" ersetzt hatte.

Du solltest versuchen, die Meldungen des Sanitizers zu verstehen und das Problem zu beseitigen. Das sind alles Bugs mit mehr oder minder schwerwiegenden Auswirkungen. Wenn das trotzdem "irgendwie" läuft, dann ist das nur Glück

Wenn du Probleme mit englissprachigen Meldungen haben solltest, dann versuchs mal mit ner Maschinenübersetzung (DeepL oder sowas) oder frag hier im Forum nochmal nach. Die Sanitizer-Fehler solltest du schon alle beheben, das erachte ich als Minimum. bei Compiler-Warnungen würd ich vielleicht die eine oder andere durchgehen lassen (z.B. harmlose wie dass man eine Varibale deklariert hat, die nirgendwo verwendet wird).

Liegt es vielleicht daran das ich kein "this->" in den Methoden verwende?

Ich denke nicht, solange du dadurch nicht versehentlich auf eine andere Variable zugreifst, welche eine Member-Variable überdeckt. Z.B. ein Funktionsparameter oder eine lokale Variable mit dem selben Namen. Ich verwende auch meist nur den Namen der Member-variablen und this->variable (oder alternativ Klasse::variable) wenn es nicht eindeutig ist.

EL europ

@Finnegan
OK, ich setzt mich mit den sanitizer-Meldungen auseinander, aber das ist ein Studuim für mich und dauert ein wenig. Hab schon, gegen meine Behauptung, wieder an der Colorinterface gearbeitet

Finnegan

@EL-europ sagte in Klassenfrage :

@Finnegan
OK, ich setzt mich mit den sanitizer-Meldungen auseinander, aber das ist ein Studuim für mich und dauert ein wenig. Hab schon, gegen meine Behauptung, wieder an der Colorinterface gearbeitet

Vielleicht ist es leichter, erstmal die Compiler-Warnungen zu beseitigen. Gut möglich, dass sich dann einige Sanitizer-Fehler in Wohlgefallen auflösen

Th69

Das sieht man doch schon am Code allein, daß dort noch viele Speicherlecks sind (da zu den ganzen malloc- bzw. realloc-Aufrufen die free-Aufrufe fehlen).

Und was versprichst du dir von dem NULL setzen?

// in Apple::calc()
thr_matrix = NULL;
thr_colors = NULL;
thr_matrix = (int**)malloc(xres * sizeof(int*));
thr_colors = (int**)malloc(xres * sizeof(int*));

Ich denke auch, daß sich der Sanitizer darauf bezieht und nicht auf die ui->textFertig(false);-Zeile vorher.

Bei Ersetzen durch einen vector<...> fallen diese Probleme dann aber weg.

PS: Wenn du viele Zugriffe auf ein vector<...>-Element hast, dann erzeuge eine Referenz-Variable, anstatt x-mal auf dasselbe Element zuzugreifen, wie z.B. in "_Colorinterface::drawElem(...)" mit elements.at(apos):

auto& element = elements.at(apos); // bzw. elements[apos]

(falls du noch nicht C++11 oder höher benutzt, mußt du den konkreten Typen anstatt auto angeben).

EL europ

@Th69
Den Speicher gebe ich im Destruktor von Apple wieder frei, in der Hoffnung das es so funktioniert; denke aber auch das in den "rohen arrays" das Problem liegt. Ich werd jetzt nochmals den fbpiegel in Userinterface in ein std::vector ändern und ohne at() darauf zugreifen. Die performance muss gut sein sonst wird das prog unbrauchbar. Das bedeuted: wenn die Performance nicht gut ist muss das Prog um die Rohen Arrays "herum" geschrieben werden weil sie bleiben müssen (so meine Intuition)

Th69

Du rufst calc() aber jedesmal in onMouseOver auf, so daß bei jedem Aufruf neuer Speicher alloziert wird (der Destruktor wird ja erst aufgerufen, wenn das gesamte _Apple-Objekt gelöscht wird).

Du brauchst auch nicht jedesmal wieder neuen Speicher anzufordern, da sich ja xres und yres währenddessen nicht ändert, d.h. bei Umstellung auf vector<...> reicht es diesen einmalig in der passenden Größe zu erzeugen (bzw. direkt in Init(...)).

Und bei der Umstellung erstelle auch nur einen eindimensionalen Vektor der Größe xres * yres, anstatt, wie bisher, 2 dimensional und benutze x + y * yres als Index.

Und bzgl. Performance kann man deinen Programmcode um einiges noch verbessern, da du viel zu viele unnötige Speicherallokationen und Berechnungen durchführst (und bei einem besseren Design, d.h. Codestruktur, könnte man einiges viel eleganter lösen).

Schlangenmensch

@EL-europ Wenn du Performance Probleme hast, wie kompolierst du denn?
Wenn du wirklich zum ausführen und nicht zum debuggen kompilierst, solltest du mit -O2 kompilieren.

EL europ

@Th69
Der Code hat ein einen Haufen Kosmetik nötig, das stimmt. Und jetzt verstehe ich auch den Fehler mit der Freigabe des Speichers, Ich "Nulle" ja nur den Zeiger . Also müsste ein free(th_matrix) anstatt eines th_matrix = NULL ausreichen um mal den groben Schnitzer zu beheben. Danke!
Ich rufe Apple->calc() nur auf wenn der entsprecndeButton in der Ui oder linke+rechte maustaste über dem Apple gedrückt wird (Zoom).
soviel Kosmetik ist es nun auch nicht. Die Bitverschieberei ist sauber denke ich, kann aber kein 24bpp testen (liegt wohl am raspios).

EL europ

@Finnegan
Danke erstma
In Apple.calc() der Threadfunktion, bekomme den Zeiger auf die lokale Variable x nicht in die Funktion übertragen. Wenn ich "static void thrFunc(int val)" deklariere zweifelt mir das der compiler an. Und wenn ich nicht (void*) sonder (int*) im aufruf übergebe compiliert er zwar ohne Fehler läuft aber direkt in einen speicher fehler.
Mit sanitizer::leak sagt er mir am ende der Apple->calc(), wenn er Userinterface->writeText wiederholt aufrufen soll, das der zeiger auf eine "Zeropage" weist. Ein std::cout<<"start funk\n" zu begin der writeText() zeigt das er sie nicht wieder aufruft. Mit sanitizer::adress bleibt er auch dort hängen mit der Meldung das das memcpy einen memorie-overlap hat. (Auch wenn fbspiegel als std::vector funkioniert). Jedoch habe ich bis jetzt keine Zweifel an der Display->drawSpiegel() Funktion welche mittels memcpy den spiegel in das Devicefile schreibt?
Auf git ist jetzt genau diesr Stand. Die Sanitizer stürzen das prog an der oben beschriebenen Stelle ab, ohne die Sanitizer scheinst es problemlos zu laufen

Th69

Du kannst nicht einfach ein per Doppelzeiger angelegtes Array (int** matrix) sequentiell kopieren:

matrix = (int**)malloc(paras.xres * sizeof(int*));
for(int i=0; i<paras.xres; i++)
	matrix[i] = (int*)malloc(paras.yres * sizeof(int));

vs.

memcpy(matrix, thr_matrix, paras.xres*paras.yres*sizeof(int));

Wenn das schon in deinem Original C-Code so drin war, dann war auch der schon nicht korrekt und führte zu Speicherüberschreibern.

Ich verstehe auch nicht, warum du überhaupt für die Threads die Matrix kopierst, anstatt gleich matrix zu beschreiben?!

Auf was für einem Prozessor (mit wievielen Kernen) führst du denn überhaupt den Code aus (damit sich die Threads überhaupt lohnen)?

PS: Hast du denn einen C++11 Compiler, denn dann kannst du std::thread benutzen?!

Aber ich denke, auf diese Art wirst du nicht wirklich weiterkommen, da dir anscheinend grundlegendes C bzw. C++ Wissen fehlt.

EL europ

@Th69
Ich allociere sequenziell den speicher weil ich mit [x][y] einfacher rechnen kann als mit [x*y] (im Kopf bei der Bitverschieberei), verstehe aber grade nicht wo das ein prob sein kann. Oder vermutest du Rechenfehler?
Zu den Threads: Ich brauche ja ein array von Threads für jeden Abschnitt der x-Achse, weil ich sie ja alle wieder joinen muss und zuvor nicht weiß wieviele Threads es pro Abschnitt sind. Ich verstehe grade ein weing die std::vektor, und hab keine Ahnung wie ich in c++ ein Array von Threads, denen ein Zeiger auf eine lokale Variable übergeben wird mit dem sie die getItter()-Funktion aufrufen und deren Rückgabewert in matrix und colormatrix schreiben, erstelle. Die loaken thr_matrix und thr_colors Arrays sind nur zum kopieren weil ich nicht weis wie ich die "eigentliche" Threadfunktion umgesetzt bekomme und meinen c-code benutzen muss. Aus den Funktionen Methoden zu machen scheint mir unnötig wenn ich sie eh nicht "richtig" nutzen kann. Die sind praktisch nur weil ich mit der *thrFunction() nicht auf Apple->matrix und Apple->colormatrix zugreifen kann.

Und die Threadfunktion befinde ich als gut weil sie ohne "Semaphoren" und "Raceconditions" auskommt von denen ich noch weniger Ahnung als vom ganzen Rest

Ich beutze eine RPI5 (4-kerner) und gcc 12.2

EL europ

@Th69 sagte in Klassenfrage :

...
Aber ich denke, auf diese Art wirst du nicht wirklich weiterkommen, da dir anscheinend grundlegendes C bzw. C++ Wissen fehlt.

Dein teile deines doch bitte mit mir zumindest bezüglich der Threads

Th69

@EL-europ sagte in Klassenfrage :

Ich allociere sequenziell den speicher ...

Tust du doch nicht, s. meinen geposteten Code von dir bzgl. malloc...

Ich be(n)utze eine RPI5 (4-kerner) und gcc 12.2

Und warum dann 13 Threads, wenn du eh nur 4 Kerne auslasten kannst (die anderen Threads müssen sich ja dann abwechseln - und dann kannst du gleich diese sequentiell bearbeiten)?!

Dein teile deines doch mit mir zumindest bezüglich des Threads

Ich meinte damit hauptsächlich dein Verständnis bzgl. der Speicherallokationen und -zugriffe.
Und ich hatte dir doch den Link zu C++ std::thread gegeben - dann mach dich mal darüber schlau...

EL europ

@Th69 sagte in Klassenfrage :

@EL-europ sagte in Klassenfrage :

...
Und warum dann 13 Threads, wenn du eh nur 4 Kerne auslasten kannst (die anderen Threads müssen sich ja dann abwechseln - und dann kannst du gleich diese sequentiell bearbeiten)?!

Das regelt die Systembibliothek

EL europ

@Th69 sagte in Klassenfrage :

@EL-europ sagte in Klassenfrage :

...
Ich meinte damit hauptsächlich dein Verständnis bzgl. der Speicherallokationen und -zugriffe.
Und ich hatte dir doch den Link zu C++ std::thread gegeben - dann mach dich mal darüber schlau...

Das ich in c++ rohe c-array benötige ist vielleicht schwach, aber wo liegt der Fehler?

Schlangenmensch

Ich habe gerade mal in dein Makefile geguckt. Du scheint immer noch keine Optimierungsflags zu nutzen. Das solltest du immer machen, z.b. -O2: https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html
Das ist viel wichtiger, als zu versuchen manuell mit mehreren Threads irgendwie die Performance zu optimieren.

memcpy kopiert einen zusammenhängenden Speicherblock, deine Matrix ist aber kein zusammenhängender Block, sondern ein Array, in dem jeder Eintrag auf einen anderen Block zeigt.

Hier im Forum gibt es irgendwo einen Thread, in dem sehr gute erklärt wird, wie man eine Matrix Klasse in C++ programmieren würde, damit man (x, y) Zugriff hat, aber trotzdem den Speicher direkt im Block vorliegen hat, was auch aus Performance Gründen Vorteile hat.

Wenn du den Malloc Teil weglassen würdest, und zum Beispiel einfach mit std::vector<std::vector<int>> arbeiten würdest, wären es immer noch kein zusammenhängender Speicherblocke und es ging schöner, aber du bräuchtest kein malloc kein free und einzelne Komponenten kopieren würde einfach so funktionieren, wie du es erwarten würdest.

EL europ

@Schlangenmensch sagte in Klassenfrage :

Ich habe gerade mal in dein Makefile geguckt. Du scheint immer noch keine Optimierungsflags zu nutzen. Das solltest du immer machen, z.b. -O2: https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html
Das ist viel wichtiger, als zu versuchen manuell mit mehreren Threads irgendwie die Performance zu optimieren.

memcpy kopiert einen zusammenhängenden Speicherblock, deine Matrix ist aber kein zusammenhängender Block, sondern ein Array, in dem jeder Eintrag auf einen anderen Block zeigt.

Hier im Forum gibt es irgendwo einen Thread, in dem sehr gute erklärt wird, wie man eine Matrix Klasse in C++ programmieren würde, damit man (x, y) Zugriff hat, aber trotzdem den Speicher direkt im Block vorliegen hat, was auch aus Performance Gründen Vorteile hat.

Wenn du den Malloc Teil weglassen würdest, und zum Beispiel einfach mit std::vector<std::vector<int>> arbeiten würdest, wären es immer noch kein zusammenhängender Speicherblocke und es ging schöner, aber du bräuchtest kein malloc kein free und einzelne Komponenten kopieren würde einfach so funktionieren, wie du es erwarten würdest.

Die colormatrix schreibe ich ja auch entsprechend ihres Aufbaus in den fbspiegel den ich dann mit memcpy ins Devicefile kopiere. Da liegt der Fehler nicht und es funktioniert ja ohne die Sanitizer bis auf die Meldung, das er die Breite des Wertes einer arithmetischen Operation nicht kennt, in der Thread-Funktion.

Quiche Lorraine

@EL-europ sagte in Klassenfrage :

Das ich in c++ rohe c-array benötige ist vielleicht schwach, aber wo liegt der Fehler?

Sorry, aber das Kernproblem liegt daran dass du auf Ratschläge anderer nicht hörst und vor allen Dingen du dich nicht weiterbildest und Sachen prüfst.

Sehr viele Sachen in der Informatik lassen sich beweisen. So auch die Aussage "Du kannst nicht einfach ein per Doppelzeiger angelegtes Array (int** matrix) sequentiell kopieren"

Eine kleines Prüfprogramm ergäbe dann folgendes:

#include <stdlib.h>
#include <stdio.h>

// Testprogramm
int main() {
	int xres = 5;
	int yres = 5;
	int **matrix = (int**)malloc(xres * sizeof(int*)); 
	for (int i = 0; i < xres; i++)
		matrix[i] = (int*)malloc(yres * sizeof(int));

	int Count = 0;

	for (int x = 0; x < xres; x++)
	{
		for (int y = 0; y < yres; y++)
		{
			matrix[x][y] = Count++;
		}
	}

	// memcpy greift sequenziell zu, also gebe ich diese mal sequenziell aus
	int* P = matrix;
	//int* P = *matrix;

	for (int i = 0; i < xres * yres; i++)
	{
		printf("%i ", *P);
		if (i % xres == 0 && i != 0)
			printf("\n");
		P++;
	}
	return 0;
}