Jump to content
The Dark Mod Forums

Design for a new module system


Recommended Posts

Looks good, I've made a few minor changes which are in the SVN log. Mostly naming/structural stuff, although importantly I have removed the requirement for getModule() to throw an exception if initialiseModules() has not been invoked first, since modules might call getModule() during their own initialisation routine (if they require any access to other modules).

Right, thanks, I would have run into that one soon enough, I reckon. :)

 

Btw, the ModelLoader family (plugins/model/plugin.cpp) definitely needs more work. There are several classes which encapsulate the C interface of the picomodel library, and it's not clear where the RegisterableModule interface should go here, as this needs serious cleanup. I added it somewhere for the moment, so that the module gets at least recognised, but we will have to come back to that soon, I guess.

Link to post
Share on other sites
  • Replies 93
  • Created
  • Last Reply

Top Posters In This Topic

Status update: I'm mostly through with defining the RegisterableModule interfaces on all the modules (there are more than I thought!). For the radiant binary I introduced a convenience template StaticModule which registers the static modules with the ModuleRegistry.

 

Currently, the application is unstable like hell and crashes almost immediately, but all the modules get registered correctly at least. I'll try to insert a round of bugfixing to let it go further than just the Archive module.

Link to post
Share on other sites

Ok, we're getting a cyclic dependency and DarkRadiant crashes after an infinite loop:

 

Initialising module: Radiant
Initialising dependency: BrushModule
Initialising module: BrushModule
Initialising dependency: FilterSystem
Initialising module: FilterSystem
Initialising dependency: EventManager
Initialising module: EventManager
Initialising dependency: Radiant
Initialising module: Radiant
Initialising dependency: BrushModule
Initialising module: BrushModule
Initialising dependency: FilterSystem
Initialising module: FilterSystem
Initialising dependency: EventManager
Initialising module: EventManager
Initialising dependency: Radiant
Initialising module: Radiant
Initialising dependency: BrushModule
Initialising module: BrushModule
Initialising dependency: FilterSystem
Initialising module: FilterSystem
Initialising dependency: EventManager
Initialising module: EventManager
Initialising dependency: Radiant
Initialising module: Radiant
Initialising dependency: BrushModule
Initialising module: BrushModule
Initialising dependency: FilterSystem
Initialising module: FilterSystem
Initialising dependency: EventManager
Segmentation fault (core dumped)

Link to post
Share on other sites

OK, there are obviously two problem there: first, the cyclic dependency exists in the first place, and secondly it is not caught by the initialisation code like I thought it would.

 

To fix the initialisation code, I suspect the addition of the module to _initialiseModules needs to happen BEFORE the recursive dependency initialisation, rather than afterwards. This is independent of the actual call to RegisterableModule::initialiseModule(), so while the dependencies are initialised there will briefly be an uninitialised module in _initialiseModules.

 

Regarding the actual dependencies, you might know more than I about some of those modules: do the dependencies look correct to you? EventManager depending on Radiant looks a bit suspect to me, but I know relatively little about the EventManager to say for sure.

Link to post
Share on other sites

The dependencies are ok (the EventManager uses the GlobalRadiant() interface to set the status text), the problem arises like this. I have all my debug code on SVN, so you can see for yourself what's happening:

 Initialising module: ArchivePK4
ArchivePK4::initialiseModule called
=> Module ArchivePK4 initialised.
Initialising module: BrushModule
Initialising dependency: FilterSystem
 Initialising module: FilterSystem
 Initialising dependency: EventManager
  Initialising module: EventManager
  Initialising dependency: Radiant
Initialising module: Radiant
Initialising dependency: BrushModule (blocked) 
Initialising dependency: Clipper
 Initialising module: Clipper
 Initialising dependency: PreferenceSystem
  Initialising module: PreferenceSystem
PreferenceSystem::initialiseModule called
=> Module PreferenceSystem initialised.
 Initialising dependency: Radiant (blocked) 
 Initialising dependency: SceneGraph
  Initialising module: SceneGraph
CompiledGraph::initialiseModule called
=> Module SceneGraph initialised.
 Initialising dependency: SelectionSystem
  Initialising module: SelectionSystem
  Initialising dependency: Grid
   Initialising module: Grid
   Initialising dependency: EventManager (blocked) 
   Initialising dependency: PreferenceSystem
	Initialising module: PreferenceSystem
Module PreferenceSystem already initialised.
	Initialising dependency: Radiant (blocked) 
	Initialising dependency: XMLRegistry
	 Initialising module: XMLRegistry
XMLRegistry::initialiseModule called
=> Module XMLRegistry initialised.
GridManager::initialiseModule called.
Getting module: EventManager
ModuleRegistry: Warning! Module with name EventManager requested but not found!

