[RESOLVED] Managing pointers with STL map

Exterminator questions:
1. N/A
2. I don't know how to check for an STL version number. Whatever I am using was installed before I was hired.
3. I don't know what the array form of new is, so that must be a no.
4. No copy constructor. No pointer members. Comment on = overload below.
5. No child classes.
6. See below.
7. Called just once by the destructor for the manager class.
8. No passing by value.

My initial intent with this post was to get other developers to try it with their debugger to see if the results were the same. Now that I am back in the offic, I began running the same code on the computer of my coworkers, who also had VS 6.0. There was no crash at all! Very suspicious. I rebuilt every project in my workspace, and now it runs as expected. I guess the problem was simply with one of the compiled files.

Paul you're right, it doesn't compile. I was too hasty when trying to copy/paste my cpp file and h file together in the one post and accidentally left off my return *this. Thank you for catching it.

Thank you Paul for your information about the default = operator. I didn't realize it would copy all the variables for me. I'm sure there are other mini classes where I could apply the default = operator.

Paul you have given me much to consider... I may even have to start a new thread (only after first searching older post of course:). Yes I am going by my instincts, not an actual profilling tool. It's no doubt due to my lack of experience, but I just assumed that using variables by reference, instead of by value would make my functions perform faster. Same for the String classes. I replaced all functions that returned String as a void function that had as one of it's arguments a String& strOut. Most of my 'speed optimizations' resulted from google searches on code performance, instead of profilers. I certainly hope I haven't just made things worse as you suggest.

I'm interested in what you mentioned about the "compiler's own optimization strategy". I'll have to take some time and google the topic for a while. Thank you to all who responded to the post. Sorry for wasting your time, but thankfully I learned some new things that I didn't know before so this has been a successful post afterall :)

Quote:

---

Originally Posted by nice_guy_mel

I replaced all functions that returned String as a void function that had as one of it's arguments a String& strOut.

---

Most modern C++ compilers do this optimization automatically. It's called Named Return Value Optimization or NRVO, and it is one (maybe the only) optimization that is described in the ANSI C++ standard document.

http://blogs.msdn.com/aymans/archive...13/480699.aspx

So if you had one of these compilers, you didn't have to change your code. The latest versions of Visual C++ has this optimization, as does gcc. Basically, if a modern compiler doesn't do this optimization, IMO it can be considered as "lacking" and is a minus.

Quote:

---

I'm interested in what you mentioned about the "compiler's own optimization strategy".

---

If you use well known constructs, the compiler will recognize them and attempt to optimize them. For example, if you use std::sort with a predicate instead of writing your own sort (or using qsort), the compiler can optimize the code by doing "inlining".

If you try and be too elaborate with esoteric pointer usage, the code will contain too many pointers and pointer aliases. Then the compiler cannot do anything with the code in terms of optimizations, since none of the optimization techniques are guaranteed to produce the same results when the code is unoptimized. That is basically what I mean when I stated about "compiler optimization strategy".

Regards,

Paul McKenzie

Quote:

---

Originally Posted by nice_guy_mel

Sorry for wasting your time, but thankfully I learned some new things that I didn't know before so this has been a successful post afterall :)

---

Well I don't think it's wasted time if you learned something from it.

So honestly, I would stop all code changes right now and spend a few hours on profiling. You may even have to change the algorithms that are used in some parts to make the code faster. The "cosmetic" changes rarely affect speed much unless it's something really stupid like a for (i = 0; i < strlen(s); ++i) loop.

Looking at your code, and ignoring the error, I would say switching to pointers may well be a pessimisation, since you've had to add lots of calls to "new". You may well find that this code is faster:

Code:

---

typedef map<int, CInfo> MAP_ONE;
typedef map<int, MAP_ONE> MAP_TWO;

MAP_TWO m_map_Two;
MAP_ONE& map_One1 = m_map_Two[5];
map_One1[1]; //default constructs
map_One1[2]; //default constructs

MAP_ONE& map_One2 = m_map_Two[6];
map_One2[3];
map_One2[4];

---

Note that the end aim is not to reduce the number of object copies, but to speed the code up. Generally, the only reason reducing object copying improves speed is because it reduces the number of memory allocations. However, your CInfo class doesn't make any allocations, so dynamically allocating instances to avoid copying may well be a pessimization.

I have spotted the bug immediately. You are deleting the "second" in all of your map nodes but these "second" pointers are not necessarily unique, i.e. they don't appear in only one place. In fact you place one pointer in there twice:

Code:

---

