Engine Memory Management

From FIFE development wiki
Jump to: navigation, search

This article is outdated and is just stored for archive purposes. Archived.png

This article became outdated and is just stored for archive purposes in this wiki. There are several reasons why an article could become outdated. The development team may have decided to use a different concept or even the author itself felt that the article is not really up-to-date with the current development status of the project anymore.

Introduction

Problems to consider

SWIG memory management

SWIG memory management in swig proxies work like described here: [1]. The main points below:

  • Each proxy class keeps an extra flag to indicate ownership. C++ objects are only destroyed if the ownership flag is set.
  • When new objects are created in the target language, the ownership flag is set.
  • When a reference to an internal C++ object is returned, it is wrapped by a proxy class, but the proxy class does not have ownership.
  • In certain cases, ownership is adjusted. For instance, when a value is assigned to the member of a class, ownership is lost.
  • Manual ownership control is provided by special disown() and acquire() method

Some points on SWIG and c++ smart pointers: [2]

Memory management with SWIG & Python: [3]

In general, exposal of constructors bring the memory management issues most visible to the scripting side (since that sets thisown automatically to one). One could set thisown to zero, but then we have potential memory leaks, unless the ownership is handed to the engine. With these points in mind it could be a good idea that engine only exposes factory methods for instance creation. This can be done at least by setting constructors to private in engine side OR by not exposing the constructor in interface file + using %nodefaultctor. Another alternative could be to use smart pointer support to some extent, but in that case we might encounter cyclic reference problems (like in the old engine version).

See related discussion from fife forums

Increased test verbosity

Simply the amount of test code produced by the memory management solution.

Increased test complexity

The complexity of test cases resulted by the memory management solution. Mentioned as a separate item from verbosity, since verbose test case might still be trivial.

Inability to test small functionalities in separation

The goal of unit testing is to isolate each part of the program and show that the individual parts are correct (wikipedia). In case chosen memory management solution cannot instantiate isolated small parts of the engine in separation of its containers, engine cannot effectively be unit tested. In that case engine can be only tested via integration testing [4], thus losing the unit testing benefits.

Increased code verbosity

Simply the amount of functional code produced by the memory management solution.

Increased code complexity

The complexity of functional code resulted by the memory management solution. Mentioned as a separate item from verbosity, since verbose code might still be trivial.

Inconsistency

In case memory management practices differ from module / interface to another, errors are easier to make. Also implementation in general becomes harder due to different memory management rules in different domains.

Interface misuse

It shouldn't be possible to easily by mistake misuse any of the interfaces. Accidental misuse create mysterious error cases, like when adding instances to containers where they shouldn't be added (iteration), or adding instances to multiple managed containers (so that delete fails on other container). All misuse cannot probably be prevented, but it should be prevented where possible and made clear where not possible.

Broken encapsulation

Some solutions compromise class encapsulation [5] by introducing friend classes. Strictly managed friend class usage is probably viable compromise, but how well that can be managed in open source project where new members come and go without knowledge of prior decisions. In general it might be better to not use compromises at all, but instead rely on strict rules in normally recognized "design smell" cases (like prevent the usage of gotos, multiple implementation inheritance or friend classes).

Cyclic references

Some solutions have problems with cyclic memory references, leaving memory unallocated.

Alternatives

Plain strict instance hierarchy

With strict instance hierarchy, each class instance has a clear owner. Its the owner's responsibility to delete the owned instance. This is usually done either on owner destruction or with separate purging call to owner interface.

Engine memory strict.png

Pros

  • Each instance has clear ownership, easy to understand
  • With usual use cases, results possibly the smallest code amount
  • With usual use cases, results possibly the most trivial code
  • Enforces ownership via factory methods, hard to misuse
  • No cyclic reference problem

