[RESOLVED] Can Singleton GetInstance() function be inlined?

[KrishnaPG]

Dear NMTop40,

Yes it is a singleton counter.

The address tests of Mr.venkyhyd also confirm this.

But, as pointed out by Mr.SuperKoko, due to the hidden boolen variable involved in realizing the static local variables by compilers, this implementaion may not be thread-safe.

The better implementation in this case would be

class Counter
{
private:
unsigned int* m_pValue;
inline Counter(unsigned int* pPtr) { m_pValue = pPtr;} // might want to assert(pPtr);
 
public:
 
inline Counter ( const Counter & other ) { *this = other; }
inline Counter& operator=( const Counter& other) 
{ 
m_pValue = other.m_pValue;
return *this; 
}
 
unsigned int getCount() const { return *m_pValue; }
unsigned int increment() { return InterlockedIncrement((LONG*)m_pValue); }
 
inline static Counter instance()
{
static unsigned int nCounter = 0;
return &nCounter;
}
};

The use of pointer here avoids the inherent boolean variable of the compiler and is thread-safe and faster (assuming proper inlining).

Thanking you,
Yours,
P.GopalaKrishna.

[NMTop40]

Guaranteed to be by the standard? Or just on his implementation?

It is threadsafe in its creation too. Of course the increment() function is not totally thread-safe - two different threads could get the same return value.

Did he try using it from a DLL/shared-object-library too?

[KrishnaPG]

Dear NMTop40,

It is threadsafe in its creation too.

Are you sure? Perhaps you might want to check old posts here !!

Thanking you,
Yours,
P.GopalaKrishna.

[NMTop40]

According to the standard, a static creating within a function is always an atomic action, ie.

A& instance()
{
   static A a;
   return a;
}

will never create more than one instance of A, no matter how many threads access this function.

The same is not true of this:

static A* pA = 0;

A* getAInstance()
{
   if ( !pA )
   {
       pA = new A;
   }
   return pA;
}

which is totally non-threadsafe, in fact you have to make 2 checks, one to first check if pA is NULL, then another after locking the mutex to check that another thread hasn't done so first.

[KrishnaPG]

Dear NMTop40,

Thankyou for the time and efforts you have spent in providing the information, though I must admit that it is of little use to our discussion here --- as it has already been pointed out in earlier posts in various ways.

Perhaps it would make a little bit good to you if I were to clarify that the notion of "thread-safe" here we are concerned with is --- initialization of the object while it is being created; (not the number of instances of the object, which, ofcourse, is guaranteed by the design to be one(1)).

Dear, Is this not time that you should check the old posts thoroughly before posting any further!!

Thankyou again for your time. It has been nice of you.

Thanking you,
Yours,
P.GopalaKrishna.

[SuperKoko]

Here is a theorically correct singleton class (i am open to any critical advice):

#include <windows.h>

class TCriticalSection
	{
	CRITICAL_SECTION cs;
	public:
	TCriticalSection() {InitializeCriticalSection(&cs);}
	~TCriticalSection() {DeleteCriticalSection(&cs);}
	operator CRITICAL_SECTION&() {return cs;}
	void Enter() {EnterCriticalSection(&cs);}
	void Leave() {LeaveCriticalSection(&cs);}
	};

class TClass
{
private:
static TCriticalSection critical;
static TClass * volatile Instance;


class TAutoDestroy
	{
	public:
	TAutoDestroy() {}
	~TAutoDestroy() {DestroyInstance();}
	};
friend class TClass::TAutoDestroy;

static TAutoDestroy AutoDestroy;

static TClass *CreateInstance();
static void DestroyInstance()
	{delete Instance;} // delete does nothing if the pointer is NULL.
	
	
	
private:

TClass() {MessageBox(NULL,"TClass::TClass","construction",MB_ICONEXCLAMATION | MB_OK);}
~TClass() {MessageBox(NULL,"TClass::~TClass","destruction",MB_ICONEXCLAMATION | MB_OK);}
TClass(const TClass &);
void operator =(const TClass &);

public:
static TClass *GetInstance()
	{
	if (Instance!=NULL) return Instance;
	else return CreateInstance();
	}
};
	
