2

I create a class, which creates a new list of pointers to objects:

FeatureSet::FeatureSet() {
    this->featuresList = new list<HaarFeature*>;
    globalCounter = 0;
}

Now I would like to delete all object from the list, and then delete the pointer to the list. My code is:

FeatureSet::~FeatureSet() {
    for (list<HaarFeature*>::iterator it = featuresList->begin(); it !=     featuresList->end(); it++) {
        delete *it;
    }
    delete featuresList; // this take a long time (more than half a minute)
}

My question is what is the best method to solve this problem? Does my approach is correct? Unfortunately, currently the last operation (deleting a pointer to the list) takes about half minute. List has about 250000 objects.

Lightness Races in Orbit
  • 378,754
  • 76
  • 643
  • 1,055
Viper
  • 597
  • 1
  • 8
  • 24
  • 1
    Are you sure you need to store pointers to `HaarFeature`? How big is it? Is it expensive to create/copy? Storing `HaarFeature` by value may actually speed things up quite a bit. – Emile Cormier Mar 14 '12 at 19:54
  • 1
    Also, you shouldn't need to allocate a `std::list` on the heap. `std::list` already stores objects on the heap. Finally, are you sure that `std::list` is better than `std::vector` for your use? – Emile Cormier Mar 14 '12 at 19:58
  • I chose a list container because I iterate many times through all objects in sequential order. In my opinion vector is faster to clear, but slower during creation and accessing objects in sequential order. I created a pointer to the list, because I would like to ensure an access to objects, even when FeatureSet object will be destroyed. The one object of the HaarFeature class is small, but I'm not sure if storing whole objects instead of pointers to object increases speed of clearing list. – Viper Mar 15 '12 at 10:36
  • I don't understand: "*I created a pointer to the list, because I would like to ensure an access to objects, even when FeatureSet object will be destroyed.*". If FeatureSet is destroyed, what objects are there to access? – Emile Cormier Mar 15 '12 at 17:52
  • Sequential traversal will be faster with a `vector`, because that structure is more cache-friendly. For creation, a `vector` will require far fewer heap allocations (heap allocation is slow) than a `list`, because `vector` allocates in chunks. Use `vector::reserve` to make `vector` allocate the right amount of storage in advance and to avoid recopying its elements when it grows. Anyways, you shouldn't guess these things. Try and measure. – Emile Cormier Mar 15 '12 at 17:56
  • You are right - using vector is better and faster solution. I have posted results of my tests and answer to your question. Sorry that not here, but I'd like to also post some code. – Viper Mar 16 '12 at 14:11

4 Answers4

1

You really need to use a different data structure. Depending on your mutation patterns, you may be better off with containers like a deque or vector.

Puppy
  • 144,682
  • 38
  • 256
  • 465
1

Your approach is correct. And it seems like the appropriate way to go about doing what you're trying to do. But list deallocation is expensive. Maybe try a different data structure?

The following:

 for (; _Pnode != _Myhead; _Pnode = _Pnext)
 {  // delete an element
    _Pnext = _Nextnode(_Pnode);
     this->_Alnod.destroy(_Pnode);
     this->_Alnod.deallocate(_Pnode, 1);
 }

is where the code spends most of the time. It needs to iterate through all the nodes and remove them. There's really no way going around this for a list.

I suggest you use a std::vector, deallocation is much faster.

Luchian Grigore
  • 253,575
  • 64
  • 457
  • 625
  • You are right. Line "delete FeatureSet" calls list destructor and .clear() method. What's funny, deleting all objects from the list with iterator takes many less time, than deleting empty list structure (all nodes)... I'll try to use different container, but as I wrote above, I think that using a list container should be better in my case. BTW: I heard that STL library containers aren't designed optimally. E.g in C# deletion of 250000 objects (the same size) from the list takes less than one second. – Viper Mar 15 '12 at 10:44
  • @Viper but the overhead may just be moved to later on, when the garbage collector kicks in in C#. You *could* make a compromise, break your data in buckets and have a vector of lists or something similar. – Luchian Grigore Mar 15 '12 at 10:51
0

Given that you're using a list, yes, that's the way to go about deleting the sub-elements. Note that whatever container you use, if you're storing pointers, the operation of 'acquiring the next pointer and deleting it' is likely to take the same amount of time. It's certainly worth testing as an exercise.

