From 16ac349e8b48c901d97e28d78e34f0f61af0fef3 Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Thu, 25 Jun 2015 13:59:24 +0100 Subject: [PATCH 1/5] Multithread plugin loading more effectively. Using hardware concurrency instead of the pretty arbitrary mean file size criterion previously used. File sizes are now used to load the threads as evenly as possible. While this coincidently doesn't have much effect on my computer (4 threads now vs. 5 before), it should help performance for users with CPUs that have less hardware concurrency support. --- src/backend/game/game.cpp | 83 +++++++++++++++++++++------------------ 1 file changed, 44 insertions(+), 39 deletions(-) diff --git a/src/backend/game/game.cpp b/src/backend/game/game.cpp index 5fd263a3..918c8a9c 100644 --- a/src/backend/game/game.cpp +++ b/src/backend/game/game.cpp @@ -120,63 +120,68 @@ namespace loot { } void Game::LoadPlugins(bool headersOnly) { - vector threads; uintmax_t meanFileSize = 0; - unordered_map tempMap; - std::vector groupPlugins; - //First calculate the mean plugin size. Store it temporarily in a map to reduce filesystem lookups and file size recalculation. + multimap sizeMap; + + // First find out how many plugins there are, and their sizes. BOOST_LOG_TRIVIAL(trace) << "Scanning for plugins in " << this->DataPath(); for (fs::directory_iterator it(this->DataPath()); it != fs::directory_iterator(); ++it) { if (fs::is_regular_file(it->status()) && Plugin(it->path().filename().string()).IsValid(*this)) { + Plugin temp(it->path().filename().string()); + BOOST_LOG_TRIVIAL(info) << "Found plugin: " << temp.Name(); + uintmax_t fileSize = fs::file_size(it->path()); meanFileSize += fileSize; - tempMap.insert(pair(it->path().filename().string(), fileSize)); + //Insert the lowercased name as a key for case-insensitive matching. + string name = boost::locale::to_lower(temp.Name()); + plugins.insert(pair(name, temp)); + sizeMap.insert(pair(fileSize, name)); } } - meanFileSize /= tempMap.size(); //Rounding error, but not important. + meanFileSize /= sizeMap.size(); //Rounding error, but not important. - //Now load plugins. - for (const auto &pluginPair : tempMap) { - BOOST_LOG_TRIVIAL(info) << "Found plugin: " << pluginPair.first; + // Get the number of threads to use. + // hardware_concurrency() may be zero, if so then use only one thread. + size_t threadsToUse = std::min((size_t)thread::hardware_concurrency(), plugins.size()); + threadsToUse = std::max(threadsToUse, (size_t)1); - //Insert the lowercased name as a key for case-insensitive matching. - Plugin temp(pluginPair.first); - auto plugin = plugins.insert(pair(boost::locale::to_lower(temp.Name()), temp)); + // Divide the plugins up by thread. + unsigned int pluginsPerThread = ceil((double)plugins.size() / threadsToUse); + vector::iterator>> pluginGroups(threadsToUse); + BOOST_LOG_TRIVIAL(info) << "Loading " << plugins.size() << " plugins using " << threadsToUse << " threads, with up to " << pluginsPerThread << " plugins per thread."; - if (pluginPair.second > meanFileSize) { - BOOST_LOG_TRIVIAL(trace) << "Creating individual loading thread for: " << pluginPair.first; - threads.push_back(thread([this, plugin, headersOnly]() { - BOOST_LOG_TRIVIAL(trace) << "Loading " << plugin.first->second.Name() << " individually."; + // The plugins should be split between the threads so that the data + // load is as evenly spread as possible. + size_t currentGroup = 0; + for (const auto& plugin : sizeMap) { + if (currentGroup == threadsToUse) + currentGroup = 0; + BOOST_LOG_TRIVIAL(trace) << "Adding plugin " << plugin.second << " to loading group " << currentGroup; + pluginGroups[currentGroup].push_back(plugins.find(plugin.second)); + ++currentGroup; + } + + // Load the plugins. + BOOST_LOG_TRIVIAL(trace) << "Starting plugin loading."; + vector threads; + while (threads.size() < threadsToUse) { + vector::iterator>& pluginGroup = pluginGroups[threads.size()]; + threads.push_back(thread([this, &pluginGroup, headersOnly]() { + for (auto it : pluginGroup) { + BOOST_LOG_TRIVIAL(trace) << "Loading " << it->second.Name(); try { - plugin.first->second = Plugin(*this, plugin.first->second.Name(), headersOnly); + it->second = Plugin(*this, it->second.Name(), headersOnly); } catch (exception &e) { - BOOST_LOG_TRIVIAL(error) << plugin.first->second.Name() << ": Exception occurred: " << e.what(); - Plugin p; + BOOST_LOG_TRIVIAL(error) << it->second.Name() << ": Exception occurred: " << e.what(); + Plugin p(it->second.Name()); p.Messages(list(1, Message(Message::error, lc::translate("An exception occurred while loading this plugin. Details:").str() + " " + e.what()))); - plugin.first->second = p; + it->second = p; } - })); - } - else { - groupPlugins.push_back(&plugin.first->second); - } + } + })); } - threads.push_back(thread([this, &groupPlugins, headersOnly]() { - for (auto plugin : groupPlugins) { - BOOST_LOG_TRIVIAL(trace) << "Loading " << plugin->Name() << " as part of a group."; - try { - *plugin = Plugin(*this, plugin->Name(), headersOnly); - } - catch (exception &e) { - BOOST_LOG_TRIVIAL(error) << plugin->Name() << ": Exception occurred: " << e.what(); - Plugin p; - p.Messages(list(1, Message(Message::error, lc::translate("An exception occurred while loading this plugin. Details:").str() + " " + e.what()))); - *plugin = p; - } - } - })); // Join all threads. for (auto& thread : threads) { From debae8c4cdcd2e5785382dff19276545a20a4226 Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Tue, 14 Jul 2015 23:27:39 +0100 Subject: [PATCH 2/5] Plugin class code tidy. Just assign Form IDs, remove unused member function, and move NumOverrideFormIDs() declaration to data accessors block. --- src/backend/plugin/plugin.cpp | 11 +---------- src/backend/plugin/plugin.h | 3 +-- 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/src/backend/plugin/plugin.cpp b/src/backend/plugin/plugin.cpp index 1d20153b..88da030e 100644 --- a/src/backend/plugin/plugin.cpp +++ b/src/backend/plugin/plugin.cpp @@ -102,7 +102,7 @@ namespace loot { isMaster = loader.IsMaster(); masters = loader.Masters(); _isEmpty = loader.IsEmpty(); - formIDs.insert(loader.FormIDs().begin(), loader.FormIDs().end()); + formIDs = loader.FormIDs(); if (!headerOnly) { BOOST_LOG_TRIVIAL(trace) << name << ": Caching CRC value."; @@ -203,15 +203,6 @@ namespace loot { return overlap; } - std::set Plugin::OverrideFormIDs() const { - set fidSubset; - for (const auto &formID : formIDs) { - if (!boost::iequals(formID.Plugin(), name)) - fidSubset.insert(formID); - } - return fidSubset; - } - std::vector Plugin::Masters() const { return masters; } diff --git a/src/backend/plugin/plugin.h b/src/backend/plugin/plugin.h index 56f34744..0118c65e 100644 --- a/src/backend/plugin/plugin.h +++ b/src/backend/plugin/plugin.h @@ -50,6 +50,7 @@ namespace loot { bool IsEmpty() const; std::string Version() const; uint32_t Crc() const; + size_t NumOverrideFormIDs() const; bool LoadsBSA(const Game& game) const; bool IsValid(const Game& game) const; @@ -61,9 +62,7 @@ namespace loot { //Load ordering functions. bool DoFormIDsOverlap(const Plugin& plugin) const; - size_t NumOverrideFormIDs() const; std::set OverlapFormIDs(const Plugin& plugin) const; - std::set OverrideFormIDs() const; //Validity checks. bool CheckInstallValidity(const Game& game); //Checks that reqs and masters are all present, and that no incs are present. Returns true if the plugin is dirty. From ff885545b2948dd0c9c835e5e5448c31e713d6c2 Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Tue, 14 Jul 2015 23:29:54 +0100 Subject: [PATCH 3/5] Fully test Game::LoadPlugins(). It's long-winded, but just because a single plugin's loader works, doesn't necessarily mean that they'll all get loaded into the Game data correctly. --- src/tests/backend/game/test_game.h | 357 ++++++++++++++++++++++++++++- 1 file changed, 353 insertions(+), 4 deletions(-) diff --git a/src/tests/backend/game/test_game.h b/src/tests/backend/game/test_game.h index 9cd8c9b2..b0ba8fbf 100644 --- a/src/tests/backend/game/test_game.h +++ b/src/tests/backend/game/test_game.h @@ -217,11 +217,360 @@ TEST_F(Game, LoadPlugins) { loot::Game game(loot::Game::tes5); game.SetGamePath(dataPath.parent_path()); - // Try loading only plugin headers first. - EXPECT_NO_THROW(game.LoadPlugins(true)); - - // Test fullly loading plugins. EXPECT_NO_THROW(game.LoadPlugins(false)); + EXPECT_EQ(11, game.plugins.size()); + + // Check that all the plugins' data have loaded correctly. + ASSERT_NE(game.plugins.end(), game.plugins.find("skyrim.esm")); + loot::Plugin plugin = game.plugins.find("skyrim.esm")->second; + EXPECT_EQ("Skyrim.esm", plugin.Name()); + EXPECT_FALSE(plugin.IsEmpty()); + EXPECT_TRUE(plugin.IsMaster()); + EXPECT_EQ(std::set({ + loot::FormID("Skyrim.esm", 0xCF0), + loot::FormID("Skyrim.esm", 0xCF1), + loot::FormID("Skyrim.esm", 0xCF2), + loot::FormID("Skyrim.esm", 0xCF3), + loot::FormID("Skyrim.esm", 0xCF4), + loot::FormID("Skyrim.esm", 0xCF5), + loot::FormID("Skyrim.esm", 0xCF6), + loot::FormID("Skyrim.esm", 0xCF7), + loot::FormID("Skyrim.esm", 0xCF8), + loot::FormID("Skyrim.esm", 0xCF9), + }), plugin.FormIDs()); + EXPECT_TRUE(plugin.Masters().empty()); + EXPECT_EQ("5.0", plugin.Version()); + EXPECT_EQ(0xD33753E4, plugin.Crc()); + EXPECT_EQ(0, plugin.NumOverrideFormIDs()); + + ASSERT_NE(game.plugins.end(), game.plugins.find("blank.esm")); + plugin = game.plugins.find("blank.esm")->second; + EXPECT_EQ("Blank.esm", plugin.Name()); + EXPECT_FALSE(plugin.IsEmpty()); + EXPECT_TRUE(plugin.IsMaster()); + EXPECT_EQ(std::set({ + loot::FormID("Blank.esm", 0xCF0), + loot::FormID("Blank.esm", 0xCF1), + loot::FormID("Blank.esm", 0xCF2), + loot::FormID("Blank.esm", 0xCF3), + loot::FormID("Blank.esm", 0xCF4), + loot::FormID("Blank.esm", 0xCF5), + loot::FormID("Blank.esm", 0xCF6), + loot::FormID("Blank.esm", 0xCF7), + loot::FormID("Blank.esm", 0xCF8), + loot::FormID("Blank.esm", 0xCF9), + }), plugin.FormIDs()); + EXPECT_TRUE(plugin.Masters().empty()); + EXPECT_EQ("5.0", plugin.Version()); + EXPECT_EQ(0xD33753E4, plugin.Crc()); + EXPECT_EQ(0, plugin.NumOverrideFormIDs()); + + ASSERT_NE(game.plugins.end(), game.plugins.find("blank - different.esm")); + plugin = game.plugins.find("blank - different.esm")->second; + EXPECT_EQ("Blank - Different.esm", plugin.Name()); + EXPECT_FALSE(plugin.IsEmpty()); + EXPECT_TRUE(plugin.IsMaster()); + EXPECT_EQ(std::set({ + loot::FormID("Blank - Different.esm", 0xCEF), + loot::FormID("Blank - Different.esm", 0xCF0), + loot::FormID("Blank - Different.esm", 0xCF1), + loot::FormID("Blank - Different.esm", 0xCF2), + loot::FormID("Blank - Different.esm", 0xCF3), + loot::FormID("Blank - Different.esm", 0xCF4), + loot::FormID("Blank - Different.esm", 0xCF5), + loot::FormID("Blank - Different.esm", 0xCF6), + loot::FormID("Blank - Different.esm", 0xCF7), + }), plugin.FormIDs()); + EXPECT_TRUE(plugin.Masters().empty()); + EXPECT_EQ("", plugin.Version()); + EXPECT_EQ(0x64B9F757, plugin.Crc()); + EXPECT_EQ(0, plugin.NumOverrideFormIDs()); + + ASSERT_NE(game.plugins.end(), game.plugins.find("blank - master dependent.esm")); + plugin = game.plugins.find("blank - master dependent.esm")->second; + EXPECT_EQ("Blank - Master Dependent.esm", plugin.Name()); + EXPECT_FALSE(plugin.IsEmpty()); + EXPECT_TRUE(plugin.IsMaster()); + EXPECT_EQ(std::set({ + loot::FormID("Blank.esm", 0xCF0), + loot::FormID("Blank.esm", 0xCF1), + loot::FormID("Blank.esm", 0xCF2), + loot::FormID("Blank.esm", 0xCF3), + loot::FormID("Blank - Master Dependent.esm", 0xCEA), + loot::FormID("Blank - Master Dependent.esm", 0xCEB), + loot::FormID("Blank - Master Dependent.esm", 0xCEC), + loot::FormID("Blank - Master Dependent.esm", 0xCED), + }), plugin.FormIDs()); + EXPECT_EQ(std::vector({ + "Blank.esm" + }), plugin.Masters()); + EXPECT_EQ("", plugin.Version()); + EXPECT_EQ(0xB2D4119E, plugin.Crc()); + EXPECT_EQ(4, plugin.NumOverrideFormIDs()); + + ASSERT_NE(game.plugins.end(), game.plugins.find("blank - different master dependent.esm")); + plugin = game.plugins.find("blank - different master dependent.esm")->second; + EXPECT_EQ("Blank - Different Master Dependent.esm", plugin.Name()); + EXPECT_FALSE(plugin.IsEmpty()); + EXPECT_TRUE(plugin.IsMaster()); + EXPECT_EQ(std::set({ + loot::FormID("Blank - Different.esm", 0xCEF), + loot::FormID("Blank - Different.esm", 0xCF0), + loot::FormID("Blank - Different.esm", 0xCF1), + loot::FormID("Blank - Different.esm", 0xCF2), + loot::FormID("Blank - Different Master Dependent.esm", 0xCE9), + loot::FormID("Blank - Different Master Dependent.esm", 0xCEA), + loot::FormID("Blank - Different Master Dependent.esm", 0xCEB), + }), plugin.FormIDs()); + EXPECT_EQ(std::vector({ + "Blank - Different.esm" + }), plugin.Masters()); + EXPECT_EQ("", plugin.Version()); + EXPECT_EQ(0xAADF6710, plugin.Crc()); + EXPECT_EQ(4, plugin.NumOverrideFormIDs()); + + ASSERT_NE(game.plugins.end(), game.plugins.find("blank.esp")); + plugin = game.plugins.find("blank.esp")->second; + EXPECT_EQ("Blank.esp", plugin.Name()); + EXPECT_FALSE(plugin.IsEmpty()); + EXPECT_FALSE(plugin.IsMaster()); + EXPECT_EQ(std::set({ + loot::FormID("Blank.esp", 0xCEC), + loot::FormID("Blank.esp", 0xCED), + loot::FormID("Blank.esp", 0xCEE), + loot::FormID("Blank.esp", 0xCEF), + loot::FormID("Blank.esp", 0xCF0), + loot::FormID("Blank.esp", 0xCF1), + }), plugin.FormIDs()); + EXPECT_TRUE(plugin.Masters().empty()); + EXPECT_EQ("", plugin.Version()); + EXPECT_EQ(0xE12EFAAA, plugin.Crc()); + EXPECT_EQ(0, plugin.NumOverrideFormIDs()); + + ASSERT_NE(game.plugins.end(), game.plugins.find("blank - different.esp")); + plugin = game.plugins.find("blank - different.esp")->second; + EXPECT_EQ("Blank - Different.esp", plugin.Name()); + EXPECT_FALSE(plugin.IsEmpty()); + EXPECT_FALSE(plugin.IsMaster()); + EXPECT_EQ(std::set({ + loot::FormID("Blank - Different.esp", 0xCEB), + loot::FormID("Blank - Different.esp", 0xCEC), + loot::FormID("Blank - Different.esp", 0xCED), + loot::FormID("Blank - Different.esp", 0xCEE), + loot::FormID("Blank - Different.esp", 0xCEF), + }), plugin.FormIDs()); + EXPECT_TRUE(plugin.Masters().empty()); + EXPECT_EQ("", plugin.Version()); + EXPECT_EQ(0xD4C9B7AE, plugin.Crc()); + EXPECT_EQ(0, plugin.NumOverrideFormIDs()); + + ASSERT_NE(game.plugins.end(), game.plugins.find("blank - master dependent.esp")); + plugin = game.plugins.find("blank - master dependent.esp")->second; + EXPECT_EQ("Blank - Master Dependent.esp", plugin.Name()); + EXPECT_FALSE(plugin.IsEmpty()); + EXPECT_FALSE(plugin.IsMaster()); + EXPECT_EQ(std::set({ + loot::FormID("Blank.esm", 0xCF0), + loot::FormID("Blank.esm", 0xCF1), + loot::FormID("Blank - Master Dependent.esp", 0xCE9), + loot::FormID("Blank - Master Dependent.esp", 0xCEA), + }), plugin.FormIDs()); + EXPECT_EQ(std::vector({ + "Blank.esm" + }), plugin.Masters()); + EXPECT_EQ("", plugin.Version()); + EXPECT_EQ(0x832152DC, plugin.Crc()); + EXPECT_EQ(2, plugin.NumOverrideFormIDs()); + + ASSERT_NE(game.plugins.end(), game.plugins.find("blank - different master dependent.esp")); + plugin = game.plugins.find("blank - different master dependent.esp")->second; + EXPECT_EQ("Blank - Different Master Dependent.esp", plugin.Name()); + EXPECT_FALSE(plugin.IsEmpty()); + EXPECT_FALSE(plugin.IsMaster()); + EXPECT_EQ(std::set({ + loot::FormID("Blank - Different.esm", 0xCEF), + loot::FormID("Blank - Different.esm", 0xCF0), + loot::FormID("Blank - Different Master Dependent.esp", 0xCE7), + }), plugin.FormIDs()); + EXPECT_EQ(std::vector({ + "Blank - Different.esm" + }), plugin.Masters()); + EXPECT_EQ("", plugin.Version()); + EXPECT_EQ(0x3AD17683, plugin.Crc()); + EXPECT_EQ(2, plugin.NumOverrideFormIDs()); + + ASSERT_NE(game.plugins.end(), game.plugins.find("blank - plugin dependent.esp")); + plugin = game.plugins.find("blank - plugin dependent.esp")->second; + EXPECT_EQ("Blank - Plugin Dependent.esp", plugin.Name()); + EXPECT_FALSE(plugin.IsEmpty()); + EXPECT_FALSE(plugin.IsMaster()); + EXPECT_EQ(std::set({ + loot::FormID("Blank.esp", 0xCEC), + loot::FormID("Blank - Plugin Dependent.esp", 0xCE7), + }), plugin.FormIDs()); + EXPECT_EQ(std::vector({ + "Blank.esp" + }), plugin.Masters()); + EXPECT_EQ("", plugin.Version()); + EXPECT_EQ(0x28EF26DB, plugin.Crc()); + EXPECT_EQ(1, plugin.NumOverrideFormIDs()); + + ASSERT_NE(game.plugins.end(), game.plugins.find("blank - different plugin dependent.esp")); + plugin = game.plugins.find("blank - different plugin dependent.esp")->second; + EXPECT_EQ("Blank - Different Plugin Dependent.esp", plugin.Name()); + EXPECT_FALSE(plugin.IsEmpty()); + EXPECT_FALSE(plugin.IsMaster()); + EXPECT_EQ(std::set({ + loot::FormID("Blank - Different.esp", 0xCEB), + }), plugin.FormIDs()); + EXPECT_EQ(std::vector({ + "Blank - Different.esp" + }), plugin.Masters()); + EXPECT_EQ("", plugin.Version()); + EXPECT_EQ(0xEB47BE63, plugin.Crc()); + EXPECT_EQ(1, plugin.NumOverrideFormIDs()); +} + +TEST_F(Game, LoadPlugins_HeadersOnly) { + loot::Game game(loot::Game::tes5); + game.SetGamePath(dataPath.parent_path()); + + EXPECT_NO_THROW(game.LoadPlugins(true)); + EXPECT_EQ(11, game.plugins.size()); + + // Check that all the plugins' data have loaded correctly. + ASSERT_NE(game.plugins.end(), game.plugins.find("skyrim.esm")); + loot::Plugin plugin = game.plugins.find("skyrim.esm")->second; + EXPECT_EQ("Skyrim.esm", plugin.Name()); + EXPECT_FALSE(plugin.IsEmpty()); + EXPECT_TRUE(plugin.IsMaster()); + EXPECT_TRUE(plugin.FormIDs().empty()); + EXPECT_TRUE(plugin.Masters().empty()); + EXPECT_EQ("5.0", plugin.Version()); + EXPECT_EQ(0, plugin.Crc()); + EXPECT_EQ(0, plugin.NumOverrideFormIDs()); + + ASSERT_NE(game.plugins.end(), game.plugins.find("blank.esm")); + plugin = game.plugins.find("blank.esm")->second; + EXPECT_EQ("Blank.esm", plugin.Name()); + EXPECT_FALSE(plugin.IsEmpty()); + EXPECT_TRUE(plugin.IsMaster()); + EXPECT_TRUE(plugin.FormIDs().empty()); + EXPECT_TRUE(plugin.Masters().empty()); + EXPECT_EQ("5.0", plugin.Version()); + EXPECT_EQ(0, plugin.Crc()); + EXPECT_EQ(0, plugin.NumOverrideFormIDs()); + + ASSERT_NE(game.plugins.end(), game.plugins.find("blank - different.esm")); + plugin = game.plugins.find("blank - different.esm")->second; + EXPECT_EQ("Blank - Different.esm", plugin.Name()); + EXPECT_FALSE(plugin.IsEmpty()); + EXPECT_TRUE(plugin.IsMaster()); + EXPECT_TRUE(plugin.FormIDs().empty()); + EXPECT_TRUE(plugin.Masters().empty()); + EXPECT_EQ("", plugin.Version()); + EXPECT_EQ(0, plugin.Crc()); + EXPECT_EQ(0, plugin.NumOverrideFormIDs()); + + ASSERT_NE(game.plugins.end(), game.plugins.find("blank - master dependent.esm")); + plugin = game.plugins.find("blank - master dependent.esm")->second; + EXPECT_EQ("Blank - Master Dependent.esm", plugin.Name()); + EXPECT_FALSE(plugin.IsEmpty()); + EXPECT_TRUE(plugin.IsMaster()); + EXPECT_TRUE(plugin.FormIDs().empty()); + EXPECT_EQ(std::vector({ + "Blank.esm" + }), plugin.Masters()); + EXPECT_EQ("", plugin.Version()); + EXPECT_EQ(0, plugin.Crc()); + EXPECT_EQ(0, plugin.NumOverrideFormIDs()); + + ASSERT_NE(game.plugins.end(), game.plugins.find("blank - different master dependent.esm")); + plugin = game.plugins.find("blank - different master dependent.esm")->second; + EXPECT_EQ("Blank - Different Master Dependent.esm", plugin.Name()); + EXPECT_FALSE(plugin.IsEmpty()); + EXPECT_TRUE(plugin.IsMaster()); + EXPECT_TRUE(plugin.FormIDs().empty()); + EXPECT_EQ(std::vector({ + "Blank - Different.esm" + }), plugin.Masters()); + EXPECT_EQ("", plugin.Version()); + EXPECT_EQ(0, plugin.Crc()); + EXPECT_EQ(0, plugin.NumOverrideFormIDs()); + + ASSERT_NE(game.plugins.end(), game.plugins.find("blank.esp")); + plugin = game.plugins.find("blank.esp")->second; + EXPECT_EQ("Blank.esp", plugin.Name()); + EXPECT_FALSE(plugin.IsEmpty()); + EXPECT_FALSE(plugin.IsMaster()); + EXPECT_TRUE(plugin.FormIDs().empty()); + EXPECT_TRUE(plugin.Masters().empty()); + EXPECT_EQ("", plugin.Version()); + EXPECT_EQ(0, plugin.Crc()); + EXPECT_EQ(0, plugin.NumOverrideFormIDs()); + + ASSERT_NE(game.plugins.end(), game.plugins.find("blank - different.esp")); + plugin = game.plugins.find("blank - different.esp")->second; + EXPECT_EQ("Blank - Different.esp", plugin.Name()); + EXPECT_FALSE(plugin.IsEmpty()); + EXPECT_FALSE(plugin.IsMaster()); + EXPECT_TRUE(plugin.FormIDs().empty()); + EXPECT_TRUE(plugin.Masters().empty()); + EXPECT_EQ("", plugin.Version()); + EXPECT_EQ(0, plugin.Crc()); + EXPECT_EQ(0, plugin.NumOverrideFormIDs()); + + ASSERT_NE(game.plugins.end(), game.plugins.find("blank - master dependent.esp")); + plugin = game.plugins.find("blank - master dependent.esp")->second; + EXPECT_EQ("Blank - Master Dependent.esp", plugin.Name()); + EXPECT_FALSE(plugin.IsEmpty()); + EXPECT_FALSE(plugin.IsMaster()); + EXPECT_TRUE(plugin.FormIDs().empty()); + EXPECT_EQ(std::vector({ + "Blank.esm" + }), plugin.Masters()); + EXPECT_EQ("", plugin.Version()); + EXPECT_EQ(0, plugin.Crc()); + EXPECT_EQ(0, plugin.NumOverrideFormIDs()); + + ASSERT_NE(game.plugins.end(), game.plugins.find("blank - different master dependent.esp")); + plugin = game.plugins.find("blank - different master dependent.esp")->second; + EXPECT_EQ("Blank - Different Master Dependent.esp", plugin.Name()); + EXPECT_FALSE(plugin.IsEmpty()); + EXPECT_FALSE(plugin.IsMaster()); + EXPECT_TRUE(plugin.FormIDs().empty()); + EXPECT_EQ(std::vector({ + "Blank - Different.esm" + }), plugin.Masters()); + EXPECT_EQ("", plugin.Version()); + EXPECT_EQ(0, plugin.Crc()); + EXPECT_EQ(0, plugin.NumOverrideFormIDs()); + + ASSERT_NE(game.plugins.end(), game.plugins.find("blank - plugin dependent.esp")); + plugin = game.plugins.find("blank - plugin dependent.esp")->second; + EXPECT_EQ("Blank - Plugin Dependent.esp", plugin.Name()); + EXPECT_FALSE(plugin.IsEmpty()); + EXPECT_FALSE(plugin.IsMaster()); + EXPECT_TRUE(plugin.FormIDs().empty()); + EXPECT_EQ(std::vector({ + "Blank.esp" + }), plugin.Masters()); + EXPECT_EQ("", plugin.Version()); + EXPECT_EQ(0, plugin.Crc()); + EXPECT_EQ(0, plugin.NumOverrideFormIDs()); + + ASSERT_NE(game.plugins.end(), game.plugins.find("blank - different plugin dependent.esp")); + plugin = game.plugins.find("blank - different plugin dependent.esp")->second; + EXPECT_EQ("Blank - Different Plugin Dependent.esp", plugin.Name()); + EXPECT_FALSE(plugin.IsEmpty()); + EXPECT_FALSE(plugin.IsMaster()); + EXPECT_TRUE(plugin.FormIDs().empty()); + EXPECT_EQ(std::vector({ + "Blank - Different.esp" + }), plugin.Masters()); + EXPECT_EQ("", plugin.Version()); + EXPECT_EQ(0, plugin.Crc()); + EXPECT_EQ(0, plugin.NumOverrideFormIDs()); } TEST_F(Game, ArePluginsFullyLoaded) { From 03d43c28f6b8d2ecc0470a9dba0e1bbe303a6e5a Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Wed, 15 Jul 2015 10:16:29 +0100 Subject: [PATCH 4/5] Add Google Test value printers. For easier debugging test failures. --- CMakeLists.txt | 1 + src/tests/fixtures.h | 1 + src/tests/printers.h | 113 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 115 insertions(+) create mode 100644 src/tests/printers.h diff --git a/CMakeLists.txt b/CMakeLists.txt index e43334c7..f4c107e2 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -156,6 +156,7 @@ set (LOOT_API_HEADERS ${LOOT_HEADERS} set (LOOT_TESTS_SRC "${CMAKE_SOURCE_DIR}/src/tests/main.cpp") set (LOOT_TESTS_HEADERS "${CMAKE_SOURCE_DIR}/src/tests/fixtures.h" + "${CMAKE_SOURCE_DIR}/src/tests/printers.h" "${CMAKE_SOURCE_DIR}/src/tests/api/test_api.h" "${CMAKE_SOURCE_DIR}/src/tests/backend/game/test_game.h" "${CMAKE_SOURCE_DIR}/src/tests/backend/game/test_game_settings.h" diff --git a/src/tests/fixtures.h b/src/tests/fixtures.h index 89770fb4..1213e191 100644 --- a/src/tests/fixtures.h +++ b/src/tests/fixtures.h @@ -26,6 +26,7 @@ along with LOOT. If not, see #define LOOT_TEST_FIXTURES #include "backend/helpers/streams.h" +#include "printers.h" #include diff --git a/src/tests/printers.h b/src/tests/printers.h new file mode 100644 index 00000000..1733c581 --- /dev/null +++ b/src/tests/printers.h @@ -0,0 +1,113 @@ +/* LOOT + +A load order optimisation tool for Oblivion, Skyrim, Fallout 3 and +Fallout: New Vegas. + +Copyright (C) 2013-2015 WrinklyNinja + +This file is part of LOOT. + +LOOT is free software: you can redistribute +it and/or modify it under the terms of the GNU General Public License +as published by the Free Software Foundation, either version 3 of +the License, or (at your option) any later version. + +LOOT is distributed in the hope that it will +be useful, but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +GNU General Public License for more details. + +You should have received a copy of the GNU General Public License +along with LOOT. If not, see +. +*/ + +#ifndef LOOT_TEST_PRINTERS +#define LOOT_TEST_PRINTERS + +#include + +#include + +#include "backend/metadata/file.h" +#include "backend/metadata/formid.h" +#include "backend/metadata/location.h" +#include "backend/metadata/message.h" +#include "backend/metadata/message_content.h" +#include "backend/metadata/plugin_dirty_info.h" +#include "backend/metadata/plugin_metadata.h" +#include "backend/metadata/tag.h" +#include "backend/plugin/plugin.h" + +namespace loot { + void PrintTo(const File& value, ::std::ostream* os) { + *os << "loot::File(\"" << value.Name() << "\", " + << "\"" << value.DisplayName() << "\", " + << "\"" << value.Condition() << "\"" + << ")"; + } + + void PrintTo(const FormID& value, ::std::ostream* os) { + *os << "loot::FormID(\"" << value.Plugin() << "\", 0x" + << std::hex << std::uppercase + << value.Id() + << std::nouppercase << std::dec + << ")"; + } + + void PrintTo(const Location& value, ::std::ostream* os) { + *os << "loot::Location(\"" << value.URL() << "\", " + << ::testing::PrintToString(value.Versions()) + << ")"; + } + + void PrintTo(const Message& value, ::std::ostream* os) { + std::string type; + if (value.Type() == Message::warn) + type = "warn"; + else if (value.Type() == Message::error) + type = "error"; + else + type = "say"; + + *os << "loot::Message(\"" << type << "\", " + << ::testing::PrintToString(value.Content()) << ", " + << "\"" << value.Condition() << "\"" + << ")"; + } + + void PrintTo(const MessageContent& value, ::std::ostream* os) { + *os << "loot::MessageContent(\"" << value.Str() << "\", " + << "\"" << Language(value.Language()).Name() << "\"" + << ")"; + } + + void PrintTo(const PluginDirtyInfo& value, ::std::ostream* os) { + *os << "loot::PluginDirtyInfo(0x" + << std::hex << std::uppercase + << value.CRC() + << std::nouppercase << std::dec << ", " + << value.ITMs() << ", " + << value.DeletedRefs() << ", " + << value.DeletedNavmeshes() << ", " + << "\"" << value.CleaningUtility() << "\"" + << ")"; + } + + void PrintTo(const PluginMetadata& value, ::std::ostream* os) { + *os << "loot::PluginMetadata(\"" << value.Name() << "\")"; + } + + void PrintTo(const Tag& value, ::std::ostream* os) { + *os << "loot::Tag(\"" << value.Name() << "\", " + << value.IsAddition() << ", " + << "\"" << value.Condition() << "\"" + << ")"; + } + + void PrintTo(const Plugin& value, ::std::ostream* os) { + *os << "loot::Plugin(\"" << value.Name() << "\")"; + } +} + +#endif From 9b57fef7f15ba9ccf389a0dd32bdaf51fb9017fc Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Wed, 15 Jul 2015 11:22:16 +0100 Subject: [PATCH 5/5] Define a consistent graph vertex storage order. The multithreading patch in 16ac349e8b48c901d97e28d78e34f0f61af0fef3 changed the order in which plugins are inserted into the unordered map that is iterated over to create graph vertices. With MSVC 2013, this insertion order doesn't seem to matter, and the iteration order seems to be consistently the lexicographical order of elements. With GCC 5.0 though, the insertion order does seem to matter, as the multithreading patch's changes produced a different iteration order. The order in which graph vertices are created (and so stored, and so iterated over) matters when tie-break edges are added, as earlier tie-breaks can make potential later tie-breaks instead cause a cycle and so be skipped, leading to a possibly different sorting order. To resolve this issue, vertices are now created in lexicographical order of their lowercased plugin filenames, in order to mimic the previous apparent behaviour with MSVC 2013. --- src/backend/plugin_sorter.cpp | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/src/backend/plugin_sorter.cpp b/src/backend/plugin_sorter.cpp index 02954202..91640802 100644 --- a/src/backend/plugin_sorter.cpp +++ b/src/backend/plugin_sorter.cpp @@ -156,8 +156,35 @@ namespace loot { void PluginSorter::BuildPluginGraph(Game& game, const unsigned int language) { BOOST_LOG_TRIVIAL(info) << "Merging masterlist, userlist into plugin list, evaluating conditions and checking for install validity."; + + // The resolution of tie-breaks in the plugin graph may be dependent + // on the order in which vertices are iterated over, as an earlier tie + // break resolution may cause a potential later tie break to instead + // cause a cycle. Vertices are stored in a std::list and added to the + // list using push_back(). + // Plugins are stored in an unordered map, so simply iterating over + // its elements is not guarunteed to produce a consistent vertex order. + // MSVC 2013 and GCC 5.0 have been shown to produce consistent + // iteration orders that differ, and while MSVC 2013's order seems to + // be independent on the order in which the unordered map was filled + // (being lexicographical), GCC 5.0's unordered map iteration order is + // dependent on its insertion order. + // Given that, the order of vertex creation should be made consistent + // in order to produce consistent sorting results. While MSVC 2013 + // doesn't strictly need this, there is no guaruntee that this + // unspecified behaviour will remain in future compiler updates, so + // implement it generally. + + // Using a set of plugin names followed by finding the matching key + // in the unordered map, as it's probably faster than copying the + // full plugin objects then sorting them. + set pluginNames; for (const auto &plugin : game.plugins) { - vertex_t v = boost::add_vertex(plugin.second, graph); + pluginNames.insert(plugin.first); + } + + for (const auto &plugin : pluginNames) { + vertex_t v = boost::add_vertex(game.plugins.find(plugin)->second, graph); BOOST_LOG_TRIVIAL(trace) << "Merging for plugin \"" << graph[v].Name() << "\""; //Check if there is a plugin entry in the masterlist. This will also find matching regex entries.