TClass::TAutoDestroy TClass::AutoDestroy;
TCriticalSection TClass::critical;
TClass * volatile TClass::Instance=NULL;

TClass *TClass::CreateInstance()
	{
	critical.Enter();
        if (Instance==NULL)
               {
               TClass *NewInstance=new TClass; // there is a sequence point at the end of this statement, to avoid an assignment to hInstance before the object is totally constructed!
               Instance=NewInstance;
               }
	critical.Leave();
	
	return Instance;
	}

int main()
	{
	TClass *p=TClass::GetInstance();
	MessageBox(NULL,"hello, world!","hello!",MB_ICONINFORMATION | MB_OK);
	return 0;
	}

Note, that a CRITICAL_SECTION is always created, even if GetInstance is never called.
But, CRITICAL_SECTIONs are very optimized objects, so they are fast to initialize (a mutex associated with the critical section is only created at runtime if needed).
CreateInstance is separated from GetInstance in the hope that the compiler don't inline it, so GetInstance (if inlined) uses less code.

[SuperKoko]

According to the standard, a static creating within a function is always an atomic action, ie.

You seems right, and i think that compilers misinterprets the standard, (because the standard talk mainly about monothread applications) and should produce thread-safe code (at least if the multithread command line option is checked).
But, we can just use compilers that don't conform to the standard on this point!

[venkyhyd]

now the question is --- would it make any difference between class objects and native types for local variable initialization ?
Because, as you could easily see, native types do not have any associated function with their initialization and thus are much different from classes in that --- a native type can be defined (initialized for the first time) with a constant value at any point of time before its usage, which is not the case for class objects --- for their definition might require the suitable values from its member variables !!

Dear P.GopalaKrishna,

I was just testing the code using Class objects as per my previous code where in i used Native Types (like int variable) but I didnot find any difference in using the native types and the user defined class objects. Can execute the below code and clarify the things..

 class Class1
{
public:
	int m_nVal;
	Class1() 
	{
		printf("In the constructor\n");
		m_nVal = 10;
	}
	~Class1(){ }
public:
	inline static Class1* GetInstance()
	{
		if(!UniqueObject)
		{
			printf("In the if statement\n");
			printf("Value of UniqueObject = %x\n", UniqueObject);	
			UniqueObject = new Class1();
			printf("Value of UniqueObject = %x\n", UniqueObject);	
			printf("Value of UniqueObject->m_Val = %d\n\n", UniqueObject->m_nVal);
			return UniqueObject;
		}
		else
		{
			printf("In the else statement\n");
			printf("Value of UniqueObject = %x\n", UniqueObject);	
			printf("Value of UniqueObject->m_Val = %d\n\n", UniqueObject->m_nVal);
			return UniqueObject;
		}
	}

private:
	static Class1 *UniqueObject;
};

Class1* Class1::UniqueObject = 0;

void Func()
{
	Class1* pFunct = Class1::GetInstance();
	printf("Value of pFunct = %x\n", pFunct);
	printf("Value of pFunct->m_Val = %d\n\n", pFunct->m_nVal);

}

int main(int argc, char* argv[])
{
	Class1* pClass1 = Class1::GetInstance();
	printf("Value of pClass1 = %x\n", pClass1);
	printf("Value of pClass1->m_Val = %d\n\n", pClass1->m_nVal);
	pClass1->m_nVal = 30;
	Func();

	Class1* pClass2 = Class1::GetInstance();
	printf("Value of pClass2 = %x\n", pClass2);
	printf("Value of pClass2->m_Val = %d\n", pClass2->m_nVal);

	printf("Value of pClass1 = %x\n", pClass1);
	printf("Value of pClass1->m_Val = %d\n\n", pClass1->m_nVal);
	pClass2->m_nVal = 40;

	printf("Value of pClass2 = %x\n", pClass2);
	printf("Value of pClass2->m_Val = %d\n", pClass2->m_nVal);

	printf("Value of pClass1 = %x\n", pClass1);
	printf("Value of pClass1->m_Val = %d\n", pClass1->m_nVal);


	return 0;
}