You may get some savings in the deletion of the container (and it's contained nodes) itself. But the choice of parent container is more likely to be determined by it's usage elsewhere. For instance: If you're adding nodes to it piecemeal then a vector will have to reallocate and copy the whole block of pointers to maintain it's internal structure.

That said, I'd also agree with the point made above about dynamic allocation of the container itself. Might as well make it a concrete member variable. (Of course you still have to iterate across it and delete the node sub-contents. But that's more a function of convention than anything else.

Michael Wilson
  • 442
  • 1
  • 4
  • 11
0

OK. So I performed some research. I just have tested four solutions. Results are presented in the table below: (I can't put an image, please click the link)

Comparing an efficiency of list and vector containers

As you can see from the table, vector containing objects is the fastest solution. It is also true that storing pointers to objects is much slower than containing real objects.

SUMMARY:

  • Sequential access to objects aggregated in list is slow, probably because it requires "jumping" from pointer to pointer and particular objects don't have to be stored in consecutive memory cells
  • Sequential access to vector is fast, probably because objects are stored in linear, consecutive memory cells
  • Using iterator to access consecutive objects is faster than using indexes, what isn't surprising
  • Storing pointers instead of real objects is much slower, especially during allocation and deallocation of container (heap allocation)
  • Independently from storing object type, deallocation of list is much slower than deallocating of vector

Answering to question asked by @Emile Cormier - Why I would like to use pointer to container? This ensure me an access to vector/list elements even after deletion of parent class object. Please consider this part of code:

class A{
public:
A(){
    this->test = new vector<int>;
    for(int i = 0; i < 10; i++){
        test->push_back(i);
    }
}
~A(){
    cout << "object A is dead\n";
    test->clear(); // <--- COMMENTING this line allows to access to the vector's elements "after death" without copying
}
vector<int>* GetPointer(){
    return this->test;
}
private:
vector<int>* test;
};

class B{
public:
B(){
    for(int i = 0; i < 10; i++){
        test.push_back(i);
    }
}
~B(){
    cout << "object B is dead\n";
    test.clear();
}
vector<int> GetbyValue(){
    return test;
}
private:
vector<int> test;
};

cout << "\nCASE A\n";
A* instance = new A();
vector<int>* local = instance->GetPointer();
delete instance; //deletion before calling!!!
//Access is impossible, because clear method in destructor destroys original vector, connected with pointer
cout << "Printing A\n";
for(int i = 0; i < local->size(); i++){
    cout << (*local)[i] << "\n";
}
cout << "\nCASE B\n";
B* instance2 = new B();
vector<int> local2 = instance2->GetbyValue();
delete instance2; //deletion before calling!!!
//Access is still possible, because vector has been copied to local variable
cout << "Printing B\n";
for(int i = 0; i < local2.size(); i++){
    cout << (local2)[i] << "\n";
}
cout << "THE END\n";

When I don't use a pointer to vector (case B), I although can access objects from vector after deletion of "B" class object BUT in reality I use a local copy of returned vector. Using a pointer to vector (case A) allows me to get an access to vector objects even after deletion of "A" class object, when I don't call clear() method in destructor manually of course.

Finally - thanks everyone for help. I think that my problem has been solved.

Viper
  • 597
  • 1
  • 8
  • 24
  • 1
    I'm so glad that you took the time to test and measure. When it comes to performance, you shouldn't rely on intuition. :) – Emile Cormier Mar 16 '12 at 16:59
  • If you want your `vector` to be shared outside the class, without making copies, the current best practice is to use smart pointers. Smart pointers avoid problems with dangling pointers and memory leaks. If your compiler supports C++11, you can use `std::shared_ptr`. If not, there's `boost::shared_ptr` (http://www.boost.org/doc/libs/release/libs/smart_ptr/shared_ptr.htm). – Emile Cormier Mar 16 '12 at 17:05
  • Learning about smart pointers is an investment that will pay itself back at least ten-fold, because pointer-related bugs are just about the nastiest to resolve. See http://stackoverflow.com/questions/106508/what-is-a-smart-pointer-and-when-should-i-use-one, http://en.wikipedia.org/wiki/Smart_pointer to learn about smart pointers. – Emile Cormier Mar 16 '12 at 17:09
  • If your compiler doesn't have `std::shared_ptr`, it may have `std::tr1::shared_ptr`. – Emile Cormier Mar 16 '12 at 17:11