From 8580d9e416cb04d0bef330402cab024e709abaf0 Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Thu, 2 Jan 2025 18:07:09 -0800 Subject: [PATCH 1/3] Track other kinds of dependencies in query Signed-off-by: Danila Fedorin --- .../include/chpl/framework/Context-detail.h | 40 ++++++++++++++++--- frontend/lib/framework/Context.cpp | 12 +++++- 2 files changed, 46 insertions(+), 6 deletions(-) diff --git a/frontend/include/chpl/framework/Context-detail.h b/frontend/include/chpl/framework/Context-detail.h index 60de258b78ac..650845d049e6 100644 --- a/frontend/include/chpl/framework/Context-detail.h +++ b/frontend/include/chpl/framework/Context-detail.h @@ -228,17 +228,47 @@ auto queryArgsToStrings(const std::tuple& tuple) { // return ss.str(); } -// Performance: this struct only contains a pointer and an additional bit -// field. We could probably get away with storing `errorCollectionRoot` -// in the last bit of the result pointer, and and thus reduce the overhead +// Performance: this struct only contains a pointer and a few additional small +// fields. We could possibly get away with storing these bit fields +// in the last bits of the result pointer, and and thus reduce the overhead // of this struct. struct QueryDependency { + enum DependencyKind { + /* Dependency introduced via a call to another query. */ + DEPENDENCY_DIRECT, + /* Dependency introduced via 'isQueryRunning', and 'isQueryRunning' returned + true. */ + DEPENDENCY_CHECK_TRUE, + /* Dependency introduced via 'isQueryRunning', and 'isQueryRunning' returned + false. */ + DEPENDENCY_CHECK_FALSE, + }; + const QueryMapResultBase* query; bool errorCollectionRoot; + DependencyKind kind; QueryDependency(const QueryMapResultBase* query, - bool errorCollectionRoot) : - query(query), errorCollectionRoot(errorCollectionRoot) {} + bool errorCollectionRoot, + DependencyKind kind) : + query(query), errorCollectionRoot(errorCollectionRoot), kind(kind) {} + + bool isCheck() const { + return kind == DEPENDENCY_CHECK_TRUE || kind == DEPENDENCY_CHECK_FALSE; + } + + bool isDirect() const { + return kind == DEPENDENCY_DIRECT; + } + + // returns true when all of the following conditions are met: + // + // * this is a check dependency (introduced via isQueryRunning) + // * the result of isQueryRunning has changed since this dependency was added + // + // This means that regardless of the result of the query or its dependencies, + // the dependent query should be re-run. + bool checkHasChanged() const; }; using QueryDependencyVec = std::vector; diff --git a/frontend/lib/framework/Context.cpp b/frontend/lib/framework/Context.cpp index 2b6052d558bf..44082c4db538 100644 --- a/frontend/lib/framework/Context.cpp +++ b/frontend/lib/framework/Context.cpp @@ -1236,7 +1236,9 @@ void Context::saveDependencyInParent(const QueryMapResultBase* resultEntry) { } else { bool errorCollectionRoot = !errorCollectionStack.empty() && errorCollectionStack.back().collectingQuery() == parentQuery; - parentQuery->dependencies.push_back(QueryDependency(resultEntry, errorCollectionRoot)); + auto newDependency = QueryDependency(resultEntry, errorCollectionRoot, + QueryDependency::DEPENDENCY_DIRECT); + parentQuery->dependencies.push_back(std::move(newDependency)); parentQuery->errorsPresentInSelfOrDependencies |= resultEntry->errorsPresentInSelfOrDependencies; } @@ -1342,6 +1344,14 @@ void queryArgsPrintSep(std::ostream& s) { s << ", "; } +bool QueryDependency::checkHasChanged() const { + CHPL_ASSERT(isCheck()); + // note: the condition on 'query' should match isQueryRunning + bool isRunning = query->lastChecked == -1 || query->beingTestedForReuse; + return ((isRunning && kind == DEPENDENCY_CHECK_FALSE) || + (!isRunning && kind == DEPENDENCY_CHECK_TRUE)); +} + QueryMapResultBase::QueryMapResultBase(RevisionNumber lastChecked, RevisionNumber lastChanged, bool beingTestedForReuse, From ec32589030b53701e53ae67bd4d3e1c841c24c2d Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Thu, 2 Jan 2025 18:09:13 -0800 Subject: [PATCH 2/3] Update existing code that only handled direct dependencies Signed-off-by: Danila Fedorin --- frontend/lib/framework/Context.cpp | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/frontend/lib/framework/Context.cpp b/frontend/lib/framework/Context.cpp index 44082c4db538..e7644427bb1c 100644 --- a/frontend/lib/framework/Context.cpp +++ b/frontend/lib/framework/Context.cpp @@ -567,6 +567,8 @@ void Context::gatherRecursionTrace(const querydetail::QueryMapResultBase* root, CHPL_ASSERT(result->recursionErrors.count(root) > 0); for (auto& dep : result->dependencies) { + if (!dep.isDirect()) continue; + if (dep.query->recursionErrors.count(root) > 0) { if (auto te = dep.query->tryTrace()) { trace.emplace_back(*te); @@ -1020,6 +1022,14 @@ void Context::recomputeIfNeeded(const QueryMapResultBase* resultEntry) { bool useSaved = true; resultEntry->beingTestedForReuse = true; for (auto& dependency : resultEntry->dependencies) { + if (dependency.isCheck()) { + if (dependency.checkHasChanged()) { + useSaved = false; + break; + } + continue; + } + const QueryMapResultBase* dependencyQuery = dependency.query; if (dependencyQuery->lastChanged > resultEntry->lastChanged) { useSaved = false; @@ -1128,6 +1138,14 @@ bool Context::queryCanUseSavedResult( useSaved = true; resultEntry->beingTestedForReuse = true; for (auto& dependency: resultEntry->dependencies) { + if (dependency.isCheck()) { + if (dependency.checkHasChanged()) { + useSaved = false; + break; + } + continue; + } + const QueryMapResultBase* dependencyQuery = dependency.query; if (dependency.errorCollectionRoot) { @@ -1187,6 +1205,8 @@ void Context::emitHiddenErrorsFor(const querydetail::QueryMapResultBase* result) } result->emittedErrors = true; for (auto& dependency : result->dependencies) { + if (!dependency.isDirect()) continue; + if (!dependency.query->emittedErrors && !dependency.errorCollectionRoot) { emitHiddenErrorsFor(dependency.query); } @@ -1202,6 +1222,8 @@ static void storeErrorsForHelp(const querydetail::QueryMapResultBase* result, into.storeError(error.first->clone()); } for (auto& dependency : result->dependencies) { + if (!dependency.isDirect()) continue; + if (!dependency.errorCollectionRoot && dependency.query->errorsPresentInSelfOrDependencies) { storeErrorsForHelp(dependency.query, visited, into); From b82467f123f6e30ef4099e9d458bc194c7e1c77c Mon Sep 17 00:00:00 2001 From: Danila Fedorin Date: Thu, 2 Jan 2025 18:34:13 -0800 Subject: [PATCH 3/3] Track non-direct dependencies Signed-off-by: Danila Fedorin --- frontend/include/chpl/framework/query-impl.h | 38 ++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/frontend/include/chpl/framework/query-impl.h b/frontend/include/chpl/framework/query-impl.h index b9f33f84e7f3..5db8aee7af02 100644 --- a/frontend/include/chpl/framework/query-impl.h +++ b/frontend/include/chpl/framework/query-impl.h @@ -555,24 +555,58 @@ bool Context::isQueryRunning( const ResultType& (*queryFunction)(Context* context, ArgTs...), const std::tuple& tupleOfArgs) { + // The parent query has effectively created a dependecy on the query being + // checked (since its behavior will be different depending on if it's running), + // so we need to note it as a check dependency. However, the query map, + // or query result entry, may not exist yet, so create empty ones if they + // don't. If the query being checked has never been run, we'll insert + // a never-executed result, which will get executed when needed. + // Look up the map entry for this query name const void* queryFuncV = (const void*) queryFunction; // Look up the map entry for this query auto search = this->queryDB.find(queryFuncV); if (search == this->queryDB.end()) { + // need to create a new entry for the query but that's hard because we need + // to know its string name and whether it's an input query. Can't assert + // here since the first call to (isQueryRunning() else query()) will + // hit the assertion, always. So, just return false. + // + // TODO: can we do something clever here? return false; } // found an entry for this query QueryMapBase* base = search->second.get(); auto queryMap = (QueryMap*)base; + + // find the query, or create a new uninitialized result QueryMapResult key(queryMap, tupleOfArgs); + const QueryMapResult* dependencyQuery = nullptr; auto search2 = queryMap->map.find(key); if (search2 == queryMap->map.end()) { - return false; + // no query like this has ever been executed before. Insert a blank result, + // without running the query, so that we have something to list as a dependency. + dependencyQuery = getResult(queryMap, tupleOfArgs); + dependencyQuery->lastChecked = -2; // special value: never checked, but also not running. + } else { + dependencyQuery = &(*search2); + } + + bool isRunning = dependencyQuery->lastChecked == -1 || dependencyQuery->beingTestedForReuse; + + if (!queryStack.empty()) { + // The parent query has effectively created a dependecy on search2 + // by checking if it's running, so note that. Note that it's safe to + // always add an edge here instead of checking for recursion errors + // since these dependency edges are not traversed. + auto kind = isRunning ? QueryDependency::DEPENDENCY_CHECK_TRUE + : QueryDependency::DEPENDENCY_CHECK_FALSE; + auto newDependency = QueryDependency(dependencyQuery, false, kind); + queryStack.back()->dependencies.push_back(newDependency); } - return search2->lastChecked == -1 || search2->beingTestedForReuse; + return isRunning; } template