Re: [boost] Is there interest in an alternative to the Singleton anti-pattern?
I would like to see the updated source code. Even if boost vault is deprecated and moved to github, it is still a good place to post files.
I have posted the files to: http://www.mediafire.com/?d8kn5fx2n5d22
Two more remarks: 1) it would be great to use auto_ptr for instance_ptr_type. It will make sure that the resource is deleted and the constructor called.
The purpose of auto_ptr is to clean up the instance automatically. It was a design goal to make destruction of the Singularity explicit, especially in multi-threaded contexts. However, users can create custom ThreadingModel policies which use any kind of smart pointer they desires. I thought to provide the only a primitive pointer for the included policies.
Singularity, I feel I still must make "instance_ptr" volatile for thread safety. ?If I do not, the compiler can store the pointer in a register, and other threads of execution will not see changes to its value. ?The linked article seems mostly concerned with fencing access to the variable, which I am achieving by using a mutex object. ?I only need volatile to prevent effectively reading a "cached" value for the pointer. ?I found
2) 2011/6/25 Ben Robinson <icaretaker@gmail.com>: the
following article by Andrei Alexandrescu especially interesting: http://drdobbs.com/cpp/184403766
It is not thread safe. Consider the article http://www.aristeia.com/Papers/DDJ_Jul_Aug_2004_revised.pdf by Scott Meyers and Andrei Alexandrescu. In the article by Andrei Alexandrescu at http://drdobbs.com/cpp/184403766 there is no construction of object in LockingPtr, and that makes a big difference!
Singularity does not using the Double-Checked Locking Pattern. When using the multi_threaded policy, it always acquire the mutex before accessing the volatile instance pointer. Therefore, it is thread safe, although it is certainly not as efficient as possible. The recommended usage of Singularity is without a globally accessible get() function, so the performance of create() and destroy() should not be an issue. If global_access is enabled, it would be advisable to minimize calls to get(), such as calling get() once per thread, storing the result in thread-local storage.
Best regards, Antony Polukhin
I greatly appreciate the feedback you are providing. In addition, the linked article was a fascinating read. Thank you, Ben Robinson, Ph.D.
2011/6/27 Ben Robinson <icaretaker@gmail.com>:
The purpose of auto_ptr is to clean up the instance automatically. It was a design goal to make destruction of the Singularity explicit, especially in multi-threaded contexts. However, users can create custom ThreadingModel policies which use any kind of smart pointer they desires. I thought to provide the only a primitive pointer for the included policies.
It is a good thing, to have an explicit destructor, but we must call destructor in case, when user forgets to call destruction or when the program suddenly exits. Consider the following example: DescriptorThatMustBeClosed & horizon = DescriptorThatMustBeClosed::create(&event); ... function_that_calls_exit(); ... DescriptorThatMustBeClosed::destroy(); Moreover, auto_ptr is primitive enough and will not make the performance worse.
The recommended usage of Singularity is without a globally accessible get() function, so the performance of create() and destroy() should not be an issue. If global_access is enabled, it would be advisable to minimize calls to get(), such as calling get() once per thread, storing the result in thread-local storage.
Than it is OK. More remarks: 1) it is a common practice to put all the support structures and variables to namespace boost::detail 2) Overload the virtual const char* what() const throw(); function for all your exceptions (struct singularity_already_created, already_destroyed, singularity_not_created, singularity_destroy_on_incorrect_threading ) 3) Back to the question with auto_ptr. It looks like there is no need in specifying the ptr_type. You always return references, user can't get access to the pointer directly. So, all you need is to hold the pointer and take care of freeing resources, if user forget to do it. And std::auto_ptr looks like a great thing for such cases Best regards, Antony Polukhin
On Mon, Jun 27, 2011 at 12:59 AM, Antony Polukhin <antoshkka@gmail.com>wrote:
2011/6/27 Ben Robinson <icaretaker@gmail.com>:
The purpose of auto_ptr is to clean up the instance automatically. It was a design goal to make destruction of the Singularity explicit, especially in multi-threaded contexts. However, users can create custom ThreadingModel policies which use any kind of smart pointer they desires. I thought to provide the only a primitive pointer for the included policies.
It is a good thing, to have an explicit destructor, but we must call destructor in case, when user forgets to call destruction or when the program suddenly exits. Consider the following example:
DescriptorThatMustBeClosed & horizon = DescriptorThatMustBeClosed::create(&event); ... function_that_calls_exit(); ... DescriptorThatMustBeClosed::destroy();
Moreover, auto_ptr is primitive enough and will not make the performance worse.
[...] Forgive me as I don't know the entire context of this, but would boost::scoped_ptr be sufficient here? One would never transfer the ownership of the object referenced by std::auto_ptr, would they? - Jeff
2011/6/27 Jeffrey Lee Hellrung, Jr. <jeffrey.hellrung@gmail.com>:
Forgive me as I don't know the entire context of this, but would boost::scoped_ptr be sufficient here? One would never transfer the ownership of the object referenced by std::auto_ptr, would they?
Yes, you are right. boost::scoped_ptr will suit better. Thank you. Best regards, Antony Polukhin
On Mon, Jun 27, 2011 at 10:53 AM, Antony Polukhin <antoshkka@gmail.com>wrote:
2011/6/27 Jeffrey Lee Hellrung, Jr. <jeffrey.hellrung@gmail.com>:
Forgive me as I don't know the entire context of this, but would boost::scoped_ptr be sufficient here? One would never transfer the ownership of the object referenced by std::auto_ptr, would they?
Yes, you are right. boost::scoped_ptr will suit better. Thank you.
Antony and Jeffrey,
An improved version of Singularity is now available at https://github.com/icaretaker/Singularity. I have reimplemented Singularity using the scoped_ptr<T>. After further research, I am convinced the volatile keyword was useless, given that all accesses to the shared pointer were fenced by acquisition of a boost::mutex. This greatly simplifies the implementation. Also, should the user fail to call Singularity<T>::destroy(), the destructor for the instance of T will be called when the program exits, as this is when the static scoped_ptr<T> goes out of scope. Thanks again, Ben Robinson, Ph.D. Best regards,
Antony Polukhin _______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
On June 28, 2011 03:29, Ben Robinson wrote:
I have reimplemented Singularity using the scoped_ptr<T>. After further research, I am convinced the volatile keyword was useless, given that all accesses to the shared pointer were fenced by acquisition of a boost::mutex.
The problem with using shared or scoped pointer and boost::mutex is that you're depending on order-of-construction. For a singleton to be safely used pre-main, this is not an option. I have previously implemented my own generic singleton template, which depends only on static initializers. Since it didn't need to be cross-platform, I could take advantage of PTHREAD_MUTEX_INITIALIZER and employ lazy construction w/ double-checked locking (where 'volatile' can be useful). You can further optimize double-checked locking (for GCC), using "__attribute__ ((const,nothrow))" to eliminate some redundant calls to get the singleton from within the same scope. A singleton is declared as: static singleton<T> Single_inst = SINGLETON_INIT; Where: #define SINGLETON_INIT { NULL, { PTHREAD_MUTEX_INITIALIZER } } It does have a destructor, which does the same job as boost::scoped_ptr's. The singleton template supports two initialization approaches. The first is roughly: T *Get_single() __attribute__ ((const,nothrow)) { T *s = Single_inst.get(); if (!s) s = Single_inst.init(new T()); return s; } This assumes you don't mind multiple T's being constructed, in the rare case that multiple threads are competing for first access (singleton<T>::init() will reliably delete any superfluous ones). The second defers creating T until the init lock has been obtained (which only succeeds for the first caller): T *Get_single() __attribute__ ((const,nothrow)) { T *s = Single_inst.get(); if (!s) { if (Single_inst.try_init_lock()) { s = Single_inst.init_locked(new T()); } else s = Single_inst.get_blocking(); } return s; } The single<T> member functions should be pretty self-explanatory. The reason for the second get method is that try_init_lock() can fail if the init lock is currently held, though the pointer still might not yet be valid. Therefore, you need a method that reads the value only after obtaining the mutex. In both of initialization sequences, singleton<T>::get() simply returns the value of the enclosed pointer. Obviously, Get_single() can be a static member function of singleton<T>. In most cases, there's no reason to do otherwise. However, I recommend at least giving users the option to initialize from the outside, as it has the advantage of facilitating more elaborate initialization sequences (e.g. if a few things need to be configured on T, before passing it to singleton<T>::init_locked(); or if T * is actually returned by a function, rather than directly constructed). Cheers, Matthew A. Gruenke
On Thu, Jun 30, 2011 at 4:40 PM, Gruenke, Matt <mgruenke@tycoint.com> wrote:
On June 28, 2011 03:29, Ben Robinson wrote:
T *Get_single() __attribute__ ((const,nothrow)) { T *s = Single_inst.get(); if (!s) { if (Single_inst.try_init_lock()) { s = Single_inst.init_locked(new T()); } else s = Single_inst.get_blocking(); } return s; }
I think you need a memory barrier after setting s, as well as on the if (!s) test. The assignment to s can drift upwards and *into* the init_locked() or get_blocking() calls, including drifting up above an unlock() inside those functions. Then another thread, at if (!s) will see s as set, but maybe not all the members of T will have been initialized yet. Yet s is returned and used... Or do you have a full read-write barrier at the end of init_locked() and get_blocking(). Also what barrier, if any, is inside get()? I'd suggest using boost::call_once(). Tony
participants (5)
-
Antony Polukhin -
Ben Robinson -
Gottlob Frege -
Gruenke, Matt -
Jeffrey Lee Hellrung, Jr.