CODEWITHSUNDEEP
c++performancealgorithmmemory
Cahit Burak Küçüksütcü
Cahit Burak Küçüksütcü

Reputation: 587

Performance Boosting C++

Here is my code below.

Point3* LASReader::GetPoint(int index)
{
    if (m_index == m_header.NPointRecords)
        return 0;

stream.seekg(GetOffset(index));
m_index++;

LASPOINTF1 p0;
LASPOINTF1 p1;
LASPOINTF2 p2;
LASPOINTF3 p3;
LASPOINTF4 p4;
LASPOINTF5 p5;
Vector3 position;

// Get the point values
switch (m_header.PointDataFormat)
{
case 0:
    stream.read((char *)&p0, sizeof(LASPOINTF0));
    position = Vector3(p0.X, p0.Y, p0.Z);
    break;
case 1:
    stream.read((char *)&p1, sizeof(LASPOINTF1));
    position = Vector3(p1.X, p1.Y, p1.Z);
    break;
case 2:
    stream.read((char *)&p2, sizeof(LASPOINTF2));
    position = Vector3(p2.X, p2.Y, p2.Z);
    break;
case 3:
    stream.read((char *)&p3, sizeof(LASPOINTF3));
    position = Vector3(p3.X, p3.Y, p3.Z);
    break;
case 4:
    stream.read((char *)&p4, sizeof(LASPOINTF4));
    position = Vector3(p4.X, p4.Y, p4.Z);
    break;
case 5:
    stream.read((char *)&p5, sizeof(LASPOINTF5));
    position = Vector3(p5.X, p5.Y, p5.Z);
    break;
}


// Calculate the real coordinates of the point
position = position * *m_scale + *m_offset;

return new Point3(position.GetX(), position.GetY(), position.GetZ());
}

// Main

int main()
{
    VertexPosColF* pcmVertices = new VertexPosColF[reader->GetHeader().NPointRecords];
    while (!reader->AllPointsRead())
{
    pcmVertices[i].Position = reader->GetPoint()->GetPosition()->GetD3DXVECTOR3();
    pcmVertices[i].Color = D3DXVECTOR4(1.0f, 1.0f, 1.0f, 1.0f);
    i++;
}
}

And here is my problem. I'm reading a file using a method like GetFromFile() (it is not enough I know but it is a sample only), maybe I'm calling it milion times so the if else conditions running million times. I want it called one time before the method called. how can i handle the different types? The switch case block must not be called million times I think.

My second problem is when I called GetFromFile method million times there is a lot of memory allocation about 4gbs. In my original code A, B,... structures' total size is nearly 200-300 byte and when I called the GetFromFile method 20.000.000 times I can see clearly there is 5GBs of memory allocation. Why? When the method finishes, are structures A a, and B b mustn't be released from memory? They are not pointers so I cannot delete manually.

It is a las file contains point cloud which rendered using DirectX. edit: Updated code and explanation

Upvotes: 1

Views: 170

Answers (3)

Tony Delroy
Tony Delroy

Reputation: 106096

For a start, in...

pcmVertices[i].Position = reader->GetPoint()->GetPosition()->GetD3DXVECTOR3();

...you call GetPoint() which returns a newly dynamically allocated object, but you keep no reference to the pointer with which to delete it. If you say change from...

Point3* LASReader::GetPoint(int index)

...to either of...

Point3 LASReader::GetPoint(int index)  // ALSO remove "new" from GetPoint's return

std::unique_ptr<Point3> LASReader::GetPoint(int index)

...the memory deallocation will take care of itself relatively efficiently. Unless GetD3DXVECTOR3 is returning a reference or pointer to anything inside the Point3 object... if that's the case, then you better keep the objects hanging around - saving the pointers from GetPoint somewhere first.

It would also be more typical C++ to change pcmVertices to...

std::vector<VertexPosColF> pcmVertices(reader->GetHeader().NPointRecords);

...which ensures the VertexPosCoLF destructors run when pcmVertices goes out of scope, then initialise as in...

for (int i = 0; !reader->AllPointsRead(); ++i)
{
    VertexPosColF v;
    v.Position = reader->GetPoint()->GetPosition()->GetD3DXVECTOR3();
    v.Color = D3DXVECTOR4(1.0f, 1.0f, 1.0f, 1.0f);
    pcmVertices.push_back(v);
}

Upvotes: 2

alexbuisson
alexbuisson

Reputation: 8469

If you can put your method as member of an object you could do something like that (pseudo code)

 class PLoaderInterface 
 {
      virtual P GetFromFile();
 }

Create 2 class LoaderA and LoaderB based on that interface

in the constructor of LoaderA and LoaderB preload everything in array of P(struct P seem equal to A) and in array of B

Now create a kind of factory

   PLoaderIntetface GetLoader(string filename);

where depending of your file type you allocate on type or the other !

Upvotes: 1

petersohn
petersohn

Reputation: 11730

I don't know if you can avoid the if statement, but if that's just a checking of a bool variable, then it doesn't have much performance impact. You may improve performance somewhat by declaring a and b only in the scope where it is required.

P GetFromFile()
{
   P p;

   if(file_is_A_formatted) {
      A a;
      stream.read((char*)&a, sizeof(A));
      p.x = a.x;
      p.y = a.y;
   }
   else {
      B b;
      stream.read((char*)&b, sizeof(B));
      p.x = b.x;
      p.y = b.y + b.z;
   }

  return p;
}

If you have memory leaks though, that's somewhere else in your code because this code shouldn't leak. Of course your code contains a lot of syntactic errors, but I guess that it's just something you typed in here.

Upvotes: 0

Related Questions