STL vector mit eigener Klasse

l'abra d'or

Du brauchst Kopierkonstruktor und op=. Dein "double *m_data;" muss elementweise Kompiert werden. Dein jetziger Ansatz sorgt dafür, dass alle Images den selben m_data-Zeiger haben. Wird ein Objekt zerstört, gehen alle m_datas hopps.
Du könntest dir den Stress schenken, und kein Array hernehemen, sondern mit std::vector arbeiten, da hast du automatisch eine ordentliche Kopie.

MrPink

Danke für die schnelle Antwort - die Umwandlung in vector hat das Problem gelöst. Meine Klasse sieht nun wie folgt aus:

class Image
{
public:
	vector<double> m_data;
	int m_rows, m_columns;

	Image(int rows, int columns)
	{
		m_data = vector<double>(rows * columns,0);
		m_rows=rows;
		m_columns=columns;
	}

	inline double Get(int x, int y)
	{
		{
			int k = x + y * m_rows;
			return m_data[k];
		}

		return 0;
	}
	inline void Set(int x, int y, double val)
	{
		int k=x + y * m_rows;
		m_data[k]=val;
	}

};

Um nochmal die Fehlerursache zu klären - damit ich auch etwas daraus lerne Der Fehler ist entstanden, weil ich einen Vektor mit der Klasse Image zurückgebe und für diesen Vorgang wird ein Kopierkonstruktor und das Überladen des Zuweisungsoperators benötigt?
Der Fehler ist in der Zeile aufgetreten, wo ich dem Vector ein neues Image hinzufüge und für das Einfügen wird auch ein Kopierkonstruktor benötigt, richtig?

l'abra d'or

Nein. kleines Beispiel:

class Image {
  double* data;
public:
  Image() : data(new double[10]) {}
  ~Image() { delete[] data; }
};

int main() {
  Image img;
  { // neuer Scope
    Image img2 = img; // 1)
  } // 2)
  // 3)
}

img2 ist eine Kopie von img. Du hast keine eigene Kopier-Logik eingebaut, deshalb stellt der Compiler selber eine zur Verfügung. Zeiger können auch kopiert werden. Dabei wird aber nur die Adresse (!) kopiert. Die Adresse ist der Value eines Zeigers. Wenn alles glatt läuft, liegt an der Adresse im Speicher ein gültiges Objekt, der Zeiger zeigt also auf dieses Objekt.
Es gilt jetzt: img.data == img2.data -> beide zeigen auf das selbe Objekt!
Der Scope wird verlassen. Dein img2 wird hier automatisch zerstört. Das bedeutet der Destruktor wird aufgerufen. Dies wiederum heißt, img2.data wird zerstört. Das heißt aber auch: img.data ist jetzt kaputt!
Ab hier hast du Probleme, wenn du auf img.data zugreifst: undefiniertes Verhalten bei der Dereferenzierung.

Hoffe das ist jetzt ein wenig klarer.

Nexus

Der Fehler ist aufgetreten, weil push_back() und die Kopie des Vektors zur Kopie von Image -Objekten führt. Diese Kopie funktioniert aber ohne selbstdefinierten Kopierkonstruktor nicht wie gewünscht (analog Zuweisungen), weil alles 1:1 kopiert wird. Bei Zeigern willst du aber nicht nur den Wert, sondern auch den referenzierten Speicherbereich kopieren.

Benutze im Übrigen die Konstruktor-Initialisierungsliste. Dann erstellst du nämlich nicht Objekte, die du gleich wieder überschreibst. Statt

Image(int rows, int columns)
{
    m_data = vector<double>(rows * columns,0);
    m_rows=rows;
    m_columns=columns;
}

nimmst du

Image(int rows, int columns)
: m_data(rows * columns, 0)
, m_rows(rows)
, m_columns(colums)
{
}

MrPink

Danke für die Tipps und Erklärungen. Ich denke ich habe das mit dem Copyconstructor nun verstanden. Mein Programm läuft nun und auch die Initialisierung über die Initialisierungsliste habe ich angepasst. Ich habe allerdings scheinbar ein Speicherleck in meiner Anwendung. Beim ersten Ausführen braucht mein Programm 3.57 sec, bei der Ausführung beim zweiten mal 30 sec und beim dritten mal stürzt der Rechner ab. Ein Blick auf die Speicherauslastung nach der zweiten Ausführung zeigt, dass mein ganzer Speicher(8GB) belegt ist

