Do not needlessly re-evaluate module properties in Groups

For a Group that contains at least one module property assignment, we
used to re-evaluate all module properties. This was of course immensely
wasteful, as there are normally only a handful of properties in need of
re-evaluation.
We fix the problem in the following way:
    - When evaluating a product's module properties, keep track of
      the dependencies between them.
    - For all Groups of that product, re-evaluate only the properties
      which are either explicitly set in that group or have a dependency
      on such a property.

Here's the output of qbs_benchmarker, using qbs itself as the test
project:
========== Performance data for Resolving ==========
    Old instruction count: 2083482809
    New instruction count: 1696643887
    Relative change: -19 %
    Old peak memory usage: 2997349 Bytes
    New peak memory usage: 2704033 Bytes
    Relative change: -10 %

I've also profiled with the Qt Creator super project (not using
valgrind, as that would take ages). For that project, this change halves
the time spent in the ProjectResolver, while the overall resolving time
goes down by about a third.

Note that we could potentially use the gathered dependency data in
change tracking as well, provided we manage to find out what exactly
changed in the project files. That should result in an improved
experience for IDE users.

Change-Id: I6ceffe7a6f0421617f53a4dd2eade51993c2785f
Reviewed-by: Joerg Bornemann <joerg.bornemann@qt.io>

7 files changed, 170 insertions, 26 deletions

diff --git a/src/lib/corelib/language/evaluator.cpp b/src/lib/corelib/language/evaluator.cpp
index a801311bb..329ea4716 100644
--- a/src/lib/corelib/language/evaluator.cpp
+++ b/src/lib/corelib/language/evaluator.cpp
@@ -242,5 +242,15 @@ void Evaluator::setCachingEnabled(bool enabled)
     m_scriptClass->setValueCacheEnabled(enabled);
 }
 
+PropertyDependencies Evaluator::propertyDependencies() const
+{
+    return m_scriptClass->propertyDependencies();
+}
+
+void Evaluator::clearPropertyDependencies()
+{
+    m_scriptClass->clearPropertyDependencies();
+}
+
 } // namespace Internal
 } // namespace qbs
diff --git a/src/lib/corelib/language/evaluator.h b/src/lib/corelib/language/evaluator.h
index 49262d8a5..3701935d3 100644
--- a/src/lib/corelib/language/evaluator.h
+++ b/src/lib/corelib/language/evaluator.h
@@ -42,6 +42,7 @@
 
 #include "forward_decls.h"
 #include "itemobserver.h"
+#include "qualifiedid.h"
 
 #include <QHash>
 #include <QScriptValue>
@@ -77,6 +78,9 @@ public:
 
     void setCachingEnabled(bool enabled);
 
+    PropertyDependencies propertyDependencies() const;
+    void clearPropertyDependencies();
+
     void handleEvaluationError(const Item *item, const QString &name,
             const QScriptValue &scriptValue);
 private:
diff --git a/src/lib/corelib/language/evaluatorscriptclass.cpp b/src/lib/corelib/language/evaluatorscriptclass.cpp
index e7d5a3647..b14cbc0db 100644
--- a/src/lib/corelib/language/evaluatorscriptclass.cpp
+++ b/src/lib/corelib/language/evaluatorscriptclass.cpp
@@ -501,6 +501,43 @@ static void convertToPropertyType(const Item *item, const PropertyDeclaration& d
     }
 }
 
