PLUGINS: improved one-at-a-time plugin code

I reduced memory fragmentation using 2 principles: Plugins should be loaded for as little time as possible, and long lasting memory allocations should be allocated before plugins are loaded. There might still be a little fragmentation left.
Note that command line settings that require plugins to be loaded don't work yet, but they didn't work (properly) before either.

svn-id: r54097

5 files changed, 99 insertions, 90 deletions

diff --git a/base/main.cpp b/base/main.cpp
index 8928396642..a32589e701 100644
--- a/base/main.cpp
+++ b/base/main.cpp
@@ -107,7 +107,7 @@ static const EnginePlugin *detectPlugin() {
 	printf("%s", "  Looking for a plugin supporting this gameid... ");
 
 #if defined(ONE_PLUGIN_AT_A_TIME) && defined(DYNAMIC_MODULES)
-	GameDescriptor game = EngineMan.findGameOnePlugAtATime(gameid, &plugin);
+	GameDescriptor game = EngineMan.findGameOnePluginAtATime(gameid, &plugin);
 #else
  	GameDescriptor game = EngineMan.findGame(gameid, &plugin);
 #endif
@@ -216,6 +216,11 @@ static Common::Error runGame(const EnginePlugin *plugin, OSystem &system, const
 	// Run the engine
 	Common::Error result = engine->run();
 
+#if defined(ONE_PLUGIN_AT_A_TIME) && defined(DYNAMIC_MODULES)
+	// do our best to prevent fragmentation by unloading as soon as we can
+	PluginManager::instance().unloadPluginsExcept(PLUGIN_TYPE_ENGINE, NULL, false);
+#endif	
+	
 	// Inform backend that the engine finished
 	system.engineDone();
 
@@ -343,7 +348,7 @@ extern "C" int scummvm_main(int argc, const char * const argv[]) {
 
 #if defined(ONE_PLUGIN_AT_A_TIME) && defined(DYNAMIC_MODULES)
 	// Only load non-engine plugins and first engine plugin initially in this case.
-	PluginManager::instance().loadFirstPlugin();
+	PluginManager::instance().loadNonEnginePluginsAndEnumerate();
 #else
  	// Load the plugins.
  	PluginManager::instance().loadPlugins();
@@ -363,6 +368,7 @@ extern "C" int scummvm_main(int argc, const char * const argv[]) {
 	// config file and the plugins have been loaded.
 	Common::Error res;
 
+	// TODO: deal with settings that require plugins to be loaded
 	if ((res = Base::processSettings(command, settings)) != Common::kArgumentNotProcessed)
 		return res;
 
@@ -431,9 +437,7 @@ extern "C" int scummvm_main(int argc, const char * const argv[]) {
 
 			// PluginManager::instance().unloadPlugins();
 
-#if defined(ONE_PLUGIN_AT_A_TIME) && defined(DYNAMIC_MODULES)
-			PluginManager::instance().loadFirstPlugin();
-#else
+#if !defined(ONE_PLUGIN_AT_A_TIME)
 			PluginManager::instance().loadPlugins();
 #endif
 		} else {
diff --git a/base/plugins.cpp b/base/plugins.cpp
index d2fad34579..41e213a53a 100644
--- a/base/plugins.cpp
+++ b/base/plugins.cpp
@@ -305,7 +305,6 @@ DECLARE_SINGLETON(PluginManager)
 PluginManager::PluginManager() {
 	// Always add the static plugin provider.
 	addPluginProvider(new StaticPluginProvider());
-	_skipStaticPlugs = false;
 }
 
 PluginManager::~PluginManager() {
@@ -324,58 +323,60 @@ void PluginManager::addPluginProvider(PluginProvider *pp) {
 	_providers.push_back(pp);
 }
 
-void PluginManager::loadFirstPlugin() { //TODO: rename? It's not quite clear that this loads all non-engine plugins and first engine plugin.
-	unloadPluginsExcept(PLUGIN_TYPE_ENGINE, NULL);
-	_allPlugs.clear();
+//
+// This should only be run once
+void PluginManager::loadNonEnginePluginsAndEnumerate() {
+	unloadPlugins();
+	_allEnginePlugins.clear();
+	
+	// We need to resize our pluginsInMem list to prevent fragmentation
+	// Otherwise, as soon as we add our 1 engine plugin (which is all we'll have in memory at a time)
+	// We'll get an allocation in memory that will never go away
+	_pluginsInMem[PLUGIN_TYPE_ENGINE].resize(2);	// more than we need
+
 	for (ProviderList::iterator pp = _providers.begin();
 	                            pp != _providers.end();
 	                            ++pp) {
-		if ((_skipStaticPlugs && (*pp)->isFilePluginProvider()) || !_skipStaticPlugs) {
-			PluginList pl((*pp)->getPlugins());
-			for (PluginList::iterator p = pl.begin(); p != pl.end(); ++p) {
-				_allPlugs.push_back(*p);
-			}
-		}
-	}
-
-	_nonEnginePlugs = 0;
-	_skipStaticPlugs = true; //Only need to load static plugins once.
-
-	_currentPlugin = _allPlugs.begin();
-
-	if (_allPlugs.empty()) {
-		return; //return here if somehow there are no plugins to load.
-	}
+		PluginList pl((*pp)->getPlugins());
+		for (PluginList::iterator p = pl.begin(); p != pl.end(); ++p) {
+			// To find out which are engine plugins, we have to load them. This is inefficient
+			// Hopefully another way can be found (e.g. if the music plugins are all static, 
+			// we can use only the static provider
+			if ((*p)->loadPlugin()) {
+				if ((*p)->getType() == PLUGIN_TYPE_ENGINE) {
+					(*p)->unloadPlugin();				// to prevent fragmentation
+					_allEnginePlugins.push_back(*p);
+				} else {	// add non-engine plugins to the 'in-memory' list
+							// these won't ever get unloaded (in this implementation)
+					addToPluginsInMemList(*p);			
+				}
+			}	
+ 		}
+ 	}
+}
 
-	//this loop is for loading all non-engine plugins and the first engine plugin.
-	for (; _currentPlugin != _allPlugs.end(); ++_currentPlugin) {
-		if (!tryLoadPlugin(*_currentPlugin))
-			continue;
+void PluginManager::loadFirstPlugin() { 
+	unloadPluginsExcept(PLUGIN_TYPE_ENGINE, NULL, false);
 
-		// TODO: This assumes all non-engine plugins will precede the first engine plugin!
-		if ((*_currentPlugin)->getType() == PLUGIN_TYPE_ENGINE)
+	// let's try to find one we can load
+	for (_currentPlugin = _allEnginePlugins.begin(); _currentPlugin != _allEnginePlugins.end(); ++_currentPlugin) {
+		if ((*_currentPlugin)->loadPlugin()) {
+			addToPluginsInMemList(*_currentPlugin);
 			break;
-
-		_nonEnginePlugs++;
+		}
 	}
-
-	return;
 }
 
 bool PluginManager::loadNextPlugin() {
-	if (_nonEnginePlugs == _allPlugs.size())
-		return false; //There are no Engine Plugins in this case.
+	unloadPluginsExcept(PLUGIN_TYPE_ENGINE, NULL, false);
 
-	// To ensure only one engine plugin is loaded at a time, we unload all engine plugins before trying to load a new one.
-	unloadPluginsExcept(PLUGIN_TYPE_ENGINE, NULL);
-
-	++_currentPlugin;
-	for (; _currentPlugin != _allPlugs.end(); ++_currentPlugin)
-		if (tryLoadPlugin(*_currentPlugin))
+	for (++_currentPlugin; _currentPlugin != _allEnginePlugins.end(); ++_currentPlugin) {
+		if ((*_currentPlugin)->loadPlugin()) {
+			addToPluginsInMemList(*_currentPlugin);
 			return true;
-
-	loadFirstPlugin();
-	return false;
+		}
+	}
+	return false;	// no more in list
 }
 
 void PluginManager::loadPlugins() {
@@ -392,19 +393,20 @@ void PluginManager::unloadPlugins() {
 		unloadPluginsExcept((PluginType)i, NULL);
 }
 
-void PluginManager::unloadPluginsExcept(PluginType type, const Plugin *plugin) {
+void PluginManager::unloadPluginsExcept(PluginType type, const Plugin *plugin, bool deletePlugin /*=true*/) {
 	Plugin *found = NULL;
-	for (PluginList::iterator p = _plugins[type].begin(); p != _plugins[type].end(); ++p) {
+	for (PluginList::iterator p = _pluginsInMem[type].begin(); p != _pluginsInMem[type].end(); ++p) {
 		if (*p == plugin) {
 			found = *p;
 		} else {
 			(*p)->unloadPlugin();
-			delete *p;
+			if (deletePlugin)
+				delete *p;
 		}
 	}
-	_plugins[type].clear();
+	_pluginsInMem[type].clear();
 	if (found != NULL) {
-		_plugins[type].push_back(found);
+		_pluginsInMem[type].push_back(found);
 	}
 }
 
@@ -412,26 +414,7 @@ bool PluginManager::tryLoadPlugin(Plugin *plugin) {
 	assert(plugin);
 	// Try to load the plugin
 	if (plugin->loadPlugin()) {
-		// The plugin is valid, see if it provides the same module as an
-		// already loaded one and should replace it.
-		bool found = false;
-		PluginList::iterator pl = _plugins[plugin->getType()].begin();
-		while (!found && pl != _plugins[plugin->getType()].end()) {
-			if (!strcmp(plugin->getName(), (*pl)->getName())) {
-				// Found a duplicated module. Replace the old one.
-				found = true;
-				delete *pl;
-				*pl = plugin;
-				debug(1, "Replaced the duplicated plugin: '%s'", plugin->getName());
-			}
-			pl++;
-		}
-
-		if (!found) {
-			// If it provides a new module, just add it to the list of known plugins.
-			_plugins[plugin->getType()].push_back(plugin);
-		}
-
+		addToPluginsInMemList(plugin);
 		return true;
 	} else {
 		// Failed to load the plugin
@@ -440,15 +423,38 @@ bool PluginManager::tryLoadPlugin(Plugin *plugin) {
 	}
 }
 
+void PluginManager::addToPluginsInMemList(Plugin *plugin) {
+	bool found = false;
+	// The plugin is valid, see if it provides the same module as an
+	// already loaded one and should replace it.
+		
+	PluginList::iterator pl = _pluginsInMem[plugin->getType()].begin();
+	while (!found && pl != _pluginsInMem[plugin->getType()].end()) {
+		if (!strcmp(plugin->getName(), (*pl)->getName())) {
+			// Found a duplicated module. Replace the old one.
+			found = true;
+			delete *pl;
+			*pl = plugin;
+			debug(1, "Replaced the duplicated plugin: '%s'", plugin->getName());
+		}
+		pl++;
+	}
+
+	if (!found) {
+		// If it provides a new module, just add it to the list of known plugins in memory.
+		_pluginsInMem[plugin->getType()].push_back(plugin);
+	}
+}
+
 // Engine plugins
 
 #include "engines/metaengine.h"
 
 DECLARE_SINGLETON(EngineManager)
 
-GameDescriptor EngineManager::findGameOnePlugAtATime(const Common::String &gameName, const EnginePlugin **plugin) const {
+GameDescriptor EngineManager::findGameOnePluginAtATime(const Common::String &gameName, const EnginePlugin **plugin) const {
 	GameDescriptor result;
-	//PluginManager::instance().loadFirstPlugin();
+	PluginManager::instance().loadFirstPlugin();
 	do {
 		result = findGame(gameName, plugin); 
 		if (!result.gameid().empty()) {
@@ -468,14 +474,14 @@ GameDescriptor EngineManager::findGame(const Common::String &gameName, const Eng
 
 	EnginePlugin::List::const_iterator iter;
 	
-		for (iter = plugins.begin(); iter != plugins.end(); ++iter) {
-			result = (**iter)->findGame(gameName.c_str());
-			if (!result.gameid().empty()) {
-				if (plugin)
-					*plugin = *iter;
-				return result;
-			}
+	for (iter = plugins.begin(); iter != plugins.end(); ++iter) {
+		result = (**iter)->findGame(gameName.c_str());
+		if (!result.gameid().empty()) {
+			if (plugin)
+				*plugin = *iter;
+			return result;
 		}
+	}
 	return result;
 }
 
@@ -484,6 +490,7 @@ GameList EngineManager::detectGames(const Common::FSList &fslist) const {
 	EnginePlugin::List plugins;
 	EnginePlugin::List::const_iterator iter;
 #if defined(ONE_PLUGIN_AT_A_TIME) && defined(DYNAMIC_MODULES)
+	PluginManager::instance().loadFirstPlugin();
 	do {
 #endif
 		plugins = getPlugins();
diff --git a/base/plugins.h b/base/plugins.h
index 5472ab2fce..fc8adbf0d4 100644
--- a/base/plugins.h
+++ b/base/plugins.h
@@ -301,17 +301,14 @@ protected:
 class PluginManager : public Common::Singleton<PluginManager> {
 	typedef Common::Array<PluginProvider *> ProviderList;
 private:
-	PluginList _plugins[PLUGIN_TYPE_MAX];
+	PluginList _pluginsInMem[PLUGIN_TYPE_MAX];
 	ProviderList _providers;
 
-	PluginList _allPlugs;
+	PluginList _allEnginePlugins;
 	PluginList::iterator _currentPlugin;
 
-	bool _skipStaticPlugs;
-
-	uint _nonEnginePlugs;
-
 	bool tryLoadPlugin(Plugin *plugin);
+	void addToPluginsInMemList(Plugin *plugin);
 	
 	friend class Common::Singleton<SingletonBaseType>;
 	PluginManager();
@@ -321,14 +318,15 @@ public:
 
 	void addPluginProvider(PluginProvider *pp);
 
+	void loadNonEnginePluginsAndEnumerate();	
 	void loadFirstPlugin();
 	bool loadNextPlugin();
 	
 	void loadPlugins();
 	void unloadPlugins();
-	void unloadPluginsExcept(PluginType type, const Plugin *plugin);
+	void unloadPluginsExcept(PluginType type, const Plugin *plugin, bool deletePlugin = true);
 
-	const PluginList &getPlugins(PluginType t) { return _plugins[t]; }
+	const PluginList &getPlugins(PluginType t) { return _pluginsInMem[t]; }
 };
 
 #endif
diff --git a/engines/metaengine.h b/engines/metaengine.h
index 964eee071c..7b69a3fe43 100644
--- a/engines/metaengine.h
+++ b/engines/metaengine.h
@@ -231,7 +231,7 @@ private:
 	friend class Common::Singleton<SingletonBaseType>;
 
 public:
-	GameDescriptor findGameOnePlugAtATime(const Common::String &gameName, const EnginePlugin **plugin = NULL) const;
+	GameDescriptor findGameOnePluginAtATime(const Common::String &gameName, const EnginePlugin **plugin = NULL) const;
 	GameDescriptor findGame(const Common::String &gameName, const EnginePlugin **plugin = NULL) const;
 	GameList detectGames(const Common::FSList &fslist) const;
 	const EnginePlugin::List &getPlugins() const;
diff --git a/gui/launcher.cpp b/gui/launcher.cpp
index b56591b8c4..adbc418e87 100644
--- a/gui/launcher.cpp
+++ b/gui/launcher.cpp
@@ -925,7 +925,7 @@ void LauncherDialog::loadGame(int item) {
 	const EnginePlugin *plugin = 0;
 	
 #if defined(ONE_PLUGIN_AT_A_TIME) && defined(DYNAMIC_MODULES)
-	EngineMan.findGameOnePlugAtATime(gameId, &plugin);
+	EngineMan.findGameOnePluginAtATime(gameId, &plugin);
 #else
 	EngineMan.findGame(gameId, &plugin);
 #endif