Adding resource monitors to MERGE and COLLECT methods #21899

knakatasf · 2025-08-01T16:50:51Z

Scope & Purpose

Adding resource monitors (specifically ResourceUsageScope) to MERGE and COLLECT methods to throw the exception when their performance exceeds a memory usage limit.

MERGE

Added ResourceUsageScope to ExecutorExpressionContext.
Added increaseMemoryUsage() logic to mergeParameters() method of ObjectFunctions.cpp.
Added testing functions to aql-memory-limit.js
Need to investigate how to pass the memory usage that MERGE used to AqlItemBlock.

COLLECT

Added ResourceUsageScope to SortedCollectExecutor.
Added ResourceUsageScope to one of the Aggregators.
Need to add the scope to other aggregators.
Need to add testing functions of this functionality.
💩 Bugfix
🍕 New feature
🔥 Performance improvement
🔨 Refactoring/simplification

Checklist

Related Information

Docs PR:
Enterprise PR:
GitHub issue / Jira ticket:
Design document:

…nctions.cpp

… double counting, add test (must format)

…ge-resource-monitor

…affected gtests

…ge-resource-monitor

…ally in the supervised buffer

…ge-resource-monitor

…monitor

…ge-resource-monitor

…ervised-buffer

…feature/merge-resource-monitor

…d new test

…of steal

…ge-resource-monitor

mchacki

Left some comments and changes, but the review is not yet 100% complete, I will do another one.

mchacki · 2025-08-28T09:40:15Z

arangod/Aql/Aggregator.cpp

@@ -32,6 +32,8 @@
 #include "Transaction/Helpers.h"
 #include "Transaction/Methods.h"

+#include "Logger/LogMacros.h"


Suggested change

#include "Logger/LogMacros.h"

mchacki · 2025-08-28T09:43:07Z

arangod/Aql/AqlValue.h

@@ -28,6 +28,7 @@
 #include "Aql/RegisterId.h"
 #include "Aql/RegIdFlatSet.h"
 #include "Basics/Endian.h"
+#include "Basics/ResourceUsage.h"


Why is this needed here?
I would prefer to do the import in files that actually require it directly, this way we can save on recompile costs.

mchacki · 2025-08-28T12:51:32Z

arangod/Aql/ExecutionNode/CollectNode.cpp

@@ -356,7 +360,7 @@ std::unique_ptr<ExecutionBlock> CollectNode::createBlock(
          std::move(groupRegisters), collectRegister, expressionRegister,
          _expressionVariable, std::move(aggregateTypes),
          std::move(inputVariables), std::move(aggregateRegisters),
-          &_plan->getAst()->query().vpackOptions());
+          &_plan->getAst()->query().vpackOptions(), std::move(usageScope));


Puh, we need to make sure that destruction ordering here is correct.

We reference the usageScope in the aggregators above.
We hand over responsibility into the Infos here.
Now on destructor we need to make sure aggregator vector is always destroyed before the usageScope, otherwise we get into undefined behavior here.

I am thinking:
Instead of handing in one Global scope,
should we instead give a reference to the ResourceMonitor& and then let each struct internally build it's own local scope? The ResourceMonitor is guaranteed to outlive this class.

mchacki · 2025-08-28T12:52:20Z

arangod/Aql/ExecutionNode/IndexCollectNode.cpp

