From efe451e8fcefe408f632258670a9e7a1afd3bc95 Mon Sep 17 00:00:00 2001
From: Michael Hackstein <michael@arangodb.com>
Date: Thu, 25 Apr 2019 09:37:57 +0200
Subject: [PATCH] Reinclude performance optimization of Return in Subquery
 (#8844)

* Removed template parameter from ReturnExecutor and enforce it to be passthrough

* Revert "Removed template parameter from ReturnExecutor and enforce it to be passthrough"
This was unfortunately going into the wrong direction.

This reverts commit 6a488ee1d97f519c7382ebf783e4217cdb403458.

* Create a new block with exactly 1 Register for a ReturnBlock that is supposed to be used as Subquery result

* Fixed register assertoin for return block optimization

* nished comment where i stop in the midlle of a sent
---
 arangod/Aql/ExecutionNode.cpp | 36 +++++++++++++++++++----------------
 arangod/Aql/ExecutorInfos.cpp |  6 +++++-
 2 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/arangod/Aql/ExecutionNode.cpp b/arangod/Aql/ExecutionNode.cpp
index 6203aa0c0b..5563c2473d 100644
--- a/arangod/Aql/ExecutionNode.cpp
+++ b/arangod/Aql/ExecutionNode.cpp
@@ -617,9 +617,7 @@ ExecutionNode const* ExecutionNode::getLoop() const {
     auto type = node->getType();
 
     if (type == ENUMERATE_COLLECTION || type == INDEX || type == TRAVERSAL ||
-        type == ENUMERATE_LIST || type == SHORTEST_PATH
-        || type == ENUMERATE_IRESEARCH_VIEW
-    ) {
+        type == ENUMERATE_LIST || type == SHORTEST_PATH || type == ENUMERATE_IRESEARCH_VIEW) {
       return node;
     }
   }
@@ -1762,17 +1760,16 @@ bool SubqueryNode::mayAccessCollections() {
 
   // if the subquery contains any of these nodes, it may access data from
   // a collection
-  std::vector<ExecutionNode::NodeType> const types = {
-      ExecutionNode::ENUMERATE_IRESEARCH_VIEW,
-      ExecutionNode::ENUMERATE_COLLECTION,
-      ExecutionNode::INDEX,
-      ExecutionNode::INSERT,
-      ExecutionNode::UPDATE,
-      ExecutionNode::REPLACE,
-      ExecutionNode::REMOVE,
-      ExecutionNode::UPSERT,
-      ExecutionNode::TRAVERSAL,
-      ExecutionNode::SHORTEST_PATH};
+  std::vector<ExecutionNode::NodeType> const types = {ExecutionNode::ENUMERATE_IRESEARCH_VIEW,
+                                                      ExecutionNode::ENUMERATE_COLLECTION,
+                                                      ExecutionNode::INDEX,
+                                                      ExecutionNode::INSERT,
+                                                      ExecutionNode::UPDATE,
+                                                      ExecutionNode::REPLACE,
+                                                      ExecutionNode::REMOVE,
+                                                      ExecutionNode::UPSERT,
+                                                      ExecutionNode::TRAVERSAL,
+                                                      ExecutionNode::SHORTEST_PATH};
 
   SmallVector<ExecutionNode*>::allocator_type::arena_type a;
   SmallVector<ExecutionNode*> nodes{a};
@@ -2037,10 +2034,17 @@ std::unique_ptr<ExecutionBlock> ReturnNode::createBlock(
 
   bool const returnInheritedResults = isRoot && !isDBServer;
 
+  // This is an important performance improvement:
+  // If we have inherited results, we do move the block through
+  // and do not modify it in any way.
+  // In the other case it is important to shrink the matrix to exactly
+  // one register that is stored within the DOCVEC.
+  RegisterId const numberOutputRegisters =
+      returnInheritedResults ? getRegisterPlan()->nrRegs[getDepth()] : 1;
+
   ReturnExecutorInfos infos(inputRegister,
                             getRegisterPlan()->nrRegs[previousNode->getDepth()],
-                            getRegisterPlan()->nrRegs[getDepth()], _count,
-                            returnInheritedResults);
+                            numberOutputRegisters, _count, returnInheritedResults);
   if (returnInheritedResults) {
     return std::make_unique<ExecutionBlockImpl<ReturnExecutor<true>>>(&engine, this,
                                                                       std::move(infos));
diff --git a/arangod/Aql/ExecutorInfos.cpp b/arangod/Aql/ExecutorInfos.cpp
index e038a83f42..12e7e6ff94 100644
--- a/arangod/Aql/ExecutorInfos.cpp
+++ b/arangod/Aql/ExecutorInfos.cpp
@@ -50,7 +50,11 @@ ExecutorInfos::ExecutorInfos(
   if (_outRegs == nullptr) {
     _outRegs = std::make_shared<decltype(_outRegs)::element_type>();
   }
-  TRI_ASSERT(nrInputRegisters <= nrOutputRegisters);
+  // The second assert part is from ReturnExecutor special case, we shrink all
+  // results into a single Register column.
+  TRI_ASSERT((nrInputRegisters <= nrOutputRegisters) ||
+             (nrOutputRegisters == 1 && _registersToKeep->empty() &&
+              _registersToClear->empty()));
 
 #ifdef ARANGODB_ENABLE_MAINTAINER_MODE
   for (RegisterId const inReg : *_inRegs) {