Short summary: During the initialiseModules() routine, the dependencies are traversed and recursively initialised. To prevent an infinite loop, I blocked entering the recursion for modules that are in the InitialisationStack (where all modules currently being initialised are listed). If a dependency is listed in the InitialisationStack, the recursion is not entered again for this module.

 

Obviously, this is not enough, because the EventManager can be requested in the initialisation routine of a completely different module which is also in the (let's say) Radiant module.

 

Basically, our algorithm is lost here, because Radiant depends on EventManager, and EventManager on Radiant.

Link to post
Share on other sites

By moving the addition to _initialisedModules before the dependency recursion, I get this:

 

Initialising module: ArchivePK4
ArchivePK4::initialiseModule called
=> Module ArchivePK4 initialised.
Initialising module: BrushModule
Initialising dependency: FilterSystem
Initialising module: FilterSystem
Initialising dependency: EventManager
Initialising module: EventManager
Initialising dependency: Radiant
Initialising module: Radiant
Initialising dependency: BrushModule
Initialising module: BrushModule
Module BrushModule already initialised.
Initialising dependency: Clipper
Initialising module: Clipper
Initialising dependency: PreferenceSystem
Initialising module: PreferenceSystem
PreferenceSystem::initialiseModule called
=> Module PreferenceSystem initialised.
Initialising dependency: Radiant
Initialising module: Radiant
Module Radiant already initialised.
Initialising dependency: SceneGraph
Initialising module: SceneGraph
CompiledGraph::initialiseModule called
=> Module SceneGraph initialised.
Initialising dependency: SelectionSystem
Initialising module: SelectionSystem
Initialising dependency: Grid
Initialising module: Grid
Initialising dependency: EventManager
Initialising module: EventManager
Module EventManager already initialised.
Initialising dependency: PreferenceSystem
Initialising module: PreferenceSystem
Module PreferenceSystem already initialised.
Initialising dependency: Radiant
Initialising module: Radiant
Module Radiant already initialised.
Initialising dependency: XMLRegistry
Initialising module: XMLRegistry
XMLRegistry::initialiseModule called
=> Module XMLRegistry initialised.
GridManager::initialiseModule called.
Getting module: EventManager
Getting module: XMLRegistry
=> Module Grid initialised.
Initialising dependency: OpenGL
Initialising module: OpenGL
OpenGL::initialiseModule called.
=> Module OpenGL initialised.
Initialising dependency: SceneGraph
Initialising module: SceneGraph
Module SceneGraph already initialised.
Initialising dependency: ShaderCache
Initialising module: ShaderCache
Initialising dependency: Doom3ShaderSystem
Initialising module: Doom3ShaderSystem
Initialising dependency: PreferenceSystem
Initialising module: PreferenceSystem
Module PreferenceSystem already initialised.
Initialising dependency: Radiant
Initialising module: Radiant
Module Radiant already initialised.
Initialising dependency: VFS
Initialising module: VFS
Initialising dependency: ArchivePK4
Initialising module: ArchivePK4
Module ArchivePK4 already initialised.
VFS::initialiseModule called
=> Module VFS initialised.
Initialising dependency: XMLRegistry
Initialising module: XMLRegistry
Module XMLRegistry already initialised.
Doom3ShaderSystem::initialiseModule called
=> Module Doom3ShaderSystem initialised.
Initialising dependency: OpenGLStateLibrary
Initialising module: OpenGLStateLibrary
OpenGLStateLibrary::initialiseModule called.
=> Module OpenGLStateLibrary initialised.
ShaderCache::initialiseModule called.
=> Module ShaderCache initialised.
RadiantSelectionSystem::initialiseModule called.

 

After this, I get the old "used without being initialised" error from the old modulesystem. It looks like the infinite recursion is solved, but I suggest removing all use of the old modulesystem before proceeding any further since mixing the two up could lead to disastrous and hard-to-debug errors.

 

Even if the ModuleRegistry can cope with cyclic dependencies, their existence should still be considered a design bug because they don't make logical sense and could lead to errors (e.g. if module A depends on B which depends on C, which in turn makes a call back to A which doesn't work because A has not finished initialising yet).

Link to post
Share on other sites

Thinking about that, the problem will most likely go away when the dependencies are cleaned up. Everything that isn't crucially needed during the initialiseModule() routine, must be removed from the dependency list.

 

The old Global*Ref were there to initialise the pointers, but our dependencies are just to handle the initialisation phase, which is not as strict.

Link to post
Share on other sites
Everything that isn't crucially needed during the initialiseModule() routine, must be removed from the dependency list.

 

This cannot unfortunately be relied on, because A::initialiseModule() might call B::somefunc() which in turn calls C::someOtherFunc(), even though B::initialiseModule() does NOT make any calls to module C. Module B must still be dependent on C in this case, even though the call is not made during its initialisation.

Link to post
Share on other sites