+class PropertyStackManager
+{
+public:
+    PropertyStackManager(const Item *itemOfProperty, const QScriptString &name, const Value *value,
+                         QStack<QualifiedId> &requestedProperties,
+                         PropertyDependencies &propertyDependencies)
+        : m_requestedProperties(requestedProperties)
+    {
+        if (value->type() == Value::JSSourceValueType
+                && (itemOfProperty->type() == ItemType::ModuleInstance
+                    || itemOfProperty->type() == ItemType::Module)) {
+            const VariantValueConstPtr varValue
+                    = itemOfProperty->variantProperty(QLatin1String("name"));
+            // QBS_CHECK(varValue);
+            // TODO: Check why the base module sometimes has no name. Code suggests it has to have one.
+            if (varValue) {
+                m_stackUpdate = true;
+                const QualifiedId fullPropName
+                        = QualifiedId::fromString(varValue->value().toString()) << name.toString();
+                if (!requestedProperties.isEmpty())
+                    propertyDependencies[fullPropName].insert(requestedProperties.top());
+                m_requestedProperties.push(fullPropName);
+            }
+        }
+    }
+
+    ~PropertyStackManager()
+    {
+        if (m_stackUpdate)
+            m_requestedProperties.pop();
+    }
+
+private:
+    QStack<QualifiedId> &m_requestedProperties;
+    bool m_stackUpdate = false;
+};
+
 QScriptValue EvaluatorScriptClass::property(const QScriptValue &object, const QScriptString &name,
                                             uint id)
 {
@@ -525,6 +562,9 @@ QScriptValue EvaluatorScriptClass::property(const QScriptValue &object, const QS
     if (debugProperties)
         qDebug() << "[SC] property " << name;
 
+    PropertyStackManager propStackmanager(itemOfProperty, name, value.data(),
+                                          m_requestedProperties, m_propertyDependencies);
+
     QScriptValue result;
     if (m_valueCacheEnabled) {
         result = data->valueCache.value(name);
diff --git a/src/lib/corelib/language/evaluatorscriptclass.h b/src/lib/corelib/language/evaluatorscriptclass.h
index d17428c3f..5739f7d8b 100644
--- a/src/lib/corelib/language/evaluatorscriptclass.h
+++ b/src/lib/corelib/language/evaluatorscriptclass.h
@@ -41,6 +41,7 @@
 #define QBS_EVALUATORSCRIPTCLASS_H
 
 #include "forward_decls.h"
+#include "qualifiedid.h"
 
 #include <logging/logger.h>
 
@@ -71,6 +72,9 @@ public:
 
     void setValueCacheEnabled(bool enabled);
 
+    PropertyDependencies propertyDependencies() const { return m_propertyDependencies; }
+    void clearPropertyDependencies() { m_propertyDependencies.clear(); }
+
 private:
     QueryFlags queryItemProperty(const EvaluationData *data,
                                  const QString &name,
@@ -98,6 +102,8 @@ private:
     bool m_valueCacheEnabled;
     QStack<JSSourceValue *> m_sourceValueStack;
     QSet<Value *> m_currentNextChain;
+    PropertyDependencies m_propertyDependencies;
+    QStack<QualifiedId> m_requestedProperties;
 };
 
 } // namespace Internal
diff --git a/src/lib/corelib/language/projectresolver.cpp b/src/lib/corelib/language/projectresolver.cpp
index 2b180e920..b8dd5c649 100644
--- a/src/lib/corelib/language/projectresolver.cpp
+++ b/src/lib/corelib/language/projectresolver.cpp
@@ -64,7 +64,6 @@
 #include <QQueue>
 
 #include <algorithm>
-#include <set>
 
 namespace qbs {
 namespace Internal {
@@ -328,6 +327,7 @@ void ProjectResolver::resolveSubProject(Item *item, ProjectResolver::ProjectCont
 void ProjectResolver::resolveProduct(Item *item, ProjectContext *projectContext)
 {
     checkCancelation();
+    m_evaluator->clearPropertyDependencies();
     ProductContext productContext;
     m_productContext = &productContext;
     productContext.item = item;
@@ -522,21 +522,98 @@ SourceArtifactPtr ProjectResolver::createSourceArtifact(const ResolvedProductCon
     return artifact;
 }
 
-static bool isSomeModulePropertySet(Item *group)
+static QualifiedIdSet propertiesToEvaluate(const QList<QualifiedId> &initialProps,
+                                              const PropertyDependencies &deps)
 {
-    for (QMap<QString, ValuePtr>::const_iterator it = group->properties().constBegin();
-         it != group->properties().constEnd(); ++it)
+    QList<QualifiedId> remainingProps = initialProps;
+    QualifiedIdSet allProperties;
+    while (!remainingProps.isEmpty()) {
+        const QualifiedId prop = remainingProps.takeFirst();
+        const QualifiedIdSet::InsertResult insertResult = allProperties.insert(prop);
+        if (!insertResult.second)
+            continue;
+        const QualifiedIdSet &directDeps = deps.value(prop);
+        for (auto depIt = directDeps.cbegin(); depIt != directDeps.cend(); ++depIt)
+            remainingProps << *depIt;
+    }
+    return allProperties;
+}
+
+static void gatherAssignedProperties(ItemValue *iv, const QualifiedId &prefix,
+                                     QList<QualifiedId> &properties)
+{
+    const Item::PropertyMap &props = iv->item()->properties();
+    for (auto it = props.cbegin(); it != props.cend(); ++it) {
+        switch (it.value()->type()) {
+        case Value::JSSourceValueType:
+            properties << (QualifiedId(prefix) << it.key());
+            break;
+        case Value::ItemValueType:
+            gatherAssignedProperties(it.value().staticCast<ItemValue>().data(),
+                           QualifiedId(prefix) << it.key(), properties);
+            break;
+        default:
+            break;
+        }
+    }
+}
+
+class CachingEnabler
+{
+public:
+    CachingEnabler(Evaluator *evaluator) : m_evaluator(evaluator)
     {
+        m_evaluator->setCachingEnabled(true);
+    }
+
+    ~CachingEnabler() { m_evaluator->setCachingEnabled(false); }
+
+private:
+    Evaluator * const m_evaluator;
+};
+
+QVariantMap ProjectResolver::resolveAdditionalModuleProperties(const Item *group,
+                                                               const QVariantMap &currentValues)
+{
+    // Step 1: Gather the properties directly set in the group
+    QList<QualifiedId> propsSetInGroup;
+    for (auto it = group->properties().cbegin(); it != group->properties().cend(); ++it) {
         if (it.value()->type() == Value::ItemValueType) {
-            // An item value is a module value in this case.
-            ItemValuePtr iv = it.value().staticCast<ItemValue>();
-            foreach (ValuePtr ivv, iv->item()->properties()) {
-                if (ivv->type() == Value::JSSourceValueType)
-                    return true;
-            }
+            gatherAssignedProperties(it.value().staticCast<ItemValue>().data(),
+                                     QualifiedId(it.key()), propsSetInGroup);
         }
     }
-    return false;
+    if (propsSetInGroup.isEmpty())
+        return QVariantMap();
+
+    // Step 2: Gather all properties that depend on these properties.
+    const QualifiedIdSet &propsToEval
+            = propertiesToEvaluate(propsSetInGroup, m_evaluator->propertyDependencies());
+
+    // Step 3: Evaluate all these properties and replace their values in the map
+    QVariantMap modulesMap = currentValues.value(QLatin1String("modules")).toMap();
+    QHash<QString, QStringList> propsPerModule;
+    for (auto it = propsToEval.cbegin(); it != propsToEval.cend(); ++it) {
+        const QualifiedId fullPropName = *it;
+        const QString moduleName
+                = QualifiedId(fullPropName.mid(0, fullPropName.count() - 1)).toString();
+        propsPerModule[moduleName] << fullPropName.last();
+    }
+    CachingEnabler cachingEnabler(m_evaluator);
+    for (auto it = group->modules().cbegin(); it != group->modules().cend(); ++it) {
+        const QString &fullModName = it->name.toString();
+        const QStringList propsForModule = propsPerModule.take(fullModName);
+        if (propsForModule.isEmpty())
+            continue;
+        QVariantMap reusableValues = modulesMap.value(fullModName).toMap();
+        for (auto propIt = propsForModule.cbegin(); propIt != propsForModule.cend(); ++propIt)
+            reusableValues.remove(*propIt);
+        modulesMap.insert(fullModName,
+                          evaluateProperties(it->item, it->item, reusableValues, true));
+    }
+    QVariantMap newValues = currentValues;
+    newValues.insert(QLatin1String("modules"), modulesMap);
+    return newValues;
 }
 
 void ProjectResolver::resolveGroup(Item *item, ProjectContext *projectContext)
@@ -546,11 +623,11 @@ void ProjectResolver::resolveGroup(Item *item, ProjectContext *projectContext)
     PropertyMapPtr moduleProperties = m_productContext->currentGroup
             ? m_productContext->currentGroup->properties
             : m_productContext->product->moduleProperties;
-    if (isSomeModulePropertySet(item)) {
+    const QVariantMap newModuleProperties
+            = resolveAdditionalModuleProperties(item, moduleProperties->value());
+    if (!newModuleProperties.isEmpty()) {
         moduleProperties = PropertyMapInternal::create();
-        m_evaluator->setCachingEnabled(true);
-        moduleProperties->setValue(evaluateModuleValues(item));
-        m_evaluator->setCachingEnabled(false);
+        moduleProperties->setValue(newModuleProperties);
     }
 
     AccumulatingTimer groupTimer(m_setupParams.logElapsedTime()
@@ -759,12 +836,6 @@ void ProjectResolver::resolveRule(Item *item, ProjectContext *projectContext)
         projectContext->rules += rule;
 }
 
-class QualifiedIdSet : public std::set<QualifiedId>
-{
-public:
-    typedef std::pair<iterator, bool> InsertResult;
-};
-
 void ProjectResolver::resolveRuleArtifact(const RulePtr &rule, Item *item)
 {
     RuleArtifactPtr artifact = RuleArtifact::create();
@@ -1092,7 +1163,7 @@ QVariantMap ProjectResolver::evaluateProperties(Item *item, bool lookupPrototype
     return evaluateProperties(item, item, tmplt, lookupPrototype);
 }
 
-QVariantMap ProjectResolver::evaluateProperties(Item *item, Item *propertiesContainer,
+QVariantMap ProjectResolver::evaluateProperties(const Item *item, const Item *propertiesContainer,
         const QVariantMap &tmplt, bool lookupPrototype)
 {
     AccumulatingTimer propEvalTimer(m_setupParams.logElapsedTime()
@@ -1161,10 +1232,9 @@ QVariantMap ProjectResolver::evaluateProperties(Item *item, Item *propertiesCont
 
 QVariantMap ProjectResolver::createProductConfig()
 {
-    m_evaluator->setCachingEnabled(true);
+    CachingEnabler cachingEnabler(m_evaluator);
     QVariantMap cfg = evaluateModuleValues(m_productContext->item);
     cfg = evaluateProperties(m_productContext->item, m_productContext->item, cfg);
-    m_evaluator->setCachingEnabled(false);
     return cfg;
 }
 
diff --git a/src/lib/corelib/language/projectresolver.h b/src/lib/corelib/language/projectresolver.h
index 817f54522..9bfdfd298 100644
--- a/src/lib/corelib/language/projectresolver.h
+++ b/src/lib/corelib/language/projectresolver.h
@@ -94,6 +94,8 @@ private:
     void resolveModules(const Item *item, ProjectContext *projectContext);
     void resolveModule(const QualifiedId &moduleName, Item *item, bool isProduct,
                        ProjectContext *projectContext);
+    QVariantMap resolveAdditionalModuleProperties(const Item *group,
+                                                  const QVariantMap &currentValues);
     void resolveGroup(Item *item, ProjectContext *projectContext);
     void resolveRule(Item *item, ProjectContext *projectContext);
     void resolveRuleArtifact(const RulePtr &rule, Item *item);
@@ -107,8 +109,8 @@ private:
     void applyFileTaggers(const ResolvedProductPtr &product) const;
     QVariantMap evaluateModuleValues(Item *item, bool lookupPrototype = true);
     QVariantMap evaluateProperties(Item *item, bool lookupPrototype = true);
-    QVariantMap evaluateProperties(Item *item, Item *propertiesContainer, const QVariantMap &tmplt,
-            bool lookupPrototype = true);
+    QVariantMap evaluateProperties(const Item *item, const Item *propertiesContainer,
+                                   const QVariantMap &tmplt, bool lookupPrototype = true);
     QVariantMap createProductConfig();
     QString convertPathProperty(const QString &path, const QString &dirPath) const;
     QStringList convertPathListProperty(const QStringList &paths, const QString &dirPath) const;
diff --git a/src/lib/corelib/language/qualifiedid.h b/src/lib/corelib/language/qualifiedid.h
index 330e92d38..a0ae1a658 100644
--- a/src/lib/corelib/language/qualifiedid.h
+++ b/src/lib/corelib/language/qualifiedid.h
@@ -40,8 +40,11 @@
 #ifndef QBS_QUALIFIEDID_H
 #define QBS_QUALIFIEDID_H
 
+#include <QHash>
 #include <QStringList>
 
+#include <set>
+
 namespace qbs {
 namespace Internal {
 
@@ -58,6 +61,15 @@ public:
 
 bool operator<(const QualifiedId &a, const QualifiedId &b);
 
+class QualifiedIdSet : public std::set<QualifiedId>
+{
+public:
+    typedef std::pair<iterator, bool> InsertResult;
+};
+
+// Values are the properties with a dependency on the key property
+using PropertyDependencies = QHash<QualifiedId, QualifiedIdSet>;
+
 } // namespace Internal
 } // namespace qbs