From 23368f82bde19e8c857196fa0c61434a3f8db88b Mon Sep 17 00:00:00 2001 From: mpoeter Date: Wed, 9 Jun 2021 14:22:06 +0200 Subject: [PATCH 01/10] Introduce callstack splitting to avoid stackoverflows with large AQL queries. --- CMakeLists.txt | 1 + arangod/Aql/ExecutionBlockImpl.cpp | 79 +++++++++++++++++++- arangod/Aql/ExecutionBlockImpl.h | 37 ++++++++- arangod/Aql/ExecutionNode.cpp | 4 + arangod/Aql/ExecutionNode.h | 8 ++ arangod/Aql/Optimizer.cpp | 1 + arangod/Aql/OptimizerRules.cpp | 45 +++++++++++ arangod/Aql/OptimizerRules.h | 1 + arangod/Aql/QueryOptions.cpp | 8 ++ arangod/Aql/QueryOptions.h | 2 + arangod/RestServer/QueryRegistryFeature.cpp | 8 ++ arangod/RestServer/QueryRegistryFeature.h | 1 + js/common/modules/@arangodb/aql/explainer.js | 11 ++- 13 files changed, 201 insertions(+), 5 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index d270f23ef1a4..67265f864d4d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -978,6 +978,7 @@ if (MSVC) add_definitions(/Z7) endif () + add_definitions(/bigobj) else () # NOT MSVC set(EXTRA_C_FLAGS "") set(EXTRA_CXX_FLAGS "") diff --git a/arangod/Aql/ExecutionBlockImpl.cpp b/arangod/Aql/ExecutionBlockImpl.cpp index 5506479594eb..6939de914f8c 100644 --- a/arangod/Aql/ExecutionBlockImpl.cpp +++ b/arangod/Aql/ExecutionBlockImpl.cpp @@ -188,6 +188,10 @@ ExecutionBlockImpl::ExecutionBlockImpl(ExecutionEngine* engine, // Break the stack before waiting. // We should not use this here. _stackBeforeWaiting.popCall(); + + if (_exeNode->isCallstackSplitEnabled()) { + _callstackSplit = std::make_unique(*this); + } } template @@ -1518,7 +1522,14 @@ ExecutionBlockImpl::executeWithoutTrace(AqlCallStack const& callStack) auto subqueryLevelBefore = ctx.stack.subqueryLevel(); #endif SkipResult skippedLocal; - std::tie(_upstreamState, skippedLocal, _lastRange) = executeFetcher(ctx, _upstreamRequest); + if (_callstackSplit) { + // we need to split the callstack to avoid stack overflows, so we move upstream + // execution into a separate thread + std::tie(_upstreamState, skippedLocal, _lastRange) = _callstackSplit->execute(ctx, _upstreamRequest); + } else { + std::tie(_upstreamState, skippedLocal, _lastRange) = executeFetcher(ctx, _upstreamRequest); + } + #ifdef ARANGODB_ENABLE_MAINTAINER_MODE TRI_ASSERT(subqueryLevelBefore == ctx.stack.subqueryLevel()); #endif @@ -1969,6 +1980,72 @@ void ExecutionBlockImpl::PrefetchTask::execute(ExecutionBlockImpl& blo } } +template +ExecutionBlockImpl::CallstackSplit::CallstackSplit(ExecutionBlockImpl& block) : + _block(block), + _thread(&CallstackSplit::run, this, std::cref(ExecContext::current())) {} + +template +ExecutionBlockImpl::CallstackSplit::~CallstackSplit() { + _lock.lock(); + _state.store(State::Stopped); + _lock.unlock(); + + _bell.notify_one(); + _thread.join(); +} + +template +auto ExecutionBlockImpl::CallstackSplit::execute(ExecutionContext& ctx, AqlCallType const& aqlCall) + -> UpstreamResult { + std::variant result{std::nullopt}; + Params params{result, ctx, aqlCall}; + + _lock.lock(); + _params = ¶ms; + _state.store(State::Executing); + _lock.unlock(); + + _bell.notify_one(); + + std::unique_lock guard(_lock); + _bell.wait(guard, [this]() { + return _state.load(std::memory_order_acquire) != State::Executing; + }); + TRI_ASSERT(_state.load() == State::Waiting); + + if (std::holds_alternative(result)) { + std::rethrow_exception(std::get(result)); + } + + return std::get(std::move(result)); +} + +template +void ExecutionBlockImpl::CallstackSplit::run(ExecContext const& execContext) { + ExecContextScope scope(&execContext); + std::unique_lock guard(_lock); + while (true) { + _bell.wait(guard, [this]() { + return _state.load(std::memory_order_relaxed) != State::Waiting; + }); + if (_state == State::Stopped) { + return; + } + TRI_ASSERT(_params != nullptr); + _state = State::Executing; + + try { + _params->result = _block.executeFetcher(_params->ctx, _params->aqlCall); + } catch(...) { + _params->result = std::current_exception(); + } + + _state.store(State::Waiting, std::memory_order_relaxed); + _bell.notify_one(); + } +} + template class ::arangodb::aql::ExecutionBlockImpl>; template class ::arangodb::aql::ExecutionBlockImpl>; template class ::arangodb::aql::ExecutionBlockImpl>; diff --git a/arangod/Aql/ExecutionBlockImpl.h b/arangod/Aql/ExecutionBlockImpl.h index 2a020e6b62c7..73342d5893ad 100644 --- a/arangod/Aql/ExecutionBlockImpl.h +++ b/arangod/Aql/ExecutionBlockImpl.h @@ -39,6 +39,10 @@ #include #include +namespace arangodb { + class ExecContext; +} + namespace arangodb::aql { template @@ -346,6 +350,33 @@ class ExecutionBlockImpl final : public ExecutionBlock { std::condition_variable _bell; std::optional _result; }; + + struct CallstackSplit { + explicit CallstackSplit(ExecutionBlockImpl& block); + ~CallstackSplit(); + + using UpstreamResult = + std::tuple; + UpstreamResult execute(ExecutionContext& ctx, AqlCallType const& aqlCall); + + private: + enum class State { Waiting, Signalled, Executing, Stopped }; + struct Params { + std::variant& result; + ExecutionContext& ctx; + AqlCallType const& aqlCall; + }; + + void run(ExecContext const& execContext); + std::atomic _state{State::Waiting}; + Params* _params; + ExecutionBlockImpl& _block; + + std::mutex _lock; + std::condition_variable _bell; + + std::thread _thread; + }; RegisterInfos _registerInfos; @@ -393,9 +424,11 @@ class ExecutionBlockImpl final : public ExecutionBlock { AqlCallStack _stackBeforeWaiting; std::shared_ptr _prefetchTask; - + + std::unique_ptr _callstackSplit; + Result _firstFailure; - + bool _hasMemoizedCall{false}; // Only used in passthrough variant. diff --git a/arangod/Aql/ExecutionNode.cpp b/arangod/Aql/ExecutionNode.cpp index 894393e0d1d5..fff6411464e9 100644 --- a/arangod/Aql/ExecutionNode.cpp +++ b/arangod/Aql/ExecutionNode.cpp @@ -505,6 +505,9 @@ ExecutionNode::ExecutionNode(ExecutionPlan* plan, VPackSlice const& slice) _isAsyncPrefetchEnabled = VelocyPackHelper::getBooleanValue(slice, "isAsyncPrefetchEnabled", false); + + _isCallstackSplitEnabled = + VelocyPackHelper::getBooleanValue(slice, "isCallstackSplitEnabled", false); } ExecutionNode::ExecutionNode(ExecutionPlan& plan, ExecutionNode const& other) @@ -946,6 +949,7 @@ void ExecutionNode::toVelocyPackHelperGeneric(VPackBuilder& nodes, unsigned flag nodes.add("isInSplicedSubquery", VPackValue(_isInSplicedSubquery)); nodes.add("isAsyncPrefetchEnabled", VPackValue(_isAsyncPrefetchEnabled)); + nodes.add("isCallstackSplitEnabled", VPackValue(_isCallstackSplitEnabled)); } TRI_ASSERT(nodes.isOpenObject()); } diff --git a/arangod/Aql/ExecutionNode.h b/arangod/Aql/ExecutionNode.h index 8bf45635671a..043cbefdfdd9 100644 --- a/arangod/Aql/ExecutionNode.h +++ b/arangod/Aql/ExecutionNode.h @@ -475,6 +475,10 @@ class ExecutionNode { void setIsAsyncPrefetchEnabled(bool v) noexcept { _isAsyncPrefetchEnabled = v; } + bool isCallstackSplitEnabled() const noexcept { return _isCallstackSplitEnabled; } + + void enableCallstackSplit() noexcept { _isCallstackSplitEnabled = true; } + [[nodiscard]] static bool isIncreaseDepth(NodeType type); [[nodiscard]] bool isIncreaseDepth() const; [[nodiscard]] static bool alwaysCopiesRows(NodeType type); @@ -547,6 +551,10 @@ class ExecutionNode { /// @brief whether or not asynchronous prefetching is enabled for this node bool _isAsyncPrefetchEnabled{false}; + /// @brief whether or not this node should split calls to upstream nodes to a + /// separate thread to avoid the problem of stackoverflows in large queries. + bool _isCallstackSplitEnabled{false}; + /// @brief _plan, the ExecutionPlan object ExecutionPlan* _plan; diff --git a/arangod/Aql/Optimizer.cpp b/arangod/Aql/Optimizer.cpp index b721591e2c23..cb0e3332acfe 100644 --- a/arangod/Aql/Optimizer.cpp +++ b/arangod/Aql/Optimizer.cpp @@ -375,6 +375,7 @@ void Optimizer::createPlans(std::unique_ptr plan, void Optimizer::finalizePlans() { for (auto& plan : _plans.list) { insertDistributeInputCalculation(*plan.first); + activateCallstackSplit(*plan.first); if (plan.first->isAsyncPrefetchEnabled()) { enableAsyncPrefetching(*plan.first); } diff --git a/arangod/Aql/OptimizerRules.cpp b/arangod/Aql/OptimizerRules.cpp index 50c528e9279b..4ff706671cd6 100644 --- a/arangod/Aql/OptimizerRules.cpp +++ b/arangod/Aql/OptimizerRules.cpp @@ -79,6 +79,23 @@ namespace { +bool willUseV8(arangodb::aql::ExecutionPlan const& plan) { + bool result{false}; + struct V8Checker : arangodb::aql::WalkerWorkerBase { + bool before(arangodb::aql::ExecutionNode* n) override { + if (n->getType() == arangodb::aql::ExecutionNode::CALCULATION && + static_cast(n)->expression()->willUseV8()) { + result = true; + return true; + } + return false; + } + bool result{false}; + } walker{}; + plan.root()->walk(walker); + return walker.result; +} + bool accessesCollectionVariable(arangodb::aql::ExecutionPlan const* plan, arangodb::aql::ExecutionNode const* node, arangodb::aql::VarSet& vars) { @@ -7889,6 +7906,34 @@ void arangodb::aql::enableAsyncPrefetching(ExecutionPlan& plan) { plan.root()->walk(walker); } +void arangodb::aql::activateCallstackSplit(ExecutionPlan& plan) { + if (willUseV8(plan)) { + // V8 requires thread local context configuration, so we cannot + // use our thread based split solution... + return; + } + + auto const& options = plan.getAst()->query().queryOptions(); + struct CallstackSplitter : WalkerWorkerBase { + explicit CallstackSplitter(size_t maxNodes) : maxNodesPerCallstack(maxNodes) {} + bool before(ExecutionNode* n) override { + if (n->getType() == EN::REMOTE) { + // RemoteNodes provide a natural split in the callstack, so we can reset the counter here! + count = 0; + } else if (++count >= maxNodesPerCallstack) { + count = 0; + n->enableCallstackSplit(); + } + return false; + } + size_t maxNodesPerCallstack; + size_t count = 0; + }; + + CallstackSplitter walker(options.maxNodesPerCallstack); + plan.root()->walk(walker); +} + namespace { void findSubqueriesSuitableForSplicing(ExecutionPlan const& plan, diff --git a/arangod/Aql/OptimizerRules.h b/arangod/Aql/OptimizerRules.h index c9d1b0a180b0..50d7b0557650 100644 --- a/arangod/Aql/OptimizerRules.h +++ b/arangod/Aql/OptimizerRules.h @@ -46,6 +46,7 @@ Collection* addCollectionToQuery(QueryContext& query, std::string const& cname, void insertDistributeInputCalculation(ExecutionPlan& plan); void enableAsyncPrefetching(ExecutionPlan& plan); +void activateCallstackSplit(ExecutionPlan& plan); /// @brief adds a SORT operation for IN right-hand side operands void sortInValuesRule(Optimizer*, std::unique_ptr, OptimizerRule const&); diff --git a/arangod/Aql/QueryOptions.cpp b/arangod/Aql/QueryOptions.cpp index 4e424813083d..d1738c16ddb3 100644 --- a/arangod/Aql/QueryOptions.cpp +++ b/arangod/Aql/QueryOptions.cpp @@ -38,6 +38,7 @@ using namespace arangodb::aql; size_t QueryOptions::defaultMemoryLimit = 0; size_t QueryOptions::defaultMaxNumberOfPlans = 128; +size_t QueryOptions::defaultMaxNodesPerCallstack = 250; double QueryOptions::defaultMaxRuntime = 0.0; double QueryOptions::defaultTtl; bool QueryOptions::defaultFailOnWarning = false; @@ -47,6 +48,7 @@ QueryOptions::QueryOptions() : memoryLimit(0), maxNumberOfPlans(QueryOptions::defaultMaxNumberOfPlans), maxWarningCount(10), + maxNodesPerCallstack(QueryOptions::defaultMaxNodesPerCallstack), maxRuntime(0.0), satelliteSyncWait(60.0), ttl(QueryOptions::defaultTtl), // get global default ttl @@ -128,6 +130,11 @@ void QueryOptions::fromVelocyPack(VPackSlice slice) { maxWarningCount = value.getNumber(); } + value = slice.get("maxNodesPerCallstack"); + if (value.isNumber()) { + maxNodesPerCallstack = value.getNumber(); + } + value = slice.get("maxRuntime"); if (value.isNumber()) { maxRuntime = value.getNumber(); @@ -256,6 +263,7 @@ void QueryOptions::toVelocyPack(VPackBuilder& builder, bool disableOptimizerRule builder.add("memoryLimit", VPackValue(memoryLimit)); builder.add("maxNumberOfPlans", VPackValue(maxNumberOfPlans)); builder.add("maxWarningCount", VPackValue(maxWarningCount)); + builder.add("maxNodesPerCallstack", VPackValue(maxNodesPerCallstack)); builder.add("maxRuntime", VPackValue(maxRuntime)); builder.add("satelliteSyncWait", VPackValue(satelliteSyncWait)); builder.add("ttl", VPackValue(ttl)); diff --git a/arangod/Aql/QueryOptions.h b/arangod/Aql/QueryOptions.h index 219967de2278..ce6791a9e6cf 100644 --- a/arangod/Aql/QueryOptions.h +++ b/arangod/Aql/QueryOptions.h @@ -72,6 +72,7 @@ struct QueryOptions { size_t memoryLimit; size_t maxNumberOfPlans; size_t maxWarningCount; + size_t maxNodesPerCallstack; double maxRuntime; // query has to execute within the given time or will be killed double satelliteSyncWait; double ttl; // time until query cursor expires - avoids coursors to @@ -106,6 +107,7 @@ struct QueryOptions { static size_t defaultMemoryLimit; static size_t defaultMaxNumberOfPlans; + static size_t defaultMaxNodesPerCallstack; static double defaultMaxRuntime; static double defaultTtl; static bool defaultFailOnWarning; diff --git a/arangod/RestServer/QueryRegistryFeature.cpp b/arangod/RestServer/QueryRegistryFeature.cpp index 22c487fbb950..97d46ae20df0 100644 --- a/arangod/RestServer/QueryRegistryFeature.cpp +++ b/arangod/RestServer/QueryRegistryFeature.cpp @@ -169,6 +169,7 @@ QueryRegistryFeature::QueryRegistryFeature(application_features::ApplicationServ _queryMemoryLimit(defaultMemoryLimit(PhysicalMemory::getValue(), 0.2, 0.75)), _queryMaxRuntime(aql::QueryOptions::defaultMaxRuntime), _maxQueryPlans(aql::QueryOptions::defaultMaxNumberOfPlans), + _maxNodesPerCallstack(aql::QueryOptions::defaultMaxNodesPerCallstack), _queryCacheMaxResultsCount(0), _queryCacheMaxResultsSize(0), _queryCacheMaxEntrySize(0), @@ -301,6 +302,12 @@ void QueryRegistryFeature::collectOptions(std::shared_ptr option options->addOption("--query.optimizer-max-plans", "maximum number of query plans to create for a query", new UInt64Parameter(&_maxQueryPlans)); + +options->addOption("--query.max-nodes-per-callstack", + "maximum number execution nodes on the callstack before " + "splitting the remaining nodes into a separate thread", + new UInt64Parameter(&_maxNodesPerCallstack), + arangodb::options::makeDefaultFlags(arangodb::options::Flags::Hidden)); options->addOption("--query.registry-ttl", "default time-to-live of cursors and query snippets (in " @@ -380,6 +387,7 @@ void QueryRegistryFeature::validateOptions(std::shared_ptr optio aql::QueryOptions::defaultMemoryLimit = _queryMemoryLimit; aql::QueryOptions::defaultMaxNumberOfPlans = _maxQueryPlans; + aql::QueryOptions::defaultMaxNodesPerCallstack = _maxNodesPerCallstack; aql::QueryOptions::defaultMaxRuntime = _queryMaxRuntime; aql::QueryOptions::defaultTtl = _queryRegistryTTL; aql::QueryOptions::defaultFailOnWarning = _failOnWarning; diff --git a/arangod/RestServer/QueryRegistryFeature.h b/arangod/RestServer/QueryRegistryFeature.h index ebfc97096da8..6f8de23c04f4 100644 --- a/arangod/RestServer/QueryRegistryFeature.h +++ b/arangod/RestServer/QueryRegistryFeature.h @@ -96,6 +96,7 @@ class QueryRegistryFeature final : public application_features::ApplicationFeatu uint64_t _queryMemoryLimit; double _queryMaxRuntime; uint64_t _maxQueryPlans; + uint64_t _maxNodesPerCallstack; uint64_t _queryCacheMaxResultsCount; uint64_t _queryCacheMaxResultsSize; uint64_t _queryCacheMaxEntrySize; diff --git a/js/common/modules/@arangodb/aql/explainer.js b/js/common/modules/@arangodb/aql/explainer.js index ca56497fde47..10d162a60976 100644 --- a/js/common/modules/@arangodb/aql/explainer.js +++ b/js/common/modules/@arangodb/aql/explainer.js @@ -1980,6 +1980,13 @@ function processQuery(query, explain, planIndex) { } }; + const callstackSplit = function(node) { + if (node.isCallstackSplitEnabled) { + return annotation(' /* callstack split */'); + } + return ''; + }; + var constNess = function () { if (isConst) { return ' ' + annotation('/* const assignment */'); @@ -2029,10 +2036,10 @@ function processQuery(query, explain, planIndex) { line += pad(1 + maxCallsLen - String(node.calls).length) + value(node.calls) + ' ' + pad(1 + maxItemsLen - String(node.items).length) + value(node.items) + ' ' + pad(1 + maxRuntimeLen - runtime.length) + value(runtime) + ' ' + - indent(level, node.type === 'SingletonNode') + label(node); + indent(level, node.type === 'SingletonNode') + label(node) + callstackSplit(node); } else { line += pad(1 + maxEstimateLen - String(node.estimatedNrItems).length) + value(node.estimatedNrItems) + ' ' + - indent(level, node.type === 'SingletonNode') + label(node); + indent(level, node.type === 'SingletonNode') + label(node) + callstackSplit(node); } if (node.type === 'CalculationNode') { From e1a62d979cbcde21e3fabf1b5aa45cdafe5d242b Mon Sep 17 00:00:00 2001 From: mpoeter Date: Thu, 10 Jun 2021 15:15:40 +0200 Subject: [PATCH 02/10] Add integration test. --- tests/js/server/aql/aql-explain-all.js | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/tests/js/server/aql/aql-explain-all.js b/tests/js/server/aql/aql-explain-all.js index 90179b604121..49cbc453b122 100644 --- a/tests/js/server/aql/aql-explain-all.js +++ b/tests/js/server/aql/aql-explain-all.js @@ -147,7 +147,7 @@ function explainSuite () { }, //////////////////////////////////////////////////////////////////////////////// -/// @brief test explain w/ a signle plan vs. all plans +/// @brief test explain w/ a single plan vs. all plans //////////////////////////////////////////////////////////////////////////////// testExplainAllPlansVsSingle : function () { @@ -199,6 +199,29 @@ function explainSuite () { assertTrue(Array.isArray(plan.rules)); }); }, + +//////////////////////////////////////////////////////////////////////////////// +/// @brief test callstack split +//////////////////////////////////////////////////////////////////////////////// + + testExplainCallstackSplit : function () { + const query = "FOR i IN " + cn + " FOR j IN " + cn + " FILTER i.x == j.x RETURN [i,j]"; + + let nodes = AQL_EXPLAIN(query, {}, { maxNodesPerCallstack: 1, verbosePlans: true }).plan.nodes; + assertTrue(Array.isArray(nodes)); + + nodes.forEach(function(node) { + assertTrue(node.hasOwnProperty("isCallstackSplitEnabled")); + assertTrue(node.isCallstackSplitEnabled); + }); + + nodes = AQL_EXPLAIN(query, {}, { maxNodesPerCallstack: 2, verbosePlans: true }).plan.nodes; + for (let i = 0; i < nodes.length; ++i) { + const node = nodes[i]; + assertTrue(node.hasOwnProperty("isCallstackSplitEnabled")); + assertEqual((i % 2) != 0, node.isCallstackSplitEnabled); + } + }, }; } From 79094ae962c481fab5efce490ec3313fd3a4d966 Mon Sep 17 00:00:00 2001 From: mpoeter Date: Thu, 10 Jun 2021 15:25:07 +0200 Subject: [PATCH 03/10] Update CHANGELOG. --- CHANGELOG | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGELOG b/CHANGELOG index d73215fae982..7a79a16c0282 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,13 @@ devel ----- +* Fix potential stack overflow when executing large queries. This is + achieved by splitting the callstack and moving part of the execution + to a separate thread. The number of execution nodes after which such + a callstack split should be performed can be configured via the query + option `maxNodesPerCallstack` and the command line option + `--query.max-nodes-per-callstack`; the default is 250. + * Added check to utils/generateAllMetricsDocumentation.py to check that the file name and the value of the name attribute are the same in the metrics documentation snippets. Correct a few such names. From 1018aa9f17f9e6a2c858b110c5b29660335062c2 Mon Sep 17 00:00:00 2001 From: mpoeter Date: Thu, 10 Jun 2021 16:42:53 +0200 Subject: [PATCH 04/10] Fix jslint. --- tests/js/server/aql/aql-explain-all.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/js/server/aql/aql-explain-all.js b/tests/js/server/aql/aql-explain-all.js index 49cbc453b122..11b32e0a9ff2 100644 --- a/tests/js/server/aql/aql-explain-all.js +++ b/tests/js/server/aql/aql-explain-all.js @@ -219,7 +219,7 @@ function explainSuite () { for (let i = 0; i < nodes.length; ++i) { const node = nodes[i]; assertTrue(node.hasOwnProperty("isCallstackSplitEnabled")); - assertEqual((i % 2) != 0, node.isCallstackSplitEnabled); + assertEqual((i % 2) !== 0, node.isCallstackSplitEnabled); } }, From c28d64f355b29c2a15b4c07dbc816e0b1070b1a6 Mon Sep 17 00:00:00 2001 From: mpoeter Date: Fri, 11 Jun 2021 11:42:16 +0200 Subject: [PATCH 05/10] Fix build and add comments. --- arangod/Aql/ExecutionBlockImpl.h | 30 +++++++++++++++++++++++++++++- arangod/Aql/OptimizerRules.cpp | 1 - 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/arangod/Aql/ExecutionBlockImpl.h b/arangod/Aql/ExecutionBlockImpl.h index 73342d5893ad..a73f1b51f4d2 100644 --- a/arangod/Aql/ExecutionBlockImpl.h +++ b/arangod/Aql/ExecutionBlockImpl.h @@ -351,6 +351,23 @@ class ExecutionBlockImpl final : public ExecutionBlock { std::optional _result; }; + /** + * @brief The CallstackSplit class is used for execution blocks that need to + * perform their calls to upstream nodes in a separate thread in order to + * prevent stack overflows. + * + * Execution blocks for which the callstack split has been enabled create a + * single CallstackSplit instance upon creation. The CallstackSplit instance + * manages the new thread for the upstream execution. Instead of calling + * executeFetcher directly, we call Callsplit::execute which stores the call + * parameter, signals the thread and then blocks. The other thread fetches + * the parameters, performs the call to executeFetcher, stores the result and + * notifies the original thread. + * + * This way we can split the callstack over multiple threads and thereby + * avoid stack overflows. + * + */ struct CallstackSplit { explicit CallstackSplit(ExecutionBlockImpl& block); ~CallstackSplit(); @@ -360,7 +377,18 @@ class ExecutionBlockImpl final : public ExecutionBlock { UpstreamResult execute(ExecutionContext& ctx, AqlCallType const& aqlCall); private: - enum class State { Waiting, Signalled, Executing, Stopped }; + enum class State { + /// @brief the thread is waiting to be notified about a pending upstream + /// call or that it should terminate. + Waiting, + /// @brief the thread is currently executing an upstream call. As long + /// as we are in state `Executing` the previous thread is blocked waiting + /// for the result. + Executing, + /// @brief the CallstackSplit instance is getting destroyed and the thread + /// must terminate + Stopped + }; struct Params { std::variant& result; ExecutionContext& ctx; diff --git a/arangod/Aql/OptimizerRules.cpp b/arangod/Aql/OptimizerRules.cpp index 4ff706671cd6..0e0c4cb46023 100644 --- a/arangod/Aql/OptimizerRules.cpp +++ b/arangod/Aql/OptimizerRules.cpp @@ -80,7 +80,6 @@ namespace { bool willUseV8(arangodb::aql::ExecutionPlan const& plan) { - bool result{false}; struct V8Checker : arangodb::aql::WalkerWorkerBase { bool before(arangodb::aql::ExecutionNode* n) override { if (n->getType() == arangodb::aql::ExecutionNode::CALCULATION && From ad3c12daab91f1a5e1dd5ae19f01ef6fa8337184 Mon Sep 17 00:00:00 2001 From: mpoeter Date: Tue, 15 Jun 2021 10:48:32 +0200 Subject: [PATCH 06/10] More tests. --- tests/js/server/aql/aql-explain-all.js | 14 ++++++++++---- tests/js/server/aql/aql-queries-simple.js | 12 ++++++++++++ 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/tests/js/server/aql/aql-explain-all.js b/tests/js/server/aql/aql-explain-all.js index 11b32e0a9ff2..3a87d4d638ae 100644 --- a/tests/js/server/aql/aql-explain-all.js +++ b/tests/js/server/aql/aql-explain-all.js @@ -212,14 +212,20 @@ function explainSuite () { nodes.forEach(function(node) { assertTrue(node.hasOwnProperty("isCallstackSplitEnabled")); - assertTrue(node.isCallstackSplitEnabled); + assertTrue(node.isCallstackSplitEnabled ^ (node.type == "RemoteNode")); }); nodes = AQL_EXPLAIN(query, {}, { maxNodesPerCallstack: 2, verbosePlans: true }).plan.nodes; - for (let i = 0; i < nodes.length; ++i) { - const node = nodes[i]; + print(nodes); + let shouldHaveCallstackSplitEnabled = false; + for (let i = nodes.length; i < 0; --i) { + const node = nodes[i - 1]; assertTrue(node.hasOwnProperty("isCallstackSplitEnabled")); - assertEqual((i % 2) !== 0, node.isCallstackSplitEnabled); + if (node.type == "RemoteNode") { + shouldHaveCallstackSplitEnabled = false; + } + assertEqual(shouldHaveCallstackSplitEnabled, node.isCallstackSplitEnabled); + shouldHaveCallstackSplitEnabled = !shouldHaveCallstackSplitEnabled; } }, diff --git a/tests/js/server/aql/aql-queries-simple.js b/tests/js/server/aql/aql-queries-simple.js index c7fb466377d5..dd5d6b06b71f 100644 --- a/tests/js/server/aql/aql-queries-simple.js +++ b/tests/js/server/aql/aql-queries-simple.js @@ -1335,6 +1335,18 @@ function ahuacatlQuerySimpleTestSuite () { }); }, + testLargeQuery : function() { + let q = ""; + const cnt = 1000; + for (let i = 0; i < cnt; ++i) { + q += `LET v${i} = NOOPT(1)\n`; + } + q += "RETURN v0"; + for (let i = 1; i < cnt; ++i) { + q += ` + v${1}`; + } + assertEqual([cnt], AQL_EXECUTE(q, {}, {optimizer: {rules: ['-all']}}).json); + } }; } From 8d08d89c7e61c141de95ef9d74046b367daf7421 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=B6ter?= Date: Tue, 15 Jun 2021 10:52:21 +0200 Subject: [PATCH 07/10] Update arangod/RestServer/QueryRegistryFeature.cpp Co-authored-by: Jan --- arangod/RestServer/QueryRegistryFeature.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arangod/RestServer/QueryRegistryFeature.cpp b/arangod/RestServer/QueryRegistryFeature.cpp index 97d46ae20df0..c64971ed82f2 100644 --- a/arangod/RestServer/QueryRegistryFeature.cpp +++ b/arangod/RestServer/QueryRegistryFeature.cpp @@ -307,7 +307,8 @@ options->addOption("--query.max-nodes-per-callstack", "maximum number execution nodes on the callstack before " "splitting the remaining nodes into a separate thread", new UInt64Parameter(&_maxNodesPerCallstack), - arangodb::options::makeDefaultFlags(arangodb::options::Flags::Hidden)); + arangodb::options::makeDefaultFlags(arangodb::options::Flags::Hidden)) + .setIntroducedIn(30900); options->addOption("--query.registry-ttl", "default time-to-live of cursors and query snippets (in " From b90cba35eb1194508fa6ffa1a17d9f66be830ab8 Mon Sep 17 00:00:00 2001 From: mpoeter Date: Tue, 15 Jun 2021 11:29:18 +0200 Subject: [PATCH 08/10] Fix jslint. --- tests/js/server/aql/aql-explain-all.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/js/server/aql/aql-explain-all.js b/tests/js/server/aql/aql-explain-all.js index 3a87d4d638ae..30a581efa762 100644 --- a/tests/js/server/aql/aql-explain-all.js +++ b/tests/js/server/aql/aql-explain-all.js @@ -212,16 +212,15 @@ function explainSuite () { nodes.forEach(function(node) { assertTrue(node.hasOwnProperty("isCallstackSplitEnabled")); - assertTrue(node.isCallstackSplitEnabled ^ (node.type == "RemoteNode")); + assertTrue(node.isCallstackSplitEnabled ^ (node.type === "RemoteNode")); }); nodes = AQL_EXPLAIN(query, {}, { maxNodesPerCallstack: 2, verbosePlans: true }).plan.nodes; - print(nodes); let shouldHaveCallstackSplitEnabled = false; for (let i = nodes.length; i < 0; --i) { const node = nodes[i - 1]; assertTrue(node.hasOwnProperty("isCallstackSplitEnabled")); - if (node.type == "RemoteNode") { + if (node.type === "RemoteNode") { shouldHaveCallstackSplitEnabled = false; } assertEqual(shouldHaveCallstackSplitEnabled, node.isCallstackSplitEnabled); From 9be14c769722a274f4d4c92d317314ac544da226 Mon Sep 17 00:00:00 2001 From: mpoeter Date: Mon, 28 Jun 2021 15:12:30 +0200 Subject: [PATCH 09/10] Address review comments. --- arangod/Aql/ExecutionBlockImpl.cpp | 11 ++++++----- arangod/Aql/OptimizerRules.cpp | 3 +++ tests/js/server/aql/aql-queries-simple.js | 20 ++++++++++++++++++-- 3 files changed, 27 insertions(+), 7 deletions(-) diff --git a/arangod/Aql/ExecutionBlockImpl.cpp b/arangod/Aql/ExecutionBlockImpl.cpp index 6939de914f8c..0f721c5d1ded 100644 --- a/arangod/Aql/ExecutionBlockImpl.cpp +++ b/arangod/Aql/ExecutionBlockImpl.cpp @@ -2000,11 +2000,12 @@ auto ExecutionBlockImpl::CallstackSplit::execute(ExecutionContext& ctx -> UpstreamResult { std::variant result{std::nullopt}; Params params{result, ctx, aqlCall}; - - _lock.lock(); - _params = ¶ms; - _state.store(State::Executing); - _lock.unlock(); + + { + std::unique_lock guard(_lock); + _params = ¶ms; + _state.store(State::Executing); + } _bell.notify_one(); diff --git a/arangod/Aql/OptimizerRules.cpp b/arangod/Aql/OptimizerRules.cpp index 0e0c4cb46023..cbbdf9cf6c9a 100644 --- a/arangod/Aql/OptimizerRules.cpp +++ b/arangod/Aql/OptimizerRules.cpp @@ -7916,6 +7916,9 @@ void arangodb::aql::activateCallstackSplit(ExecutionPlan& plan) { struct CallstackSplitter : WalkerWorkerBase { explicit CallstackSplitter(size_t maxNodes) : maxNodesPerCallstack(maxNodes) {} bool before(ExecutionNode* n) override { + // This rule must be executed after subquery splicing, so we must not see any subqueries here! + TRI_ASSERT(n->getType() != EN::SUBQUERY); + if (n->getType() == EN::REMOTE) { // RemoteNodes provide a natural split in the callstack, so we can reset the counter here! count = 0; diff --git a/tests/js/server/aql/aql-queries-simple.js b/tests/js/server/aql/aql-queries-simple.js index dd5d6b06b71f..fd860cf9d3e1 100644 --- a/tests/js/server/aql/aql-queries-simple.js +++ b/tests/js/server/aql/aql-queries-simple.js @@ -1343,10 +1343,26 @@ function ahuacatlQuerySimpleTestSuite () { } q += "RETURN v0"; for (let i = 1; i < cnt; ++i) { - q += ` + v${1}`; + q += ` + v${i}`; } assertEqual([cnt], AQL_EXECUTE(q, {}, {optimizer: {rules: ['-all']}}).json); - } + }, + + testLargeSubQuery : function() { + const _ = require('lodash'); + let q = "FOR i IN 0..9 LET x = (\n"; + const cnt = 900; + for (let i = 0; i < cnt; ++i) { + q += `LET v${i} = NOOPT(1)\n`; + } + q += "RETURN i + v0"; + for (let i = 1; i < cnt; ++i) { + q += ` + v${i}`; + } + q += ")[0]\nRETURN x"; + const expected = _.range(cnt, cnt + 10); + assertEqual(expected, AQL_EXECUTE(q, {}, {optimizer: {rules: ['-all']}}).json); + }, }; } From 77cf2daf4e852582796acd6d70c20ecb652238b4 Mon Sep 17 00:00:00 2001 From: mpoeter Date: Tue, 29 Jun 2021 13:49:45 +0200 Subject: [PATCH 10/10] Fix test. --- tests/js/server/aql/aql-queries-simple.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/js/server/aql/aql-queries-simple.js b/tests/js/server/aql/aql-queries-simple.js index fd860cf9d3e1..f2c07131a87d 100644 --- a/tests/js/server/aql/aql-queries-simple.js +++ b/tests/js/server/aql/aql-queries-simple.js @@ -1337,7 +1337,7 @@ function ahuacatlQuerySimpleTestSuite () { testLargeQuery : function() { let q = ""; - const cnt = 1000; + const cnt = 990; for (let i = 0; i < cnt; ++i) { q += `LET v${i} = NOOPT(1)\n`; }