This is out of curiosity i have raised the issue here and wanted to clarify, is there any difference in the Native types and User Defined Types usage here in this context as you where pointing out, and please be noted tht i did not handle the Thread Safe and Memory Leaks issues in my code.

Have nice day,
-venky

[KrishnaPG]

Dear SuperKoko,

There are dozens of "theoritically correct" singleton designs out there.

But if you have not forgotten, the issue of this thread is more about the "efficiency factor" --- amongst all those dozen or so designs, which one is the most efficient in terms of reliability and speed of execution (and what is the role of "inlining" in such scene). And the speed of execution here is not just an abstract notion, but should involve all system parameters ranging from cache flushing to the page fault thrashing --- all are vital.

So the question you should be answering yourself is --- is the code you have posted as "theoritically correct" is also "the most efficient" design? In my humble opinion, Nope, its not.

By the way, for the sake of argument, what is your justification for this:

HINSTANCE hNewInstance=new TClass; // there is a sequence point at the end of this statement, to avoid an assignment to hInstance before the object is totally constructed!
hInstance=hNewInstance;

Where from you got this HINSTANCE into your code all of a sudden? I guess you meant this..

 TClass* NewInstance=new TClass;
Instance= NewInstance;

Now, I wonder if you are not becoming too smart as to imagine that new() would be assigning its return value even before it constructs the object entirely !! Besides, we are in critical section and the call to new() is a blocking call, not an asynchronous one. It returns only after the object has entirely been constructed. Unless you are worrying about the order of assembly instructions where by the four byte pointer is assigned only some of its bytes from the return value before it got preempted by another thread, there ir no reason to worry about it (or do you have some thing to say about it ??).

Thanking you,
Yours,
P.GopalaKrishna.

[KrishnaPG]

Dear Mr.venkyhyd,

The code you have presented involves: static Class1 *UniqueObject;

So you are using a native pointer type as a static varible, and hence its initialization does not involve any implicit function invokation while defining it (initializing the value for the first time). The construction of actual object pointed by the static pointer is explicitly handled by us instead of by compiler.
In this case, all that the compiler sees is just a static pointer being initialized at module level with a constant value 0 using,

Class1* Class1::UniqueObject = 0;

Thus there is no need for it to use a boolean flag mechanism or invoke any implicit functions while assigning 0 to the static pointer variable.

Since there are no static "class objects" involved in the code, there are no more problems here.

However, you might want to see the singleton counter code posted by Mr.NMTop40 in post #15 and contrast it with the code posted in #16, where the static class object variable is being replaced with a static native type variable without loosing any functionality. Such replacement mechanism allows us to use all the strengths of the compiler (such as inlining), while freeing us from its deficiencies (such as implicit functions, boolean checks for initialization etc...).

Thanking you,
Yours,
P.GopalaKrishna.

[NMTop40]

class Counter
{
private:
unsigned int* m_pValue;
inline Counter(unsigned int* pPtr) { m_pValue = pPtr;} // might want to assert(pPtr);
 
public:
 
inline Counter ( const Counter & other ) { *this = other; }
inline Counter& operator=( const Counter& other) 
{ 
m_pValue = other.m_pValue;
return *this; 
}
 
unsigned int getCount() const { return *m_pValue; }
unsigned int increment() { return InterlockedIncrement((LONG*)m_pValue); }
 
inline static Counter instance()
{
static unsigned int nCounter = 0;
return &nCounter;
}
};

My counter was an example of a simple singleton whereby the instance() function was declared in a header, and was intended to check whether you would always get a single instance (according to the STANDARD, not one particular compiler on the Windows platform).

Now how about some improvements to your code:

unsigned int* m_pValue;
inline Counter(unsigned int* pPtr) { m_pValue = pPtr;} // might want to assert(pPtr);

Why use a pointer? Use a reference. No need to assert anything then, as references can't be NULL.

inline Counter ( const Counter & other ) { *this = other; }
inline Counter& operator=( const Counter& other)
{
m_pValue = other.m_pValue;
return *this;
}

No need to overload these, the default versions are fine. Of course your class is not actually a singleton at all.

