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
"Gennadiy Rozental" wrote:
I know I am slightly late but I am quite busy lately.
Problem domain ============
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. Explicit teardowns would be source of coupling and something
You are much welcomed. Thanks for the time to do the review and all the details. pretty hard to maintain (for me, others may find it easy).
* it doesn't seem to be ever portable to rely on order of global variables destruction in different libraries (dynamic or static)
Singletons in DLLs are pretty hard problem and standard C++ doesn't seem to provide enough means to handle them. One solution considered is to use unique shared memory where named singletons get registered but this is still huge work ahead.
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> > {}
This is valid issue. Perhaps named template parameters technique could be used here.
Inability of the author to clearly recognize orthogonal policies lead to 2 subpolicies with "create" in their name: CreationPolicy and Creator. The name and concept may be reconsidered as suggested by Dan Rosen in other thead.
I do not see why would anyone would need to employ malloc or std::allocator to allocate memory for global variable. Quite often. If you do custom memory management with new/delete you often set it up after statics initialization (say when loading a DLL).
Either one needs to do nasty tricks with shared variables or use malloc/etc.
Most interesting is that among all of these "creators"/creation_policies I don't see a solution for singleton with non-default constructor.
Yes, this should be supported. It is possible to explicitly pass parameters to the constructor as: A:pointer ptr; ptr->create(111); but this is different idiom and different use case.
-----------------
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>)
(used g++ 3.3.3 on windows)
I tried this with Intel 7 plugged in VC6 IDE, Release mode, all settings default and the ratio was 8.7 (in favor of raw class A).
-----------------
Author does admit that multithreading issues were not considered. IMO any singleton design that doesn't cover MT couldn't be accepted as generic. Not every library in Boost is MT safe even when it could be useful. MT was left into next stage.
Interface ======
* 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;
Yes. Member function T::pointer instance() { T::pointer ptr; return ptr; } has been suggested. This would allow the A::instance() syntax. It is also possible to wrap the singleton class into monostate like class: struct AA { A::pointer ptr; void DoSomething() { ptr->DoSomething(); } }; I suggested it as example but shortage of time ... /Pavel
Problem domain ============
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. Explicit teardowns would be source of coupling and something pretty hard to maintain (for me, others may find it easy).
I do not see how it may be cause of any new coupling. It does add one function to the singleton interface. If you have several singletons in you library that have non-trivial teardown I usually add single library level teardown routine.
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> > {}
This is valid issue. Perhaps named template parameters technique could be used here.
Most importantly design shoulf be cleared from all these "super policies"
Inability of the author to clearly recognize orthogonal policies lead to 2 subpolicies with "create" in their name: CreationPolicy and Creator. The name and concept may be reconsidered as suggested by Dan Rosen in other thead.
As it follows from my review I do not see a need for creator at all.
I do not see why would anyone would need to employ malloc or std::allocator to allocate memory for global variable. Quite often. If you do custom memory management with new/delete
First of all you do NOT do custom memory management "quite frequently". I mean in general in C++ comminity. But even in such case I do not see a reason to apply it to singleton. Custom memory allocation is a performace enhancement tecnique (most cases). I do not see a need to speed up singleton construction. It happends once per process lifetime.
you often set it up after statics initialization (say when loading a DLL). Either one needs to do nasty tricks with shared variables or use malloc/etc.
You lost me here with "set it up after statics initialization".
Most interesting is that among all of these "creators"/creation_policies I don't see a solution for singleton with non-default constructor.
Yes, this should be supported.
It's actually should be one of the major "features" of generic singleton (would we ever need one).
is about 300 times faster then (modify definition above to employ default singleton: class A : public basic_singleton<A>)
(used g++ 3.3.3 on windows)
I tried this with Intel 7 plugged in VC6 IDE, Release mode, all settings default and the ratio was 8.7 (in favor of raw class A).
Well it obviosly depends on compiler and it's optimizer module. But even 2 times overhead in performace critical area may not be acceptable. And singleton prone to be such an area.
Author does admit that multithreading issues were not considered. IMO any singleton design that doesn't cover MT couldn't be accepted as generic. Not every library in Boost is MT safe even when it could be useful. MT was left into next stage.
You could not leave MT for the next stage of singleton design. Singletons have a long history dealing with MT issues topped with DL idiom.
I suggested it as example but shortage of time ...
I do not see why the hurry. IMO this submission simply is not ready to be reviewed (beyond the fact it's doing the wrong service for the programmers IMO) Gennadiy
Gennadiy Rozental wrote:
Author does admit that multithreading issues were not considered. IMO any singleton design that doesn't cover MT couldn't be accepted as generic.
Not every library in Boost is MT safe even when it could be useful. MT was left into next stage.
You could not leave MT for the next stage of singleton design. Singletons have a long history dealing with MT issues topped with DL idiom.
I agree with Gennadiy (in case he's right; I haven't looked at the implementation). A Singleton without careful design for MT is IMHO unacceptable, no matter how carefully designed it is in any other dimension. Singleton is a shared object, and it must address MT issues first and foremost. Andrei
Andrei Alexandrescu (See Website For Email) wrote:
Gennadiy Rozental wrote:
Author does admit that multithreading issues were not considered. IMO any singleton design that doesn't cover MT couldn't be accepted as generic.
Not every library in Boost is MT safe even when it could be useful. MT was left into next stage.
You could not leave MT for the next stage of singleton design. Singletons have a long history dealing with MT issues topped with DL idiom.
I agree with Gennadiy (in case he's right; I haven't looked at the implementation). A Singleton without careful design for MT is IMHO unacceptable, no matter how carefully designed it is in any other dimension. Singleton is a shared object, and it must address MT issues first and foremost.
FWIW, that's also my opinion. More often than not singletons are accessed by multiple threads. A singleton lib that's not designed for MT is therefore of little help in many applications. I planned to do a (late) review but given that MT wasn't considered in the initial design I'm not too motivated anymore. Also, - as mentioned by other people - what I find a little bothersome is that the library apparently had very little real-world exposure (given the small timeframe from inception to review). Sure, such exposure isn't a requirement but it surely helps to make the library ready for review, i.e. reasonably mature. -- Andreas Huber When replying by private email, please remove the words spam and trap from the address shown in the header.
"Gennadiy Rozental" wrote:
I know I am slightly late but I am quite busy lately.
Problem domain ============
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. Explicit teardowns would be source of coupling and something
You are much welcomed. Thanks for the time to do the review and all the details. pretty hard to maintain (for me, others may find it easy).
* it doesn't seem to be ever portable to rely on order of global variables destruction in different libraries (dynamic or static)
Singletons in DLLs are pretty hard problem and standard C++ doesn't seem to provide enough means to handle them. One solution considered is to use unique shared memory where named singletons get registered but this is still huge work ahead.
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> > {}
This is valid issue. Perhaps named template parameters technique could be used here.
Inability of the author to clearly recognize orthogonal policies lead to 2 subpolicies with "create" in their name: CreationPolicy and Creator. The name and concept may be reconsidered as suggested by Dan Rosen in other thead.
I do not see why would anyone would need to employ malloc or std::allocator to allocate memory for global variable. Quite often. If you do custom memory management with new/delete you often set it up after statics initialization (say when loading a DLL).
Either one needs to do nasty tricks with shared variables or use malloc/etc.
Most interesting is that among all of these "creators"/creation_policies I don't see a solution for singleton with non-default constructor.
Yes, this should be supported. It is possible to explicitly pass parameters to the constructor as: A:pointer ptr; ptr->create(111); but this is different idiom and different use case.
-----------------
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>)
(used g++ 3.3.3 on windows)
I tried this with Intel 7 plugged in VC6 IDE, Release mode, all settings default and the ratio was 8.7 (in favor of raw class A).
-----------------
Author does admit that multithreading issues were not considered. IMO any singleton design that doesn't cover MT couldn't be accepted as generic. Not every library in Boost is MT safe even when it could be useful. MT was left into next stage.
Interface ======
* 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;
Yes. Member function T::pointer instance() { T::pointer ptr; return ptr; } has been suggested. This would allow the A::instance() syntax. It is also possible to wrap the singleton class into monostate like class: struct AA { A::pointer ptr; void DoSomething() { ptr->DoSomething(); } }; I suggested it as example but shortage of time ... /Pavel
participants (4)
-
Andreas Huber -
Andrei Alexandrescu (See Website For Email) -
Gennadiy Rozental -
Pavel Vozenilek