Adding resource monitors to MERGE and COLLECT methods #21899
Conversation
8cc0119 to
64c2c45
Compare
64c2c45 to
4f64c8e
Compare
4f64c8e to
64c2c45
Compare
… double counting, add test (must format)
…ge-resource-monitor
…ge-resource-monitor
…ge-resource-monitor
…ge-resource-monitor
…ally in the supervised buffer
…ge-resource-monitor
…ge-resource-monitor
…feature/merge-resource-monitor
…feature/merge-resource-monitor
…ge-resource-monitor
…ge-resource-monitor
…arangodb into feature/merge-resource-monitor
…hat was missing from mergeParameters
…ge-resource-monitor
| TEST(SupervisedBufferTest, CapacityAccounting_GrowAndClear) { | ||
| ResourceMonitor monitor(nullptr); | ||
| std::size_t initial = monitor.current(); | ||
|
|
There was a problem hiding this comment.
ASSERT initial == 0, for good measure
|
|
||
| ASSERT_GT(monitor.current(), initial); | ||
|
|
||
| supervisedBuffer.clear(); |
There was a problem hiding this comment.
Please do a clone of this test without the clear() call.
We need both test:
explicit clear: Reduce memory
go out of scope: reduce memory.
There was a problem hiding this comment.
Wait a second.
Is this actually expected?
Isn't the idea of clear that the capacity of the buffer stays, so it can be recycled?
| ASSERT_EQ(monitor.current(), initial); | ||
| } | ||
|
|
||
| TEST(SupervisedBufferTest, EnforcesLimit_OnGrowth) { |
There was a problem hiding this comment.
Good test.
I think there are some more options in a builder that can increase the buffer.
I would like to ask for some more detailed test.
e.g. something like this:
Open an array
Add some elements, Assert memory is low.
Add some more elements until resize needs to happen, Assert memory is higher.
Also add asserts that current memory is always >= builder.size()
So we are sure we are counting correctly.
Furthremore please check the Builder API for the method that allows to recycle the Buffer.
Call it in the test and assert that the Memory stays high.
| #include <velocypack/Builder.h> | ||
| #include <string> | ||
|
|
||
| using arangodb::GlobalResourceMonitor; |
There was a problem hiding this comment.
This is fine,
However we often use using namespace arangodb;
using namespace arangodb::aql;
It is a bit less to read.
but really optional comment here.
| builder.add(Value(std::string(1024, 'a'))); | ||
| builder.close(); | ||
|
|
||
| ASSERT_EQ(monitor.current(), 0); |
There was a problem hiding this comment.
This is unexpected to me!
We are writing something into the builder, why is it not consuming memory?
| builder.openArray(); | ||
| builder.add(Value(1)); | ||
| builder.close(); | ||
| ASSERT_GT(monitor.current(), 0); |
There was a problem hiding this comment.
I do not understand this.
Why is it here GT 0
But in above comment it is equal?
This one is what I expect!
…ge-resource-monitor
…feature/merge-resource-monitor
…arangodb into feature/merge-resource-monitor
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
ResourceUsageScopetoExecutorExpressionContext.increaseMemoryUsage()logic tomergeParameters()method ofObjectFunctions.cpp.aql-memory-limit.jsAqlItemBlock.COLLECT
Added
ResourceUsageScopetoSortedCollectExecutor.Added
ResourceUsageScopeto one of theAggregators.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