Feature/collect resource monitor #21930

knakatasf · 2025-08-29T03:02:45Z

Scope & Purpose

Adding memory monitoring mechanism to COLLECT method.

Each CollectExecutor takes ResourceMonitor and use supervised builder. For AqlValue, it uses ResourceUsageScope to count its memory.
Each Aggregator takes ResourceMonitor and use supervised builder. For AqlValue, it uses ResourceUsageScope to count its memory.

💩 Bugfix
🍕 New feature
🔥 Performance improvement
🔨 Refactoring/simplification

Checklist

Related Information

(Please reference tickets / specification / other PRs etc)

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

…increase memory

…rceMonitor

…ervised-buffer

…feature/merge-resource-monitor

…arangodb into feature/merge-resource-monitor

…s argument to helper function, format, fix wrong method call in header

…ervised-buffer

…ge-resource-monitor

…structor because of usage scope's default constructor demanding a monitor, fix order in CMakeLists

…feature/merge-resource-monitor

…ge-resource-monitor

…feature/merge-resource-monitor

… instead of ResourceUsageScope&

mchacki

This PR is on a very good way I like it !!

I have left some feedback comments.

mchacki · 2025-08-29T10:54:17Z

arangod/Aql/Aggregator.h

+
+  // static std::unique_ptr<Aggregator> fromTypeString(velocypack::Options
+  // const*,
+  //                                                 std::string_view type);


Suggested change

// static std::unique_ptr<Aggregator> fromTypeString(velocypack::Options

// const*,

// std::string_view type);

just remove this, we can always go back with git history.

mchacki · 2025-08-29T10:55:24Z

arangod/Aql/Aggregator.h

 protected:
  velocypack::Options const* _vpackOptions;
+  ResourceMonitor& _resourceMonitor;
+  std::unique_ptr<ResourceUsageScope> _usageScope;


Suggested change

std::unique_ptr<ResourceUsageScope> _usageScope;

ResourceUsageScope _usageScope;

This should be enough right?

We will perform counters to usage scope often, so having the pointer indirection will have some performance impact.

Got it. Changed to an object by value.

mchacki · 2025-08-29T10:56:49Z

arangod/Aql/AqlValue.h

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


Please include those in the files where it actually is needed.

AqlValue.h is Imported into many places so we would need to recompile a lot if any of those imports change.

Deleted. AqlValue.h doesn't use this header file.

mchacki · 2025-08-29T10:56:55Z

arangod/Aql/AqlValue.h

 #include "IResearch/Misc.h"

+#include <velocypack/Builder.h>


Please include those in the files where it actually is needed.

AqlValue.h is Imported into many places so we would need to recompile a lot if any of those imports change.

mchacki · 2025-08-29T10:57:39Z

arangod/Aql/ExecutionNode/CollectNode.cpp

@@ -37,6 +37,7 @@
 #include "Aql/WalkerWorker.h"
 #include "Transaction/Methods.h"



This file looks good 👍

mchacki · 2025-08-29T11:47:23Z

arangod/Aql/Aggregator.cpp