@@ -101,6 +101,8 @@ std::unique_ptr<ExecutionBlock> IndexCollectNode::createBlockAggregationScan(
  infos.groups.reserve(_groups.size());
  infos.index = _index;
  infos.query = &engine.getQuery();
+  infos.usageScope = std::make_unique<ResourceUsageScope>(


Why unique_ptr?
We could probably get away with a ResourceUsageScope directly right?

mchacki · 2025-08-28T12:54:45Z

arangod/Aql/ExecutionNode/WindowNode.cpp

@@ -394,10 +394,11 @@ void WindowNode::calcAggregateRegisters(
 }

 void WindowNode::calcAggregateTypes(


Am i blind or is there no one calling this function that would have to be adjusted?

mchacki · 2025-08-28T12:59:22Z

arangod/Aql/Executor/HashedCollectExecutor.h

@@ -137,6 +138,7 @@ class HashedCollectExecutorInfos {

  /// @brief resource manager
  arangodb::ResourceMonitor& _resourceMonitor;
+  std::unique_ptr<ResourceUsageScope> _usageScope;


puh, i am getting afraid of destruction ordering here again.

I think moving the Scope as member of the aggregator is way safer.

mchacki · 2025-08-28T13:02:18Z

arangod/Aql/Executor/SortedCollectExecutor.cpp

-      input.getValue(infos.getExpressionRegister())
-          .toVelocyPack(infos.getVPackOptions(), _builder,
-                        /*allowUnindexed*/ false);
+      auto val = input.getValue(infos.getExpressionRegister());


I think this can be replaced with the Supervised Builder now

mchacki · 2025-08-28T13:02:58Z

arangod/Aql/Executor/SortedCollectExecutor.cpp

@@ -266,16 +281,27 @@ void SortedCollectExecutor::CollectGroup::writeToOutput(
  // set the group values
  if (infos.getCollectRegister().value() != RegisterId::maxRegisterId) {
    TRI_ASSERT(_builder.isOpenArray());
+    size_t before = _buffer.size();


Same here let us make this _buffer a SupervisedBuffer

mchacki · 2025-08-28T13:06:17Z

arangod/Aql/Executor/WindowExecutor.cpp

@@ -92,7 +94,8 @@ BaseWindowExecutor::AggregatorList BaseWindowExecutor::createAggregators(
  // initialize aggregators
  for (auto const& r : infos.getAggregateTypes()) {
    auto& factory = Aggregator::factoryFromTypeString(r);
-    aggregators.emplace_back(factory(infos.getVPackOptions()));
+    aggregators.emplace_back(


again, destruction ordering

…ge-resource-monitor

…buffer

cla-bot bot added the cla-signed label Aug 1, 2025

knakatasf added 6 commits August 7, 2025 02:12

feature: Add ResourceMonitor to ExecutorExpressionContext

fa9d19f

feature: Add ResourceMonitor to ExecutorExpresionContext

47c58d5

chore: Add tests for resource monitor for MERGE

ebac27d

feature: Add resource monitor to aggregators

c827ed9

feature: add resource usage scope to Aggregators

b21ebe6

chore: Correct memory counting logic in mergeParameters() in ObjectFu…

64c2c45

…nctions.cpp

knakatasf force-pushed the feature/merge-resource-monitor branch from 8cc0119 to 64c2c45 Compare August 7, 2025 00:23

cpjulia force-pushed the feature/merge-resource-monitor branch from 64c2c45 to 4f64c8e Compare August 7, 2025 14:31

knakatasf force-pushed the feature/merge-resource-monitor branch from 4f64c8e to 64c2c45 Compare August 7, 2025 16:58

cpjulia and others added 20 commits August 8, 2025 12:28

Reserve memory before large operations and subtract estimate to avoid…

c877a40

… double counting, add test (must format)

Merge branch 'devel' of github.com:arangodb/arangodb into feature/mer…

79a05c1

…ge-resource-monitor

Add SupervisedBuffer for centralized memory accounting

ab9eebc

Merge branch 'devel' of github.com:arangodb/arangodb into feature/mer…

ff6a5b0

…ge-resource-monitor

Add new js tests, fix test name, avoid double memory accounting, fix …

a1f4af8

…affected gtests

Merge branch 'devel' of github.com:arangodb/arangodb into feature/mer…

7765f5a

…ge-resource-monitor

Merge branch 'devel' of github.com:arangodb/arangodb into feature/mer…

8eb0e15

…ge-resource-monitor

Introduced overriding of Buffer functions to account memory automatic…

94d7952

…ally in the supervised buffer

add new file to CMakeLists

ae8b638

Run clang-format, but leave log messages for now

9cbf0bd

chore: Correct memory counting logic of INTO, SortedCollectExecutor

7d84712

chore: Correct memory accounting logic in AggregatorUnique

2cbc538

chore: Add test cases for SortedCollectExecutor in aql-memory-limit.js

fbf31c8

Merge branch 'devel' of github.com:arangodb/arangodb into feature/mer…

0a7dfb6

…ge-resource-monitor

Update velocypack submodule to feature/supervised-buffer

6d0c8c2

Merge branch 'feature/supervised-buffer' into feature/merge-resource-…

fa785d1

…monitor

chore: Correct PUSH aggregator logic; Add tests in aql-memory-limit.js

e0d5f8f

Merge branch 'devel' of github.com:arangodb/arangodb into feature/mer…

c7d176c

…ge-resource-monitor

Merge branch 'devel' of github.com:arangodb/arangodb into feature/sup…

58856cf

…ervised-buffer

Override the clear method to account for released memory

b079443

cpjulia added 12 commits August 26, 2025 15:32

Merge branch 'devel' of github.com:arangodb/arangodb into feature/sup…

d5bed73

…ervised-buffer

Update velocypack submodule to feature/supervised-buffer

1de8853

Remove countMemory boolean and function that uses it

4ce43a8

Remove count memory usage and helper function buildSupervisedAqlValue

0b83b37

Merge remote-tracking branch 'origin/feature/supervised-buffer' into …

333f4b2

…feature/merge-resource-monitor

Remove comments, unused include

6f04984

Fix mergeParameters to properly use optional buffer and fix tests, ad…

fc02534

…d new test

Fix supervised buffer steal method to revert the usage scope instead …

a090621

…of steal

Make steal method in supervised buffer revert the scope, not steal it

62f6b81

Fixed syntax, added test

6c25ddc

Include assertion

a17a966

Merge branch 'devel' of github.com:arangodb/arangodb into feature/mer…

9273610

…ge-resource-monitor

mchacki requested changes Aug 28, 2025

View reviewed changes

cpjulia and others added 17 commits August 28, 2025 23:42

Merge branch 'devel' of github.com:arangodb/arangodb into feature/mer…

df73e45

…ge-resource-monitor

Change signatures of methors to get resource monitor ref

1ba3bff

Fix formatting

e96c332

Remove manual memory accounting, fix formatting

f29774f

Fix git metalanguage inside file

46c2a5e

Remove log message

433dde0

Make usage scope stack allocated

07b3252

Fix aggregators buffer usage

a7d6431

Implement usage of resource monitor and supervised buffer

dc1dc97

Fix RAII safety of sorted collect executor

c6f8478

Remove local files

fd7730c

Remove unused include

6b8dc30

Remove comment

cc3b4d3

Remove unused include

18b696f

Remove unused include and useles formatting

85472b1

Remove usage scope from window executor as not needed

77a24b2

Make memory accounting automatic in hashed collector, use supervised …

871d7be

…buffer

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Adding resource monitors to MERGE and COLLECT methods #21899

Adding resource monitors to MERGE and COLLECT methods #21899

Uh oh!

knakatasf commented Aug 1, 2025

Uh oh!

mchacki left a comment

Uh oh!

mchacki Aug 28, 2025

Uh oh!

mchacki Aug 28, 2025

Uh oh!

mchacki Aug 28, 2025

Uh oh!

mchacki Aug 28, 2025

Uh oh!

mchacki Aug 28, 2025

Uh oh!

mchacki Aug 28, 2025

Uh oh!

mchacki Aug 28, 2025

Uh oh!

mchacki Aug 28, 2025

Uh oh!

mchacki Aug 28, 2025

Uh oh!

Uh oh!

		@@ -394,10 +394,11 @@ void WindowNode::calcAggregateRegisters(
		}

		void WindowNode::calcAggregateTypes(

Adding resource monitors to MERGE and COLLECT methods #21899

Are you sure you want to change the base?

Adding resource monitors to MERGE and COLLECT methods #21899

Uh oh!

Conversation

knakatasf commented Aug 1, 2025

Scope & Purpose

Checklist

Related Information

Uh oh!

mchacki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!