Es werden zwei Bilder an mein Programm und zwei Arrays mit ca 500 Punkten übergeben. Daraus erstelle ich 1000 ROIs, die eine Größe von 21x21 haben. 1000*21*21*8(double) =3.528 MB -> sollte eigentlich kein Problem sein. Außer den oben dargestellten Funktionen verwende ich noch einige Funktionen, die Berechnungen auf den ROIs durchführen. Alle haben ungefähr den folgenden Aufbau(nur die Berechnung ist anders). Hier ein Beispiel für die Multiplikation zweier Bilder:

Image* mult(Image *im1,Image *im2)
{
    Image *im_mult = new Image(im1->m_rows,im1->m_columns);
    for(int i=0;i<im1->m_rows;i++)
    {
        for(int j=0;j<im1->m_columns;j++)
        {
            im_mult->Set(i,j,im1->Get(i,j)*im2->Get(i,j));
        }
    }
    return im_mult;
}

Ich erzeuge hier ja ein Bild mit new, dass ich eigentlich mit destroy wieder entfernen müsste. Wird das mit new erzeugte Bild am Ende der Funktion automatisch zerstört, so wie im Skope Beispiel l'abra d'or?
Eigentlich nicht, oder?, denn sonst würde ich ja kein gültigen Pointer zurückerhalten.
Bleibt das Bild auch nach Ablauf des Programms im Speicher und verursacht so mein Speicherleck? Danke für jeden Tipp.

Nexus

Speicher, den du mit new anforderst, musst du wieder mit delete freigeben.

Du könntest aber auch Smart-Pointer verwenden, die geben dir den Speicher von selbst frei. Zum Beispiel std::auto_ptr :

// Parameter als Zeiger auf const, da du nur lesend zugreifst!
std::auto_ptr<Image> mult(const Image *im1,const Image *im2)
{
    std::auto_ptr<Image> im_mult( new Image(im1->m_rows,im1->m_columns) );
    for(int i=0;i<im1->m_rows;i++)
    {
        for(int j=0;j<im1->m_columns;j++)
        {
            im_mult->Set(i,j,im1->Get(i,j)*im2->Get(i,j));
        }
    }
    return im_mult;
}

Der Aufrufer speichert auch einen auto_ptr . Dieser Zeiger transferiert bei Kopien seinen Besitz, d.h. das Bild wird quasi verschoben. Dereferenzierung mit * und -> funktioniert wie bei normalen Zeigern. Im Destruktor von std::auto_ptr wird der Speicher automatisch freigegeben.

MrPink

Nochmals Danke für die Antwort. Ich hatte inzwischend en Code zu:

Image mult(Image *im1,Image *im2)
{
	Image im_mult(im1->m_rows,im1->m_columns);
	for(int i=0;i<im1->m_rows;i++)
	{
		for(int j=0;j<im1->m_columns;j++)
		{
			im_mult.Set(i,j,im1->Get(i,j)*im2->Get(i,j));
		}
	}
	return im_mult;
}

geändert und das Problem des Speicherlecks hatte sich gelöst. Hat die Verwendung von Smartpointern Vorteile gegenüber meiner Umsetzung?

Generell ist es wünschenswert, dass der Code später schnell läuft, da das der Hauptgrund war, weshalb ich den Algorithmus auf C++ umschreibe. Momentan liegt der Rechnzeit Gewinn schon bei Faktor 0.5, ohne spezielle Optimierung. Es wäre aber wünschenswert noch deutlich schneller zu werden. Da ich kein C++ Experte bin ;-), bin ich für Tipps, was die Rechenzeit angeht dankbar.

Gruß und bis morgen

Nexus

MrPink schrieb:

Hat die Verwendung von Smartpointern Vorteile gegenüber meiner Umsetzung?

Prinzipiell führt eine Funktionsdeklaration

Image mult(const Image* im1, const Image* im2) // const nehmen! s.o.

dazu, dass ein Image bei der Rückgabe kopiert wird. Wenn du eine Initialisierung hast, die so aussieht:

Image c = mult(&a, &b);