m_map_Two[5] = p_map_One;
p_map_One = new MAP_ONE();
(*p_map_One)[3] = new CInfo();
(*p_map_One)[4] = new CInfo();
m_map_Two[6] = p_map_One;

---

so you are deleting p_map_One twice, once for m_map_Two[5] and once for m_map_Two[6].

Now if these really are the same map, and I will assume they are, this is a perfect case for using shared_ptr. shared_ptr will be reasonably efficient in that it won't have to copy the maps, it would just copy the pointer and up the reference count.

And it would take away from you having to handle the issues of deletion.

At the other level where the maps are unique, you might possibly these use value rather than pointer.

No, he is not. He is using the same pointer to refer to two different map_one objects but not deleting the same one twice. Here (see bolded statements):

Code:

---

typedef map<int, CInfo*> MAP_ONE;
typedef map<int, MAP_ONE*> MAP_TWO;

MAP_TWO m_map_Two;
MAP_ONE* p_map_One = new MAP_ONE();
(*p_map_One)[1] = new CInfo();
(*p_map_One)[2] = new CInfo();
m_map_Two[5] = p_map_One;
p_map_One = new MAP_ONE();
(*p_map_One)[3] = new CInfo();
(*p_map_One)[4] = new CInfo();
m_map_Two[6] = p_map_One;

---

Yes you're right, I was looking too quickly. My next criticism is that I think map is the wrong collection to use. As the key is int and the indices are low and bunched, I'd use vector.

In this case vector< vector < CInfo * > >

may be the best option (although I'd prefer vector< vector< shared_ptr< CInfo > > )

You want to use a pointer here for CInfo because it might be NULL if there is no entry.

Now we'll modify this function:

Code:

---

std::vector< std::vector< CInfo * > > sectionTable; // probably member of CAreaMgr

void CAreaMgr::AddSection(int nSection)
{
  int nArea = gn_Count++; //gn_Count is a static variable.

  if ( sectionTable.size() < nSection + 1 )
  {
      sectionTable.resize( nSection + 1 );
  }

  if ( sectionTable[ nSection ].size() < nArea + 1 )
  {
      sectionTable[ nSection ].resize( nArea + 1  );
  }
 
  CInfo* pInfo = new CInfo;
  pInfo->Init(); // funny I don't see an Init() function in your definition of CInfo

  sectionTable[ nSection ] [ nArea ] = pInfo;
}

---

There seems to still be something flawed here. If nArea is a counter then there is one single unique pInfo for each nArea so why not just use vector here.

At this point I'm also going to do away with using new and will just use instances of CInfo. As it is just a collection of 4 integers and has no problems with deep copy (i.e. no pointers to other classes), it is assumed cheap to copy. (Note: it is even cheaper to copy than a std::string).

Code:

---

// these two are both members of CAreaMgr

std::multimap< int, std::vector<CInfo>::size_type > sectionMap;
std::vector< CInfo > infoTable;

void CAreaMgr::AddSection(int nSection)
{
//  int nArea = gn_Count++; //gn_Count is a static variable.
// May I assume there is only one area manager?

    infoTable.push_back( CInfo() );
    sectionMap.insert( std::make_pair( nSection, infoTable.size() - 1 ) );

 // wow! down to just 2 lines
}

---

sectionMap is a multi-map from section to all the indices (or areas) that are in it.
Note that if you want to be able to trace the other way, either use std:: pair< CInfo, int > as your value type where int has the section in it, or extend CInfo to contain another int, that being the section that it is in (or write your own struct in place of std:: pair )

Well, vectors of vectors of objects really are a performance problem when they grow dynamically. And this is what is happening here, so I don't think this is a good solution at all. And one of the main problems may be that sometimes AddSection takes a really unreasonable time when it shouldn't, just because the outer vector needs to grow its reserve.

Looking back on this thread I realized I didn't tell you why I chose a map of maps as a container in the first place. What I was doing was tracking vessel identification numbers. When we receive a vessel message we are primarily concerned with two things. The first is what basestation the message came from, this is an integer used in the MAP_TWO. The second is the vessel id number, this is an integer used in the MAP_ONE. CInfo was in charge of holding the timestamp of when the message came in, along with a few other parameters. In a standard setup, we would only be working with 10 or less basestations, so MAP_TWO would rarely if ever consist of more then 10 MAP_ONE's. The problem was that we were getting close to 600 messages a second. Each message represents either a new instance of CInfo, or updating an existing CInfo object from one of the MAP_ONE's.
I thought it would speed things up a bit to store CInfo as a pointer instead of an object. I no longer get the crash when I use map->clear() because I always make sure to set mapItr->second = NULL after I delete the CInfo object.