@@ -205,7 +212,9 @@ struct AggregatorMax final : public Aggregator {
  void reduce(AqlValue const& cmpValue) override {
    if (value.isEmpty() ||
        AqlValue::Compare(_vpackOptions, value, cmpValue, true) < 0) {
+      resourceUsageScope().decrease(value.memoryUsage());


mchacki · 2025-08-29T11:49:59Z

arangod/Aql/Aggregator.cpp

+      : Aggregator(opts, monitor),
+        allocator(1024),
+        seen(_vpackOptions),
+        builder(std::make_shared<velocypack::SupervisedBuffer>(monitor)) {}


Looks like the changes here get rather straight forward.

mchacki · 2025-08-29T11:51:28Z

arangod/Aql/Aggregator.cpp

@@ -776,6 +809,7 @@ struct AggregatorCountDistinct : public Aggregator {
      return;
    }

+    resourceUsageScope().increase(s.byteSize());


Very good 👍

please make rue the resourceUsageScope is reverted when seen is cleared above.

Yes, I added revert() here:

mchacki · 2025-08-29T11:52:19Z

arangod/Aql/Aggregator.cpp

@@ -842,6 +887,7 @@ struct AggregatorCountDistinctStep2 final : public AggregatorCountDistinct {
        continue;
      }

+      resourceUsageScope().increase(it.byteSize());


Again reset on clear()

This Aggregator uses AggregatorCountDistinct's reset(), which is final.

mchacki · 2025-08-29T11:53:22Z

arangod/Aql/Aggregator.cpp

@@ -988,8 +1043,8 @@ struct AggregatorList : public Aggregator {
    // if preceding is `unbounded`. But closing the array here breaks the
    // velocypack slice.
    auto builderCopy = builder;
-    builderCopy.close();
-    return AqlValue(std::move(*builderCopy.steal()));
+    AqlValue a{std::move(*builderCopy.steal())};


Why does the builderCopy not be closed anymore?

Reverted to the original code where builderCopy is closed.

knakatasf and others added 30 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

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

Remove noecept from overriden methods as now can throw

c924075

Remove comment

ee48752

Give back the noexcept because methods don't actually throw as don't …

462cb41

…increase memory

Added supervised buffer test

ad3be4a

knakatasf and others added 27 commits August 22, 2025 19:17

chore: Change ExecutorExpressionContext so it hold reference of Resou…

3d53eb5

…rceMonitor

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

ca22a83

…ervised-buffer

Fixed errors in syntax

af6205b

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

3f28666

…feature/merge-resource-monitor

Update velocypack with merged branch

84aa621

Merge branch 'feature/merge-resource-monitor' of github.com:arangodb/…

dd510c3

…arangodb into feature/merge-resource-monitor

Update mergeParameters and helper function to not use scope and format

4b6e93b

Add test that reuses the builder, pass the monitor instead of scope a…

e10cd2f

…s argument to helper function, format, fix wrong method call in header

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

be8138b

…ervised-buffer

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

4c399b0

…ge-resource-monitor

Fix syntax, remove heap allocation of usage scope, delete default con…

3406b1b

…structor because of usage scope's default constructor demanding a monitor, fix order in CMakeLists

Removed LOG_DEVEL, fix ObjectFunctions wrt SupervisedBuffer, added test

191fe1e

Remove js test

20470f4

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

922c311

…feature/merge-resource-monitor

Removed LOG_DEVEL

2ffded7

Fix namespace syntax

a09a880

Fix pointer syntax in ObjectFunctions

7a1d719

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

24d57b3

…ge-resource-monitor

Make inheritance public, remove helpers

f80048f

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

0d40598

…feature/merge-resource-monitor

Remove helper usage of supervised buffer

c5c3300

Remove countMemory boolean and function that uses it

4ce43a8

Remove count memory usage and helper function buildSupervisedAqlValue

0b83b37

Add SupervisedBuffer to CollectSortedExecutor

e0d29f4

Changed SupervisedBuffer's steal()

1ffa760

chore: Refactor Aggregator instantiation so it takes ResourceMonitor&…

05a72dd

… instead of ResourceUsageScope&

Added SupervisedBuffer to each Aggregator

691ddf0

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

mchacki requested changes Aug 29, 2025

View reviewed changes

Changed based on the reviews on PR

1b8d3ac

	std::unique_ptr<ResourceUsageScope> _usageScope;
	ResourceUsageScope _usageScope;

		@@ -37,6 +37,7 @@
		#include "Aql/WalkerWorker.h"
		#include "Transaction/Methods.h"

Feature/collect resource monitor #21930

Are you sure you want to change the base?

Feature/collect resource monitor #21930

Uh oh!

Conversation

knakatasf commented Aug 29, 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!

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!