unsigned int increment() { return InterlockedIncrement((LONG*)m_pValue); }

If the atomic-increment function takes LONG* then it's not safe to just cast our pointer. Instead we should be using a LONG variable.

inline static Counter instance()
{
static unsigned int nCounter = 0;
return &nCounter;
}

So you think there is no "boolean" check there too by compilers with the use of your static int nCounter? And what makes it so much more thread-safe than my example? (The only thing that makes the counter thread-safe is using atomic-increment).

And why do you think it is more efficient? It seems less efficient, if anything, because it has to call constructors to Counter every time. (The compiler can optimise away the copy construction but I don't think it is likely to optimise away the impllicit constructor in instance() ).

By the way, why have you declared the function instance() as static? How will it be accessed externally?

[exterminator]

Perhaps it would make a little bit good to you if I were to clarify that the notion of "thread-safe" here we are concerned with is --- initialization of the object while it is being created; (not the number of instances of the object, which, ofcourse, is guaranteed by the design to be one(1)).

Is there any difference? If the number of object to be created is to be 1 then I guess the intialization would also happen once and that would be while creation of the object and no other instant of time and thread. I suppose that would be thread-safe in all senses ...wont it???

[SuperKoko]

By the way, for the sake of argument, what is your justification for this:

HINSTANCE hNewInstance=new TClass; // there is a sequence point at the end of this statement, to avoid an assignment to hInstance before the object is totally constructed!
hInstance=hNewInstance;

Where from you got this HINSTANCE into your code all of a sudden? I guess you meant this..

 TClass* NewInstance=new TClass;
Instance= NewInstance;

Thanks for the correction.
I was really tired when i wrote this post and was about sleeping, so don't search any logic in this stupid error, except that Instance sounds a little like hInstance in my head, what explains the error.

About the code:

Instance=new TClass;

There is no sequence point between the creation of the new class and the assignement, and what i fear, is that the compiler (which don't matter much about multithreading issues) produces a code like that:

push sizeof(TClass)
call operator new
mov [Instance],eax ; instance may be moved before the object is constructed!
push eax
call TClass::TClass
add esp,4

Even with the assignment "Instance=NewInstance" we can fear that it use non atomic instructions (four mov operations of one byte each, for example), but i think that no compiler will produce this code, especially with volatile variables.

[KrishnaPG]

Dear NMTOP40,

Thankyou for your reponse. As for your question

why have you declared the function instance() as static? How will it be accessed externally

May be you want to answer it yourself --- afterall it was your own singleton counter code that I have modified to show you how the boolean check can be removed !!

As for the improvements you have suggested for the references and the long types --- thankyou very much, though I would have liked it much better if you had taken note of those suggestions in your original counter code itself (atleast the long part of it w.r.to Thread-safety!!)

As for your comment:

Of course your class is not actually a singleton at all.

Hmmm.. Are you sure?? Perhaps you want to consider again..
Lets have it straight --- Do you have any example situation where this design results in a differrent behavior compared with any other singleton counter design ? Perhaps that would save all of us a lot of time, wouldn't it?

So you think there is no "boolean" check there too by compilers with the use of your static int nCounter?

IMHO Yes, unless you have some counter example (or case-in-point mechanism).

Thanking you,
Yours,
P.GopalaKrishna.

[KrishnaPG]

There is no sequence point between the creation of the new class and the assignement, and what i fear, is that the compiler (which don't matter much about multithreading issues) produces a code like that:

push sizeof(TClass)
call operator new
mov [Instance],eax ; instance may be moved before the object is constructed!
push eax
call TClass::TClass
add esp,4

Dear SuperKoko,
Do not worry about the situation of invoking the constructor after assignment. Compilers are not that dumb --- they have clearly specified rules for them, Construction of object is a "logical atomic unit of action" and assignment is another "logical atomic unit of action". There is no way the generated object code would mix them both (not even for the sake of optimization or any such nonsense). (Ofcourse, whether this logical atomic unit is really carried out as physical atomic computation is a debatable question, but it is not at all doubtful that two different logical atomic units would never be intermixed while code generation).
Thanking you,
Yours,
P.GopalaKrishna.