Is this a good use of a private constructor?

Question

Trying to learn something new every day I'd be interested if the following is good or bad design.

I'm implementing a class A that caches objects of itself in a static private member variable std::map<> cache. The user of A should only have access to pointers to elements in the map, because a full copy of A is expensive and not needed. A new A is only created if it is not yet available in the map, as construction of A needs some heavy lifting. Ok, here's some code:

class B;

class A {
    public:
        static A* get_instance(const B & b, int x) {
            int hash = A::hash(b,x);
            map<int, A>::iterator found = cache.find(hash);
            if(found == cache.end())
                found = cache.insert(make_pair(hash, A(b,x))).first;
            return &(found->second);
        }
        static int hash(B & b, int x) { 
            // unique hash function for combination of b and x
        }
        // ...

    private:
        A(B & b, int x) : _b(b), _x(x) { 
            // do some heavy computation, store plenty of results 
            // in private members
        }
        static map<int, A> cache;
        B _b;
        int _x; // added, so A::hash() makes sense (instead of B::hash())
        // ...
 };

Is there anything that is wrong with the code above? Are there any pitfalls, do I miss memory management problems or anything else?

Thank you for your feedback!

Answer 1

The implementation is intended to only allow you to create items via get_instance(). You should ideally make your copy-constructor and assignment operator private.

It would not be thread-safe. You can use the following instead:

const boost::once_flag BOOST_ONCE_INIT_CONST = BOOST_ONCE_INIT;

struct AControl
{
  boost::once_flag onceFlag;
  shared_ptr<A> aInst;

  void create( const B&b, int x )
  {
      aInst.reset( new A(b, x) );
  }

  AControl() : onceFlag( BOOST_ONCE_INIT_CONST )
  {
  }

  A& get( const B&b, int x )
  {
     boost::call_once( onceFlag, bind( &AOnceControl::create, this, b, x ) );
     return *aInst;
  }
};

Change the map to map

Have a mutex and use it thus:

AControl * ctrl;
{
  mutex::scoped_lock lock(mtx);
  ctrl = &cache[hash];
}
return ctrl->get(b,x);

Ideally only get_instance() will be static in your class. Everything else is private implementation detail and goes into the compilation unit of your class, including AControl.

Note that you could do this a lot simpler by just locking through the entire process of looking up in the map and creating but then you are locking for longer whilst you do the long construction process. As it is this implements record-level locking once you have inserted the item. A later thread may find the item uninitialised but the boost::once logic will ensure it is created exactly once.

Answer 2

Any time you use globals (in this case the static map) you have to worry about concurrency issues if this is used across multiple threads. For example, if two threads were trying to get a particular instance at once, they could both create an object resulting in duplicates. Even worse, if they both tried to update the map at the same time it could get corrupted. You'd have to use mutexes to control access to the container.

If it's single-threaded only then there's no issue until someone decides it needs to be made multi-threaded in the future.

Also as a style note, while names starting with underscore+lower case letter are technically legal, avoid any symbols starting with underscores will avoid possibly accidentally breaking the rules and getting weird behavior.

Answer 3

I think these are 3 separate things that you mix together inside A:

• the class A itself (what its intances are supposed to do).
• poolling of instances for cache purposes
• having such a static singlton pool for a certain type

I think they should be separate in the code, not all together inside A.

That means:

• write your class A without any consideration of how it should be allocated.

• write a generic module to perform pool cache of objects, along the lines of:

*

template< typename T > class PoolHashKey { ... };

template< typename T > class PoolCache  
{  
//data  
  private: std::map< .... > map_;  

//methods  
    public: template< typename B > PoolKey< T > get_instance( B b );  
    public: void release_instance( PoolKey< T > );  
    // notice that these aren't static function members  
};  

• create a singleton instance of PoolCache somewhere and use it:

*

PoolCache<A>& myAPool()  
{  
    static PoolCache<A> s;  
    return s;  
    //you should use some safe singleton idiom.  
}  

int main()  
{  
  B b;  
  PoolKey<A> const aKey( myAPool().get_instance( b );  
  A* const a( aKey.get() );  
  //...  
  myAPool().release_instance( aKey ); //not using it anymore
  /*or else the destructor of PoolKey<A> should probably do some reference count and let the pool know this instace isn't needed anymore*/
}