Cons

  • Strictly enforced ownership might result some non-trivial workarounds
    • e.g. Addition friends due to hidden constructors
  • Breaks encapsulation by introducing friend class relationship between parent and child (child's private constructor)
  • Makes unit testing impossible, since whole (possibly deeply nested) ownership chains need to be instantiated for every test
  • Makes tests verbose, since whole ownership chain needs to be instantiated

Strict instance hierarchy + visible constructors

This alternative is basically the same than the previous, with the exception that childs have exposed constructors.

Pros

  • In normal cases, each instance has clear ownership, easy to understand
  • results possibly the smallest code amount
  • results possibly the most trivial looking code
  • encapsulation not broken
  • Unit testing is possible
  • Tests not verbose
  • No cyclic reference problem

Cons

  • Easy to accidentally instantiate classes from scripting side the wrong way
    • E.g. in case the instance goes out of scope, instance gets deleted even though engine is still using it -> segfault
  • Also in engine side its easier to make similar mistakes
  • The same danger also in test cases

Memory Manager interfaces

With memory manager interfaces most of the classes that are exposed to scripting side would be inherited from IResourceManager, IManagedResource or both depending on its role. IResourceManager marks class to be such that it can contain IManagedResource instances. IManagedResource marks class to be such that its memory is managed by some engine side entity. All classes inherited from IManagedResource can only be instantiated either via static constructor taking IResourceManager as a parameter, or from its corresponding ResourceManager interface. With this solution, test code does not need to instantiate the deeply nested ownership hierarchies, instead it can instantiate IManagedResouce classes into generic resource manager instance (that would just basically hold the pointers and delete them on its destruction). See code draft from pastebin.

Engine memory memmanager.png

Pros

  • Marks clearly the class ownership responsibilities
  • Enforces the proper construction in most of the cases
  • encapsulation not broken
  • Unit testing is possible
  • Tests not verbose
  • Hard to accidentally instantiate classes from scripting side the wrong way
  • No cyclic reference problem
  • Most of the Cons can be resolved by some simple defensive programming

Cons

  • Concept harder to understand than more plain solutions
    • Additional interface inheritance to many classes
    • Container instance allocations result callbacks to container increasing complexity
  • Still leaves possibility to misuse the interfaces (by adding instances to wrong containers), though this might be quite rare accident(?).
    • Just my 2cents, but I think possible misuse of interfaces is rather serious here. Compare this solution to the Strict-Hierarchy-With-Constructor solution. The ManagedResource creation function, like the constructor in Strict-Hierarchy, should not be called in almost any normal situation (except unit tests). So this doesn't solve that problem, and in addition it adds an addManagedResource() function. To a new user, it might seem reasonable to create an object using its constructor and then add it to the appropriate place using addManagedResource. This will result in incorrect behavior. --jwt 11:35, 28 September 2007 (EST)
      • Adding wrong types to wrong containers could be prevented either by using rtti or adding type checking methods IManagedResource. Furthermore by checking the container methods for proper instance addition I see the following activities:
        • Check if we have a container for the new instance (if not, create it)
        • Check if we have already that instance created (if exists, skip)
        • Create the instance
        • Add instance to container
        • all these could be done in addManagedResource function making it possible to construct instances properly via both corresponding factory function OR direct addManagedResource call. One can naturally argue about the beauty of that solution
      • The reason why I think incorrect addition differs from exposed constructor case is that with default constructors, user can do this by accident, while with static constructors the container must be placed as a parameter by conscious decision. --Jasoka 18:37, 28 September 2007 (CEST)

Smart Pointers

Smart pointers takes care of the memory management via reference counting. Its up to the clients to call ref/unref calls depending on their instance usage. There is plenty of smart pointer alternatives available, see this article for good explanation: [6]

In general there is at least two basic alternatives here.

  1. use smart pointer libraries provided by boost: [7]
    • advantage here is to use ready made, proven libraries. Con is tighter dependency to boost and complexity (includes stuff that is not needed by fife)
  2. create fife specific smart pointers for fife specific needs.
    • advantages are opposite than boost specific approach; Libraries would not be so proven and we would have to make those ourselves (though code amount is not so big). Advantage is that pointers would contain only the stuff that fife needs, and we would have one dependency less.

In case of boost libraries would be used, best alternative would probably be the one used in pre-2007.2. Basic idea was to use reference counting pointers and let the memory be freed when count reaches to zero. Cycles were (at least attempted) to be broken with weak_ptrs.

In case of fife specific pointers, we could use ref counting solution. Another option could be stronger ownership model, where only one "strong pointer" could be alive at the time. It would be up to this pointer type to take care of freeing the memory. Weak pointers would be used to refer to this memory and handle cases where memory has been freed (exception would be thrown).

Engine memory smartptr.png

Pros

  • Unit testing is possible
  • Tests not verbose
  • In case of reference counting pointers are used, both c++ and scripting sides (python) would share similar memory management model (python does refcounting too)
  • In case strong/weak idiom with strict ownership would be used, deletion would be deterministic like with plain pointers. Also cyclic reference problem should be avoidable
  • Would mostly solve scripting side segfault cases (in case access to instances would be offered through weak pointers)

Cons

  • With refcounting there is no clear memory ownership, might result some timing dependent bugs that are hard to track
    • e.g. two clients unref the pointer in different order causing deallocation to happen in different place in the sequence. Might get even more hairy in case deallocated instance does some operations in it's destructor
  • With refcounting its easy to end up with cyclic references, i.e. memory leaks
  • More difficult to integrate with swig interfaces
  • Potentially more complex code, especially when multiple smart pointer types are mixed together
  • Cannot be used throughout the engine, at least guichan does not fit into this model

Engine Extensions wrapper library

With engine extensions wrapper library all scripting side classes would have additional wrapping counterparts (even in addition to swig generated ones). That would allow hiding the memory management details from client side, but still manage memory with script code (via thisown mechanism). This solution does not enforce any of the engine side memory management solutions.

Engine memory exts.png

Pros

  • Every possible scripting side benefit could potentially be listed here, since custom solution could be written case by case with scripting language
  • Possibility to create flexible, easy to use apis
  • Possibility to create scripting side memory management solution, and change that more easily in case there is a reason for it
  • Additional layer of abstraction potentially gives more stability to game APIs
  • Gives proper hooks for API documentation
  • Does not set rules how core should manage its memory

Cons

  • Introduces additional class layer (more code)
    • Makes also maintenance harder due to multiple change points
  • Makes usage of other scripting languages harder (due to bigger extensions layer)
  • Does not set rules how core should manage its memory

Runtime Python side class / instance modifications

Python examples

The following code can be placed into techdemo file, or to fife.py created by swig

# exception to raise on invalid access
class DeletedFifeObjectException(Exception):
	def __repr__(self):
		return 'Accessed object has been already deleted in the engine'

def deleted_raiser(*args):
	raise DeletedFifeObjectException()
	
def invalid_repr(*args):
	return 'Object deleted from the fife engine side'

# this kind of method could potentially be used to invalidate given class instance
def invalidate_fife_obj(obj):
	obj.__setattr__ = deleted_raiser
	obj.__getattr__ = deleted_raiser
	obj.__delattr__ = deleted_raiser
	obj.__repr__ = invalid_repr
	obj.__str__ = invalid_repr


# example how we could potentially wrap __getattribute__ calls. Seems to fail in some cases
# method not complete, just an example of mechanism
def wrap_getattr(self, attr):
	a = object.__getattribute__
	try:
		print 'attribute access for class %s, id %d' % (a(self, '__class__'), a(self, 'getFifeId')())
		pass
	except TypeError:
		pass
	return object.__getattribute__(self, attr)

# example how we could potentially wrap instance creations (also done by swig interface which does not call __init__ always).
# seems to fail in some cases (getFifeId wasn't working for some reason)
# method not complete, just an example of mechanism
def _wrap_new(cls):
	instance = super(cls, cls).__new__(cls)
	print type(instance)
	print issubclass(type(instance), fife.FifeClass)
	instance.__fifeid__ = instance.getFifeId()
	return instance

# modify classes generated by swig. The intent here is to modify all classes inheriting from FifeClass
# so that all calls to mutators, accessors, constructors and destructors would be wrapped.
# FifeClass in engine side would maintain list of live instance ids. When instance is accessed
# from python side, we would check if matching id is alive in the engine. That requires
# storing of FifeClass id in python side proxy instance during its creation.
for k,v in fife.__dict__.items():
	try:
		if issubclass(v, fife.FifeClass) and v is not fife.FifeClass:
			print k
			v.__getattribute__ = wrap_getattr
			#v.__old_setattr__ = v.__setattr__
			#v.__setattr__ = wrap_setattr
			v.__oldnew__ = v.__new__
			v.__new__ = staticmethod(_wrap_new)
			
	except TypeError:
		pass

...
...

in .i files, you can add python specific code to interfaces with the following mechanism (example)
	%feature("pythonprepend") Map::deleteElevation(Elevation* elevation) %{
		elevation = args[1]
		invalidate_fife_obj(elevation)
	%}

Related patches

-> modifications to FifeClass

Index: engine/core/util/fifeclass.cpp
===================================================================
--- engine/core/util/fifeclass.cpp	(revision 1861)
+++ engine/core/util/fifeclass.cpp	(working copy)
@@ -31,4 +31,17 @@
 
 namespace FIFE {
 	fifeid_t FifeClass::m_curid = 0;
+	std::set<fifeid_t> FifeClass::m_liveinstances;
+	
+	FifeClass::FifeClass(): m_fifeid(m_curid) {
+		m_liveinstances.insert(m_curid++);
+	}
+	
+	FifeClass::~FifeClass() {
+		m_liveinstances.erase(m_fifeid);
+	}
+	
+	bool FifeClass::isAlive(fifeid_t id) {
+		return m_liveinstances.find(id) != m_liveinstances.end();
+	}
 }//FIFE
Index: engine/core/util/fifeclass.h
===================================================================
--- engine/core/util/fifeclass.h	(revision 1861)
+++ engine/core/util/fifeclass.h	(working copy)
@@ -23,6 +23,7 @@
 #define FIFE_CLASS_H
 
 // Standard C++ library includes
+#include <set>
 
 // 3rd party library includes
 
@@ -40,17 +41,20 @@
 	 */
 	class FifeClass {
 	public:
-		FifeClass(): m_fifeid(m_curid++) { }
+		FifeClass();
 		
-		virtual ~FifeClass() {}
+		virtual ~FifeClass();
 		
 		fifeid_t getFifeId() {
 			return m_fifeid;
 		}
 		
+		static bool isAlive(fifeid_t id);
+		
 	private:
 		fifeid_t m_fifeid;
 		static fifeid_t m_curid;
+		static std::set<fifeid_t> m_liveinstances;
 	};
 }
 
Index: engine/core/util/fifeclass.i
===================================================================
--- engine/core/util/fifeclass.i	(revision 1861)
+++ engine/core/util/fifeclass.i	(working copy)
@@ -10,5 +10,6 @@
 	public:
 		virtual ~FifeClass();
 		fifeid_t getFifeId();
+		static bool isAlive(fifeid_t id);
 	};
 }