You're probably right. I'll still clean up the dependencies anyway, and start to remove the old modulesystem. Btw, I changed the Global*() accessor methods to this:

inline Registry& GlobalRegistry() {
static boost::shared_ptr<Registry> _registry(
	boost::static_pointer_cast<Registry>(
		module::getRegistry().getModule(MODULE_XMLREGISTRY)
	)
);
return *_registry;
}

Do you see any culprits about this approach?

Link to post
Share on other sites

The cast is fine, but don't use a static. Using a static in an inline function in a header file doesn't generally work anyway (you'll get multiple copies of your static in anything that includes it), and there is no particular benefit in caching the pointer in this case (you're probably thinking of performance improvements, but it is unlikely to be significant and if client code wants to improve performance it can cache the result of the GlobalBlah() call itself).

 

Regarding the dependencies, anywhere there is a cycle it should be broken by removing the least important piece of functionality (so in the example of EventManager depending on Radiant, just disable updating the status bar until it can be sorted out later). I wonder if one of the causes of instability in the old modulesystem is all of these cycles which we didn't detect until now.

Link to post
Share on other sites

Yes, I was thinking of performance indeed, as the module lookup is performed using a std::string as key, which is presumably not the fastest way possible. I tried to come up with something that intrinsically caches the ModuleRegistry lookup to make the life for the calling code easier.

 

And I also wondered what a static in an inline would do, but couldn't be bothered to do some proper research. I'll check sometime if Effective C++ has something to say about this type of problem.)

 

Anyway, I'll remove the static. :)

Link to post
Share on other sites
Yes, I was thinking of performance indeed, as the module lookup is performed using a std::string as key, which is presumably not the fastest way possible. I tried to come up with something that intrinsically caches the ModuleRegistry lookup to make the life for the calling code easier.

 

Good point, by using a string lookup there will be a slight performance penalty over the previous hard-typed version. Still, I don't think it is unreasonable to specify that module retrieval is not guaranteed to be high-performance, and if code is doing something performance-intensive it needs to save the pointer itself.

 

And I also wondered what a static in an inline would do, but couldn't be bothered to do some proper research. I'll check sometime if Effective C++ has something to say about this type of problem.)

 

Yes, I'm not sure myself. There is probably a difference between inlined and non-inlined code in header files, maybe the compile refuses to inline functions with statics or something. Either way, I wouldn't trust using such a construct.

Link to post
Share on other sites

My guess was that the static exists only once in per binary (once per radiant core, and once per each plugin) but that the actual code is still inlined, i.e. only the code gets inlined into the calling function, not the memory the variable itself is allocating. The constructor of the static would be called only once per module, which would be exactly what I intended. But I'll have to research that one.

Link to post
Share on other sites

Btw: If you feel like experimenting with the ModuleRegistry's initialisation algorithm, don't hold yourself back (you're better at it than me anyway, I reckon). I'll probably don't touch it for the next time, I'll be busy replacing all the accessor methods and removing old stuff. :)

Link to post
Share on other sites

OK, the only thing I changed was the removal of the InitialisationStack which is not needed once the infinite recursion thing was sorted out by moving the map insertion.

 

I wonder if this would be a good opportunity to move as many static modules as possible into their own DLLs? Hopefully the new module system should make this easier now that the templated stuff doesn't need to be used.

Link to post
Share on other sites

Yes, it's a good opportunity. A lot of stuff will be removed when I'm through with my changes, this will make things much more clearer.

 

Concerning the static keyword in the accessor methods: Currently I have a std::cout debug message in ModuleRegistry::getModule(), like "Getting XMLRegistry", which is written to the console everytime the module is requested via GlobalRegistry(). The list of "Getting XMLRegisty" outputs is very long, as expected.

 

Making the shared_ptr in the GlobalRegistry() method static, reduces the output to one line, which is an indication that the static local variable is not "inlined" along with the code. I don't know whether the code itself is inlined or not, maybe the compiler doesn't do it for inlines containing statics, but at least this is a way of caching the reference. Even if the code inlining is not performed, a simple function call is definitely cheaper than the whole ModuleRegistry lookup.

 

However, I haven't done any substantial research on this topic yet. :)

Link to post
Share on other sites

The problem is that it's one of those things that might "just happen" to work OK, but be non-standard and technically undefined meaning that it could break later under different circumstances or on a different platform (or compiler).

 

What would seem likely is that at the very least, there is no guarantee that multiple calls in different Translation Units will be seeing the same static object, because the function is included separately. In this case this would not be a problem (unlike with a singleton class) because the static object is only a pointer to a real singleton object. If this is the only effect, and using a static inside a header-file method is not Undefined Behaviour according to C++, then this might be OK to do.

 

