[boost] Singleton: review

From: Gennadiy Rozental (gennadiy.rozental_at_[hidden])
Date: 2005-05-16 03:32:52

Hi,

I know I am slightly late but I am quite busy lately.

Problem domain
============

Singleton is just a glorified global variable ( (c) Peter Dimov). Singleton 
is an old pattern. Used quite frequently. But... from my experience it's 
really bad idea to try to manage an order of destruction of global 
variables. Any responsible library designers should prefer explicit teardown 
procedure. There are many reasons for that from general to specific 
technical issues, i.e:
  * class destructors preferably shouldn't be doing anything smart
  * if teardown procedure is not trivial it's part of normal program flow 
and should reside within main() bounds
  * it doesn't seem to be ever portable to rely on order of global variables 
destruction in different libraries (dynamic or static)
  * it's unreasonable to expect all global variables to employ single 
manager facility within bound any reasonably big application
  * if any teardown procedure fails it's really non-trivial to insure proper 
destruction for the rest of globals (if possible at all), while reporting an 
error may be impossible/difficult

And so on. Taking attempts to manage destruction order out of the picture we 
have three known techniques to implement singleton:

  * don't use any objects at all. use free functions in some namespace with 
static object in implementation
namespace log {
    void print(...)
};
  * inline static T& log() { static T inst; return inst; }
  * inline static T& log() { static T* inst = 0; if(!inst) inst = new inst; 
return *inst; }

should cover majority of users needs. They do not need to be formalized in a 
form of the library but just could exist in a form of reusable idioms. 
Specific need could easily be achieved by little variations.

Design
=====

PBD is not about supplying implementation as template parameter and any kind 
of "super policies" are never a good idea IMO. They trying to bring 
simplicity of single template parameter, but does not really bring anything. 
In trivial cases (use all defaults) design with orthogonal polices is as 
simple. While any customization attempt immediately shows an advantage of 
later is more convenient and straightforward. For example if I want to use 
custom locking policy with submitted singleton I would write:

class A : singleton_ex<static_lifetime_ex<instant_creation,my_lock> > {...};

Here I not only need to mention default creation policy, but also strange 
beast static_lifetime_ex. And I only needed custom lock:

class A : singleton<lock<my_lock> > {}

-----------------

Inability of the author to clearly recognize orthogonal policies lead to 2 
subpolicies with "create" in their name: CreationPolicy and Creator. In 
addition some instances of a Creator take responsibilities of 
CreationPolicy. For example assert_creator clearly used to manage *when* to 
create not *how*. The same with throw_.. and null_... The rest of a 
"Creator"s are also a misplaced efforts. I do not see why would anyone would 
need to employ malloc or std::allocator to allocate memory for global 
variable. In most case it's done once per program lifetime. some contrived 
phoenix singletons with performance requirements seems to have contradicting 
design goals and I do not see a need for generic support. Most interesting 
is that among all of these "creators"/creation_policies I don't see a 
solution for singleton with non-default constructor.

-----------------

Singletons very frequently are the bottleneck of program performance. Any 
singleton solution couldn't afford significant performance overhead. 
Submitted design clearly leads to some rotationally inlined functions to be 
offlined (in addition to extra overhead). For the following trivial class

class A {
public:
    void foo() {++i; }
private:
    int i;
};

I noticed that

A* a = new A;

int n = ..;
while( n-- > 0)
  a->foo();

is about 300 times faster then (modify definition above to employ default 
singleton: class A : public basic_singleton<A>)

A::pointer a;

int n = ..;
while( n-- > 0)
  a->foo();

(used g++ 3.3.3 on windows)

-----------------

Author does admit that multithreading issues were not considered. IMO any 
singleton design that doesn't cover MT couldn't be accepted as generic. It's 
quite possible that it may need to be reworked completely to adopt MT 
issues. And piles of static variable used in implementation only confirm my 
suspicion.

Interface
======

* I do not see a need for these basic_singleton and singleton_ex. Single 
template singleton with default template parameters would suffice
* As I mention in design section interface with single "super policy" is 
unacceptable IMO
* A::pointer interface is not convenient. I do not want to know about any 
"pointer". It should be either A::instance() or even better simply A;
Most frequently I use this idiom (don't remember the name)
class A_t {
public:
    A_t& instance() {...}
};

namespace  {
A_t& A = A_t::instance();
}

Implementation
==========

The submitted solution dropped on unaware user dependency on a <list>, 
<map>. <new>, <memory>, <cstring>, <cstdlib>, shared_ptr, aligned_storage 
and many other things. In my experiment simple include <singleton.hpp> leads 
to 1M of preprocessed code, while preprocessed size of trivial_singleton I 
am using in Boost.Test is ~6k. This all is a consequence of incorrect design 
and non-optimal implementation (too many interdependencies). I did not delve 
deep enough to comment in more details. Also I noticed that even default 
lifetime policy require partial specialization. Isn't clear why it should be 
this way.

Tests
====

Tests are virtually absent. So there is nothing to comment here.

Documentation
===========

I guess it's not bad. But it's not my primary area of expertise.

As you may figured by now my vote is NO. A would actually vote no even to 
include this into review queue, but did not paid enough attention at the 
time.

Regards,

Gennadiy.

P.S. We really need go ahead with "mentor" idea. Also I would propose to 
enforce some "cool off" time (as 4 month since initial "query of interests 
point" is clearly not enough). May be it could enhance quality of 
submissions we invest time reviewing