From 349baf79883a96fdd85325a2900997fbf574f9a8 Mon Sep 17 00:00:00 2001 From: Christian Kandeler Date: Thu, 9 Aug 2018 13:53:53 +0200 Subject: Fix potential false caching of module prototypes Our code assumed that products with the same profile and the same multiplex configuration would also evaluate a module's condition to the same value, which is not true: Properties such as qbs.architecture, which are commonly checked in module conditions, can be set in a product item independently of multiplexing. We now evaluate the module condition for every product. According to our benchmarker, the slowdown is a very modest 2% and does not appear to increase for larger projects. This patch amends 08ce978733, which tried to address the same problem, but succeeded only for a subset of the possible cases. Change-Id: I992e0f0d5cf207949cf5d863f242b9476ecdfc05 Reviewed-by: Joerg Bornemann --- src/lib/corelib/language/moduleloader.cpp | 28 ++++++---------------- src/lib/corelib/language/moduleloader.h | 7 +++--- .../module-conditions/module-conditions.qbs | 21 ++++++++++++++++ .../testdata/module-conditions/modules/m/m1.qbs | 6 +++++ .../testdata/module-conditions/modules/m/m2.qbs | 6 +++++ .../testdata/module-conditions/modules/m/m3.qbs | 6 +++++ .../testdata/module-conditions/modules/m/m4.qbs | 6 +++++ tests/auto/blackbox/tst_blackbox.cpp | 10 ++++++++ tests/auto/blackbox/tst_blackbox.h | 1 + 9 files changed, 66 insertions(+), 25 deletions(-) create mode 100644 tests/auto/blackbox/testdata/module-conditions/module-conditions.qbs create mode 100644 tests/auto/blackbox/testdata/module-conditions/modules/m/m1.qbs create mode 100644 tests/auto/blackbox/testdata/module-conditions/modules/m/m2.qbs create mode 100644 tests/auto/blackbox/testdata/module-conditions/modules/m/m3.qbs create mode 100644 tests/auto/blackbox/testdata/module-conditions/modules/m/m4.qbs diff --git a/src/lib/corelib/language/moduleloader.cpp b/src/lib/corelib/language/moduleloader.cpp index 4b87809c4..79748d451 100644 --- a/src/lib/corelib/language/moduleloader.cpp +++ b/src/lib/corelib/language/moduleloader.cpp @@ -83,11 +83,6 @@ namespace Internal { static QString shadowProductPrefix() { return QStringLiteral("__shadow__"); } -static QString multiplexConfigurationIdPropertyInternal() -{ - return QStringLiteral("__multiplexConfigIdForModulePrototypes"); -} - static void handlePropertyError(const ErrorInfo &error, const SetupProjectParameters ¶ms, Logger &logger) { @@ -896,8 +891,6 @@ QList ModuleLoader::multiplexProductItem(ProductContext *dummyContext, I const QString multiplexConfigurationId = multiplexInfo.toIdString(row); const VariantValuePtr multiplexConfigurationIdValue = VariantValue::create(multiplexConfigurationId); - item->setProperty(multiplexConfigurationIdPropertyInternal(), - multiplexConfigurationIdValue); if (multiplexInfo.table.size() > 1 || aggregator) { multiplexConfigurationIdValues.push_back(multiplexConfigurationIdValue); item->setProperty(StringConstants::multiplexConfigurationIdProperty(), @@ -1142,8 +1135,6 @@ void ModuleLoader::prepareProduct(ProjectContext *projectContext, Item *productI } productContext.multiplexConfigurationId = m_evaluator->stringValue( productItem, StringConstants::multiplexConfigurationIdProperty()); - productContext.multiplexConfigIdForModulePrototypes = m_evaluator->stringValue( - productItem, multiplexConfigurationIdPropertyInternal()); QBS_CHECK(!productContext.profileName.isEmpty()); const auto it = projectContext->result->profileConfigs.constFind(productContext.profileName); if (it == projectContext->result->profileConfigs.constEnd()) { @@ -3107,16 +3098,11 @@ Item *ModuleLoader::loadModuleFile(ProductContext *productContext, const QString if (!module) return nullptr; - auto &conditionInfoList = m_modulePrototypeEnabledInfo[module]; - - // TODO: This is not good enough. qbs properties can differ independent of multiplexing. - const QString uniqueConfigKey = productContext->multiplexConfigIdForModulePrototypes; - - for (const auto &conditionInfo : conditionInfoList) { - if (conditionInfo.first == uniqueConfigKey) { - qCDebug(lcModuleLoader) << "prototype cache hit (level 2)"; - return conditionInfo.second ? module : nullptr; - } + const auto key = std::make_pair(module, productContext); + const auto it = m_modulePrototypeEnabledInfo.find(key); + if (it != m_modulePrototypeEnabledInfo.end()) { + qCDebug(lcModuleLoader) << "prototype cache hit (level 2)"; + return it.value() ? module : nullptr; } // Set the name before evaluating any properties. EvaluatorScriptClass reads the module name. @@ -3129,7 +3115,7 @@ Item *ModuleLoader::loadModuleFile(ProductContext *productContext, const QString deepestModuleInstance->setPrototype(origDeepestModuleInstancePrototype); if (!enabled) { qCDebug(lcModuleLoader) << "condition of module" << fullModuleName << "is false"; - conditionInfoList.push_back(std::make_pair(uniqueConfigKey, false)); + m_modulePrototypeEnabledInfo.insert(key, false); return nullptr; } @@ -3138,7 +3124,7 @@ Item *ModuleLoader::loadModuleFile(ProductContext *productContext, const QString else resolveParameterDeclarations(module); - conditionInfoList.push_back(std::make_pair(uniqueConfigKey, true)); + m_modulePrototypeEnabledInfo.insert(key, true); return module; } diff --git a/src/lib/corelib/language/moduleloader.h b/src/lib/corelib/language/moduleloader.h index ce13364d7..7be9a0fcc 100644 --- a/src/lib/corelib/language/moduleloader.h +++ b/src/lib/corelib/language/moduleloader.h @@ -176,7 +176,6 @@ private: ModuleLoaderResult::ProductInfo info; QString profileName; QString multiplexConfigurationId; - QString multiplexConfigIdForModulePrototypes; QVariantMap moduleProperties; std::map productModuleDependencies; std::unordered_map> unknownProfilePropertyErrors; @@ -392,9 +391,9 @@ private: // The keys are file paths, the values are module prototype items accompanied by a profile. std::unordered_map>> m_modulePrototypes; - // The keys are module prototypes, the values specify whether the module's condition - // is true for the respective configuration. - std::unordered_map>> m_modulePrototypeEnabledInfo; + // The keys are module prototypes and products, the values specify whether the module's + // condition is true for that product. + QHash, bool> m_modulePrototypeEnabledInfo; QHash m_parameterDeclarations; Set m_disabledItems; diff --git a/tests/auto/blackbox/testdata/module-conditions/module-conditions.qbs b/tests/auto/blackbox/testdata/module-conditions/module-conditions.qbs new file mode 100644 index 000000000..dc3768203 --- /dev/null +++ b/tests/auto/blackbox/testdata/module-conditions/module-conditions.qbs @@ -0,0 +1,21 @@ +import qbs + +Project { + Product { + name: "p1" + qbs.architecture: "a" + Depends { name: "m" } + } + Product { + name: "p2" + qbs.architecture: "b" + Depends { name: "m" } + } + Product { + name: "p3" + multiplexByQbsProperties: "architectures" + aggregate: false + qbs.architectures: ["b", "c", "d"] + Depends { name: "m" } + } +} diff --git a/tests/auto/blackbox/testdata/module-conditions/modules/m/m1.qbs b/tests/auto/blackbox/testdata/module-conditions/modules/m/m1.qbs new file mode 100644 index 000000000..884350c3f --- /dev/null +++ b/tests/auto/blackbox/testdata/module-conditions/modules/m/m1.qbs @@ -0,0 +1,6 @@ +Module { + condition: qbs.architecture === "a" + validate: { + console.info("loaded m1"); + } +} diff --git a/tests/auto/blackbox/testdata/module-conditions/modules/m/m2.qbs b/tests/auto/blackbox/testdata/module-conditions/modules/m/m2.qbs new file mode 100644 index 000000000..bcec6f424 --- /dev/null +++ b/tests/auto/blackbox/testdata/module-conditions/modules/m/m2.qbs @@ -0,0 +1,6 @@ +Module { + condition: qbs.architecture === "b" + validate: { + console.info("loaded m2"); + } +} diff --git a/tests/auto/blackbox/testdata/module-conditions/modules/m/m3.qbs b/tests/auto/blackbox/testdata/module-conditions/modules/m/m3.qbs new file mode 100644 index 000000000..5453c617f --- /dev/null +++ b/tests/auto/blackbox/testdata/module-conditions/modules/m/m3.qbs @@ -0,0 +1,6 @@ +Module { + condition: qbs.architecture === "c" + validate: { + console.info("loaded m3"); + } +} diff --git a/tests/auto/blackbox/testdata/module-conditions/modules/m/m4.qbs b/tests/auto/blackbox/testdata/module-conditions/modules/m/m4.qbs new file mode 100644 index 000000000..a4cb0350d --- /dev/null +++ b/tests/auto/blackbox/testdata/module-conditions/modules/m/m4.qbs @@ -0,0 +1,6 @@ +Module { + condition: qbs.architecture === "d" + validate: { + console.info("loaded m4"); + } +} diff --git a/tests/auto/blackbox/tst_blackbox.cpp b/tests/auto/blackbox/tst_blackbox.cpp index 06e135549..8068aee48 100644 --- a/tests/auto/blackbox/tst_blackbox.cpp +++ b/tests/auto/blackbox/tst_blackbox.cpp @@ -6004,6 +6004,16 @@ void TestBlackbox::missingOverridePrefix() m_qbsStderr.constData()); } +void TestBlackbox::moduleConditions() +{ + QDir::setCurrent(testDataDir + "/module-conditions"); + QCOMPARE(runQbs(), 0); + QCOMPARE(m_qbsStdout.count("loaded m1"), 1); + QCOMPARE(m_qbsStdout.count("loaded m2"), 2); + QCOMPARE(m_qbsStdout.count("loaded m3"), 1); + QCOMPARE(m_qbsStdout.count("loaded m4"), 1); +} + void TestBlackbox::movedFileDependency() { QDir::setCurrent(testDataDir + "/moved-file-dependency"); diff --git a/tests/auto/blackbox/tst_blackbox.h b/tests/auto/blackbox/tst_blackbox.h index 87a6b1006..0dcfc5b31 100644 --- a/tests/auto/blackbox/tst_blackbox.h +++ b/tests/auto/blackbox/tst_blackbox.h @@ -170,6 +170,7 @@ private slots: void missingDependency(); void missingProjectFile(); void missingOverridePrefix(); + void moduleConditions(); void movedFileDependency(); void multipleChanges(); void multipleConfigurations(); -- cgit v1.2.3