kann es dumm laufen, und du hast noch eine Kopie des temporären Objekts nach c . Dem Compiler ist erlaubt, diese unnötigen Kopien wegzuoptimieren (z.B. mit RVO). MSVC++ macht das relativ gut. Aber zur Sicherheit könntest du auch die Funktion ändern und einen Output-Parameter hinzufügen, in dem das Resultat gespeichert wird:

void mult(const Image* im1, const Image* im2, Image* result);

Der Aufruf wäre dann halt etwas weniger schön, aber du hast sicher keine Kopie.

Image c;
mult(&a, &b, &c);

Ausserdem könntest du etwas cache-freundlicher programmieren, indem du die Schleifen vertauschst. Jetzt ändert sich in der inneren Schleife dauernd y , was in Image::Set() jeweils Sprünge beim Index verursacht. Eine Möglichkeit wäre auch, in der Image -Klasse ein lineares Layout nach aussen zu geben, sodass nur eine Schleife notwendig ist und all die Index-Umrechnungen wegfallen. Zumindest für die Fälle, in denen du ohnehin auf alle Pixel zugreifst.

Und generell zu C++:

Mach die Funktionen nicht präventiv inline , sondern definiere sie in separaten .cpp-Dateien. Du musst sonst nur dauernd Code neu kompilieren, schneller wird die Ausführung kaum (erst recht wegen Link Time Code Generation).
Beachte Const-Correctness. Referenz- und Zeiger-Parameter, deren Objekt nicht modifiziert wird, sollen auf const verweisen. Memberfunktionen wie Get() , die nichts an der Instanz ändern, kannst du ebenfalls const -qualifizieren.

MrPink

Hallo Nexus,

danke für die ausführliche Hilfe. Die Funktion läuft nun. Die Schleifen habe ich vertauscht um Sprünge zu vermeiden.
Bei Gelegenheit werde ich vielleicht noch die Klasse so anpassen, dass es nur eine Schleife gibt. Das der Rückgabewert in Form eines Pointers als Parameter übergeben wird habe ich in OpenCV gesehen - danke, jetzt weiß ich auch warum. :-). Mit const hast Du recht, ich werde das mal anpassen - kann vermutlich wirklich später Zeit beim debuggen sparen.

Das mit inline habe ich nicht verstanden: Wenn ich etwas am Code ändere, muss ich doch sowieso neu kompilieren und spart es nicht Zeit, wenn ich einen call oder jmp weglasse? Ich vermute es hat etwas mit "Link Time Code Generation" zu tun. Google hat mir gerade gesagt, dass bei VC automatisch viele Funktionen inline sind - ich nehme an, dass ist der Grund warum es kaum zeit spart, oder? Wenn ich das aber nun mit gcc kompiliere, wäre inline wieder sinnvoll?

Heißt cache-freundlicher, keine Sprünge im Datensegment zu machen? Muss nicht so oderso eine neue Adresse auf den Adressbus gelegt werden, egal ob das nun die nächste Adresse ist, oder eine weiter entfernte?

Vielen Dank!

Nexus

MrPink schrieb:

Das mit inline habe ich nicht verstanden: Wenn ich etwas am Code ändere, muss ich doch sowieso neu kompilieren und spart es nicht Zeit, wenn ich einen call oder jmp weglasse?

Nicht zwingend. Siehe Inline functions und Inline expansion.

MrPink schrieb:

Heißt cache-freundlicher, keine Sprünge im Datensegment zu machen? Muss nicht so oderso eine neue Adresse auf den Adressbus gelegt werden, egal ob das nun die nächste Adresse ist, oder eine weiter entfernte?

Meist werden zusammenhängende Speicherblöcke vom RAM in den Cache geladen. Wenn du nun aufeinanderfolgende Adressen nacheinander benötigst, wird ein solcher Speicherblock viel besser ausgenutzt und kann freigegeben werden, sobald du dessen letzte Adresse passiert hast. Bei "Sprüngen" im Speicher benötigst du vielleicht von jedem Block nur ein paar Bytes, bevor der nächste Block drankommt. Tendenziell besteht also eher die Gefahr von Cache-Misses (Cache zu klein für alle Blöcke, einige müssen freigeben und später wieder geladen werden).