Revision 53ef86d...

Go back to digest for 10th July 2011

Features in Games

Laszlo Papp committed changes in [gluon] /:

Core/Singleton: Implement a proxy method for getting a mutex instance for need

The basic issue that this commit address is that the static mutex of the
singletons is created during the initialization stage, before the main entry
point. This would not cause any issue itself, but the gluonobjectfactory creates
objects for getting the mimetypes in case of the GluonObjects that are targetted
for registration.

The problem is also that C++ does not have virtual static method as we say in
the relevant comment of the factory code and as such we need to create objects.
The issues come into the picture when these objects, which are also created during
the initialization stage (before the main entry point), try to access the
singleton instance methods in their constructor. It might result that the mutex
tries locking in that method, but it can crash anytime without any guarantee for
the proper operation if the static mutex is not created. We cannot make the
order proper because there is no standard C/C++ way of defining these things.

The order of the global/static initialization is undefined as such during that
stage. I have tested on my i386 and the linker does it from the last included
source file, thus while creating *.o files the order is really not guaranteed
for the initializations.

There are essentially more approach for solving this issues.
1) Make static supportedMimeTypes methods for the GluonObjects that will be
registered by the factory. The problem is that with this approach we could not
provide a pure virtual static method-like way, thus we would force each class
that would like to be registered to implement it on their own which can be not
that user/programmer friendly. We can do just fine with virtual methods as
non-static if construct the objects there.

2) We could eliminate all the singleton instance method calls from the
constructor of the relevant GluonObjects that will be registered. It is not
something that handy either. However I eliminated the audio engine instance
method call (which is a singleton type) from the soundlistener component
constructor. It hsa not been still enough actually. For instance the
materialinstance constructor was also using the Graphics Engine (singleton)
instance in its constructor. That can be arguably eliminated, but the problem is
that we should take care about these things all the time when we have a class
that will be registered. In my humble opinion, that is why it is that good
approach either heretofore.

3) Guarantee that the static mutex of the singletons are created properly before
trying to use them for locking the Singleton creation. After a lot of thinking,
I chose this way. This would actually solve all the crash we experienced on
Windows, Nokia N9 (Harmattan/Maemo6) phone and the "random" crash on my Linux
desktop PC. Yes, "random" crash because the order of the collected cpp source
files in the CMakeLists.txt cmake filesmatters differently in different
environments.

After all, it is not that easy to write a thread-safe singleton, and people like
generally avoiding singletons in multi-threaded application as much as possible,
but we still ship a nice way of doing it.

The basic idea was to provide a proxy method (we called it later mutexInstance
after the agreement about it) that creates a mutex if it is needed. Imagine it
as the singleton instantiation happens (that is how the name comes from there).

There were more ideas actually how to accomplish it properly that I would like
share with you. The first idea has been to make a boost::scoped_lock like way
here, as in a critical section, but that would not have really solved the issue
because the basic issue would have remained how to "protect" a function local
static variable which is not thread safe by "purpose". There are more
discussions like that on the qt mailing list arhcive site, but the tryReadLock
and QMutexLocket, etc did not really cause the proper functionality and
operation.

There was also a very simple approach to just make the localfunction variable
static. That is not enough and not even thread-safe even if the compilers
nowadays has the proper options *by default* for that it is really not safe to
make guess work each architecture we ship. The C++ standard before C++11 or
C++0x were not really thread-safe scaled which is slighly fixed by the now
upcoming standard, thus we should not really make guess-works over there.
(even though gcc has -Wfno-global-static option, but it is enabled by default)

I was also considering using spinlock. The idea is not to lock the constructor,
the idea is to lock *before* the construction happens to ensure no duplicate
instance is created. If you locked inside the constructor, you could end up with
multiple instances. The idea of the lock is not just to ensure the constructor
body is synchronized. Which is a part of it, but there's obviously more into it.
It does not matter here if you have a spinlock or a sleeplock or whatever,
it's negligible. The lock acquisition only happens once, when you create the
instance and the only time ever the lock acquisition won't be instant is when
another thread tries to call instance() at the same time while other thread is
still constructing the singleton. The lock is only acquired during
initialization. Spinlock is just an alternative lock to sleeplock where instead
of going to sleep, it busy-waits with a loop. It is useful if you know you'd
only sleep for small amount of time, so there's no reason to do expensive
context switching or OS scheduling. Yes, going to sleep implies a context
switch, but like I said earlier: 1) the chances of two threads ever calling this
function at the same time are very very very slim. 2) the only time lock is ever
acquired is during initialization, which happens exactly once. 3) because of the
two afromentioned reasons, which kind of lock you use is negligible, VERY
negligible and making an assumption that spinlock is better for the situation is
bad: 1) you don't know HOW LONG the construction of the singleton takes, it can
take so long that a sleeplock would perform better. 2) this is a library, you
don't know on what kind of machine the code is going to be run on, spinlock on
single-core, single-cpu machine is a very very bad idea. There's no reason to
change it, it's negligible. Spinlock will be perform better in certain
situations and worse in others, but it does not matter at all.

So we are at the discussion of the final solution which was is a good idea
actually after discussing it with thiago IRC so that using QBasicAtomicPointer
and atomic swap in the proxy method. There is a good example for this uage
inside the Q_GLOBAL_STATIC internal Qt API macro, but since it is not documented
and not even planned to be a public API, they would not like to fix bugs about
it and they would like to make changes anytime without warning us, I could not
just simply use this nice macro written by thiago. That is true,
QBasicAtomicPointer is not documented either, but targetted for public usage as
I was pointed out by thiago. The documentation will happen by him as soon as
possible. There is a documented way, like QAtomicPointer, but the problem is
that with it is not really a POD type which will not solve the issue of having
another thing that should be defended. Hence after learning the internal
Q_GLOBAL_STATIC internal Qt API, I decided to implement our way similarly and
the result is awesome.

During the internal review we decided to put the class members into the private
section and also this proxy method. The only public method remained is the
instance method of the singleton.

Finally I could eliminate the hack in the gluon cmake file (CMakeLists.txt)
where the order mattered previously, so it is nice as expected by now.

About the testing: I could not test it windows, but I discussed it with the
windows guy so that he can try this patch out later today. Nevertheless I could
test on Linux and it works just neatly and I also tested on my Nokia N9 device
where it also worked. There are some graphics issues, but that is not relevant
to this change.

File Changes

Modified 2 files
  •   core/singleton.h
  •   graphics/CMakeLists.txt
2 files changed in total