Add arbitration participant and operation objects to support global memory arbitration optimization #11074
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
|
This pull request was exported from Phabricator. Differential Revision: D63055730 |
tanjialiang
reviewed
Sep 24, 2024
Comment on lines
+58
to
+77
| uint64_t fastExponentialGrowthCapacityLimit; | ||
| double slowCapacityGrowRatio; | ||
|
|
||
| /// When shrinking a memory pool capacity, the shrink bytes will be adjusted | ||
| /// in a way such that AFTER shrink, the stricter (whichever is smaller) of | ||
| /// the following conditions is met, in order to better fit the query memory | ||
| /// pool's current memory usage: | ||
| /// - Free capacity is greater or equal to capacity * | ||
| /// 'minFreeCapacityRatio' | ||
| /// - Free capacity is greater or equal to 'minFreeCapacity' | ||
| /// | ||
| /// NOTE: in the conditions when original requested shrink bytes ends up | ||
| /// with more free capacity than above 2 conditions, the adjusted shrink | ||
| /// bytes is not respected. | ||
| /// | ||
| /// NOTE: capacity shrink adjustment is enabled when both | ||
| /// 'minFreeCapacityRatio' and 'minFreeCapacity' are set. | ||
| uint64_t minFreeCapacity; | ||
| double minFreeCapacityRatio; |
There was a problem hiding this comment.
Shall we make the names consistent with other code locations? Here are the names in SharedArbitrator
const uint64_t fastExponentialGrowthCapacityLimit_;
const double slowCapacityGrowPct_;
const uint64_t memoryPoolMinFreeCapacity_;
const double memoryPoolMinFreeCapacityPct_;
There was a problem hiding this comment.
This are the configs defined in ArbitrationParticipant for memory pool so we don't need memory pool prefix, and we shall change slowCapacityGrowPct_ to slowCapacityGrowRatio_
| /// capacity growth target is set to have a coarser granularity. It can help | ||
| /// to reduce the number of future grow calls, and hence reducing the number | ||
| /// of unnecessary memory arbitration requests. | ||
| void getGrowTargets( |
There was a problem hiding this comment.
Since we are doing the refactoring, could you also help to add documentations on the 2 return values? max and min grow bytes. Thanks
| ArbitrationParticipant::Config::Config( | ||
| uint64_t _minCapacity, | ||
| uint64_t _fastExponentialGrowthCapacityLimit, | ||
| double _slowCapacityGrowRatio, |
There was a problem hiding this comment.
ditto, shall we align these names?
| } | ||
|
|
||
| uint64_t ArbitrationParticipant::maxShrinkCapacity() const { | ||
| const uint64_t capacity = pool_->capacity(); |
There was a problem hiding this comment.
this local v is only used once
| /// for the prior arbitration operations to finish first before executing to | ||
| /// ensure the serialized execution of arbitration operations from the same | ||
| /// query memory pool. | ||
| void startArbitration(ArbitrationOperation* op); |
There was a problem hiding this comment.
Shall we rename this method to enqueueArbitration() as technically the arbitration is not started by this method?
There was a problem hiding this comment.
When the function returns, the operation is started so I think it is better to call this startArbitration.
tanjialiang
reviewed
Sep 24, 2024
| /// latter ensures the serialized execution of arbitration operations from the | ||
| /// same query with one at a time. So this method blocks until all the prior | ||
| /// arbitration operations finish. | ||
| void start(); |
There was a problem hiding this comment.
similarly, start() method does not technically start the arbitration. Shall we rename it?
tanjialiang
reviewed
Sep 24, 2024
tanjialiang
reviewed
Sep 24, 2024
tanjialiang
reviewed
Sep 24, 2024
tanjialiang
reviewed
Sep 24, 2024
tanjialiang
reviewed
Sep 24, 2024
tanjialiang
reviewed
Sep 24, 2024
tanjialiang
reviewed
Sep 24, 2024
Comment on lines
+82
to
+93
| uint64_t maxGrowBytes() const { | ||
| return maxGrowBytes_; | ||
| } | ||
|
|
||
| /// Returns the min grow bytes for this arbitration operation to ensure the | ||
| /// arbitration participant has the minimum amount of memory capacity. The | ||
| /// arbitrator might allocate memory from the reserved memory capacity pool | ||
| /// for the min grow bytes. | ||
| uint64_t minGrowBytes() const { | ||
| return minGrowBytes_; |
There was a problem hiding this comment.
These 2 methods going to be used in the followup PRs?
tanjialiang
reviewed
Sep 24, 2024
|
|
||
| /// Returns the allocated bytes by this arbitration operation. | ||
| uint64_t& allocatedBytes() { | ||
| return allocatedBytes_; |
There was a problem hiding this comment.
just make it public or protected if needed to be accessed like this?
There was a problem hiding this comment.
Yes, in followup.
tanjialiang
reviewed
Sep 24, 2024
tanjialiang
approved these changes
Sep 24, 2024
xiaoxmeng
added a commit
to xiaoxmeng/velox
that referenced
this pull request
Sep 24, 2024
…emory arbitration optimization (facebookincubator#11074) Summary: Pull Request resolved: facebookincubator#11074 Add arbitration participant object to provide all the required arbitration operations and state management on a query memory pool inside the memory arbitrator, such as arbitration queue to serialize the arbitration request execution from the same query and the serialize the reclaim, shrink, grow and abort from either arbitration request and the background memory arbitrations. Add arbitration operation object to manage a memory arbitration request execution Reviewed By: tanjialiang Differential Revision: D63055730
xiaoxmeng
force-pushed
the
export-D63055730
branch
from
b4a8713
to
8157161
Compare
|
This pull request was exported from Phabricator. Differential Revision: D63055730 |
1 similar comment
|
This pull request was exported from Phabricator. Differential Revision: D63055730 |
xiaoxmeng
added a commit
to xiaoxmeng/velox
that referenced
this pull request
Sep 24, 2024
…emory arbitration optimization (facebookincubator#11074) Summary: Pull Request resolved: facebookincubator#11074 Add arbitration participant object to provide all the required arbitration operations and state management on a query memory pool inside the memory arbitrator, such as arbitration queue to serialize the arbitration request execution from the same query and the serialize the reclaim, shrink, grow and abort from either arbitration request and the background memory arbitrations. Add arbitration operation object to manage a memory arbitration request execution Reviewed By: tanjialiang Differential Revision: D63055730
xiaoxmeng
force-pushed
the
export-D63055730
branch
from
8157161
to
1f07150
Compare
|
This pull request was exported from Phabricator. Differential Revision: D63055730 |
xiaoxmeng
added a commit
to xiaoxmeng/velox
that referenced
this pull request
Sep 24, 2024
…emory arbitration optimization (facebookincubator#11074) Summary: Pull Request resolved: facebookincubator#11074 Add arbitration participant object to provide all the required arbitration operations and state management on a query memory pool inside the memory arbitrator, such as arbitration queue to serialize the arbitration request execution from the same query and the serialize the reclaim, shrink, grow and abort from either arbitration request and the background memory arbitrations. Add arbitration operation object to manage a memory arbitration request execution Reviewed By: tanjialiang Differential Revision: D63055730
xiaoxmeng
force-pushed
the
export-D63055730
branch
from
1f07150
to
6bf4d98
Compare
…emory arbitration optimization (facebookincubator#11074) Summary: Pull Request resolved: facebookincubator#11074 Add arbitration participant object to provide all the required arbitration operations and state management on a query memory pool inside the memory arbitrator, such as arbitration queue to serialize the arbitration request execution from the same query and the serialize the reclaim, shrink, grow and abort from either arbitration request and the background memory arbitrations. Add arbitration operation object to manage a memory arbitration request execution Reviewed By: tanjialiang Differential Revision: D63055730
|
This pull request was exported from Phabricator. Differential Revision: D63055730 |
xiaoxmeng
force-pushed
the
export-D63055730
branch
from
6bf4d98
to
cd51932
Compare
|
This pull request has been merged in dcaae29. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Add arbitration participant object to provide all the required arbitration operations and state management on a
query memory pool inside the memory arbitrator, such as arbitration queue to serialize the arbitration request execution
from the same query and the serialize the reclaim, shrink, grow and abort from either arbitration request and the
background memory arbitrations.
Add arbitration operation object to manage a memory arbitration request execution
Differential Revision: D63055730