The potential problem it might have is if a GlobalBla() function was called before the module was created, and then later on afterwards, the second call would still return the empty pointer because the static instance would cache it. I don't know if this could ever happen in practice, but it's a potential gotcha that would be very non-obvious to debug if you didn't know what was happening.

 

EDIT: It's IBM-specific, but

 

http://publib.boulder.ibm.com/infocenter/p...ref/cplr243.htm

 

says this:

 

Static locals and string literals defined within the body of an inline function are treated as the same object across translation units;

 

which is encouraging -- I wonder if this is therefore compiler-linker specific?

Link to post
Share on other sites
The potential problem it might have is if a GlobalBla() function was called before the module was created, and then later on afterwards, the second call would still return the empty pointer because the static instance would cache it. I don't know if this could ever happen in practice, but it's a potential gotcha that would be very non-obvious to debug if you didn't know what was happening.

In the case of the new modulesystem this shouldn't produce any problems, because a std::logic_error would be thrown when the module isn't existing yet. But I do get your point - I'm aware of the problems which blindly using such a construct in similar but not equal situations would cause.

 

I guess I'll do some reading during the compile times. :)

Link to post
Share on other sites
In the case of the new modulesystem this shouldn't produce any problems, because a std::logic_error would be thrown when the module isn't existing yet.

 

It shouldn't throw, because the interface requires getModule() to return an empty RegisterableModulePtr if the module does not exist. I added this deliberately because it allows for modules to be optional (which is something I wanted to add before but wasn't even going to attempt with the old modulesystem).

 

For example, the particles module is not critical, and could feasibly be disabled if necessary. You could therefore use:

 

ParticlesManagerPtr mgr = GlobalParticlesManager();
if (mgr) {
// Add in the particles widgets
}

 

Hmm, I guess this means the GlobalBlah() method interface needs to be changed -- it can't return a reference if the shared_ptr might be empty. It would have to return a shared_ptr instead.

 

Alternatively, it could return a reference and still throw an exception to indicate a non-existent module, but it can't be a logic_error in that case, because it is a valid operation. There would need to be a ModuleNotFoundException or something.

Link to post
Share on other sites

This guy here is also talking about statics in inline functions. According to him, a problem might arise if the function cannot actually be inlined by the compiler (like recursive inlines and other more complex functions - the inline is only a hint to the compiler). Given the case that the method is not inlined, the static gets distributed over the translation units, which is of course undesired. In our case, it would work, because the function is simple enough to be inlined.

 

Moreover, our "worst case" is that each translation unit gets its own "cached" reference, which would still be an improvement over calling the getModule() function every single time.

edit: Just saw your post. I seem to have overlooked that requirement of returning empty pointers, wouldn't this cause more problems than anything else (all the existing code is relying on the Global*() accessors returning a valid reference)? I already have a moduleExists() method defined in the moduleregistry, shouldn't we use this instead?

Link to post
Share on other sites

I extended the ModuleRegistry interface with a new routine:

	 /**
 * All the RegisterableModule::shutdownModule() routines are getting 
 * called one by one. No modules are actually destroyed during the 
 * iteration is ongoing, so each module is guaranteed to exist.
 * 
 * After the last shutdownModule() call has been invoked, the modules
 * can be safely unloaded/destroyed.
 */
virtual void shutdownModules() = 0;

The RegisterableModule::shutdownModule() method is empty by default, so modules don't need to implement them. This allows modules to de-register themselves from the various callbacks, observer roles, whatever.

 

There are just too many things going on in the old API destructors - I can't change everything in the codebase, the necessary changes due to the new ModuleSystem are already complicated enough. :)

Link to post
Share on other sites

edit: Just saw your post. I seem to have overlooked that requirement of returning empty pointers, wouldn't this cause more problems than anything else (all the existing code is relying on the Global*() accessors returning a valid reference)? I already have a moduleExists() method defined in the moduleregistry, shouldn't we use this instead?

 

I guess there is not really a lot in it either way, if a moduleExists() method is defined then this will work just as well.

 

I extended the ModuleRegistry interface with a new routine:

 

That is no problem, I initially thought that a shutdown method would not be that important but if it is needed there is nothing wrong with it.

Link to post
Share on other sites

Current status update: The old modulesystem is completely gone, everything has been migrated to use the new ModuleRegistry. The modules are cleaned from deprecated code (like STRING_CONSTANT macros, ModuleRefs, and the Radiant_RegisterModule symbol).

 

On the way here I also refactored part of the model and imageloader modules, which were a bit complicated. All the API classes were not longer needed and are removed now, save a few occurrences, where more work is required.

 

So far the good news. DarkRadiant is still far from being useable, as many things are broken (Toolbars are missing, brushes are not showing up, it crashes on every second breath). Now that the deprecated code is gone, I can start debugging and reorganising the startup code.

Link to post
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.


×
×
  • Create New...