New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Add priority based memory reclaim framework #11598

Closed

tanjialiang wants to merge 1 commit into facebookincubator:main from tanjialiang:pri_reclaim

Contributor

tanjialiang commented Nov 19, 2024 •

edited

Loading

Adds priority base reclaiming to memory reclaim framework. The priority determines which memory pool to reclaim first on the same level. This would help to make reclaim more application logic aware.
Make join node reclaim priority lower than others. This is because cost of reclaiming (spilling) on join node is high compared to other nodes.

facebook-github-bot added the CLA Signed label

netlify bot commented Nov 19, 2024 •

edited

Loading

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`f89c432`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/67527439a9d2360008ff954b

tanjialiang force-pushed the pri_reclaim branch from 38a339b to 1661131 Compare

November 20, 2024 21:51

tanjialiang marked this pull request as ready for review

November 20, 2024 21:54

tanjialiang requested a review from majetideepak as a code owner

November 20, 2024 21:54

Contributor

facebook-github-bot commented Nov 20, 2024

@tanjialiang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

tanjialiang force-pushed the pri_reclaim branch 2 times, most recently from d5693b4 to e352ec7 Compare

November 25, 2024 20:23

xiaoxmeng reviewed

View reviewed changes

Contributor

xiaoxmeng left a comment

@tanjialiang LGTM % minors. Thanks!

velox/exec/HashJoinBridge.h Outdated Show resolved Hide resolved

velox/common/memory/MemoryArbitrator.cpp Outdated Show resolved Hide resolved

velox/connectors/hive/HiveDataSink.h Outdated Show resolved Hide resolved

velox/common/memory/MemoryArbitrator.cpp Show resolved Hide resolved

velox/exec/Task.h Outdated Show resolved Hide resolved

velox/exec/Task.h Outdated Show resolved Hide resolved

velox/exec/MemoryReclaimer.cpp Outdated

-                  if (candidate.reclaimableBytes == 0) {
-                    continue;
+                uint64_t reclaimedBytes{0};
+                for (uint32_t i = 0, j = i; i < candidates.size(); i = j) {

Contributor

xiaoxmeng Dec 3, 2024

Why we need to handle priority in parallel memory reclaimer? thanks!

Contributor Author

tanjialiang Dec 3, 2024

Right, we don't need to. They all run at once.

tanjialiang force-pushed the pri_reclaim branch 7 times, most recently from d65eaf7 to beec9aa Compare

December 3, 2024 17:17

Contributor

facebook-github-bot commented Dec 3, 2024

@tanjialiang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

xiaoxmeng approved these changes

View reviewed changes

Contributor

xiaoxmeng left a comment

@tanjialiang thanks for the update % minors.

velox/exec/HashJoinBridge.h

@@ @@ -174,9 +174,10 @@ bool isLeftNullAwareJoinWithFilter( @@
               class HashJoinMemoryReclaimer final : public MemoryReclaimer {
                public:
                 static std::unique_ptr<memory::MemoryReclaimer> create(
-                    std::shared_ptr<HashJoinBridge> joinBridge) {
+                    std::shared_ptr<HashJoinBridge> joinBridge,
+                    int32_t priority = 0) {

Contributor

xiaoxmeng Dec 3, 2024

Do we enforce a priority input later?

Contributor Author

tanjialiang Dec 3, 2024

Yeah we can probably enforce that later. But now we don't want to enforce as the current priority setting is on Task level.

velox/exec/Task.h Outdated

@@ @@ -69,14 +69,17 @@ class Task : public std::enable_shared_from_this<Task> { @@
                 /// thread are passed on to a separate consumer.
                 /// @param onError Optional callback to receive an exception if task
                 /// execution fails.
+                /// @param memoryArbitrationPriority Optional priority on task that, in a

Contributor

xiaoxmeng Dec 3, 2024

Let's be specific for memory arbitration for now?

Contributor Author

tanjialiang Dec 3, 2024

yeah

tanjialiang force-pushed the pri_reclaim branch from beec9aa to e46ef29 Compare

December 3, 2024 20:28

tanjialiang added a commit to tanjialiang/velox-1 that referenced this pull request


          feat: Add priority based memory reclaim framework (facebookincubator#…

e46ef29

…11598)

Summary:
* Adds priority base reclaiming to memory reclaim framework. The priority determines which memory pool to reclaim first on the same level. This would help to make reclaim more application logic aware. 
* Make join node reclaim priority lower than others. This is because cost of reclaiming (spilling) on join node is high compared to other nodes.


Reviewed By: xiaoxmeng

Differential Revision: D66261340

Pulled By: tanjialiang

Contributor

facebook-github-bot commented Dec 3, 2024

This pull request was exported from Phabricator. Differential Revision: D66261340

facebook-github-bot added the fb-exported label

tanjialiang added a commit to tanjialiang/velox-1 that referenced this pull request


          feat: Add priority based memory reclaim framework (facebookincubator#…

d0a22d0

…11598)

Summary:
* Adds priority base reclaiming to memory reclaim framework. The priority determines which memory pool to reclaim first on the same level. This would help to make reclaim more application logic aware. 
* Make join node reclaim priority lower than others. This is because cost of reclaiming (spilling) on join node is high compared to other nodes.


Reviewed By: xiaoxmeng

Differential Revision: D66261340

Pulled By: tanjialiang

tanjialiang force-pushed the pri_reclaim branch from e46ef29 to d0a22d0 Compare

December 3, 2024 21:32

Contributor

facebook-github-bot commented Dec 3, 2024

This pull request was exported from Phabricator. Differential Revision: D66261340

tanjialiang added a commit to tanjialiang/velox-1 that referenced this pull request


          feat: Add priority based memory reclaim framework (facebookincubator#…

6dd01a9

…11598)

Summary:
* Adds priority base reclaiming to memory reclaim framework. The priority determines which memory pool to reclaim first on the same level. This would help to make reclaim more application logic aware. 
* Make join node reclaim priority lower than others. This is because cost of reclaiming (spilling) on join node is high compared to other nodes.


Reviewed By: xiaoxmeng

Differential Revision: D66261340

Pulled By: tanjialiang

tanjialiang force-pushed the pri_reclaim branch from d0a22d0 to 6dd01a9 Compare

December 3, 2024 23:29

Contributor

facebook-github-bot commented Dec 3, 2024

This pull request was exported from Phabricator. Differential Revision: D66261340

tanjialiang added a commit to tanjialiang/velox-1 that referenced this pull request


          feat: Add priority based memory reclaim framework (facebookincubator#…

078c12a

…11598)

Summary:
* Adds priority base reclaiming to memory reclaim framework. The priority determines which memory pool to reclaim first on the same level. This would help to make reclaim more application logic aware. 
* Make join node reclaim priority lower than others. This is because cost of reclaiming (spilling) on join node is high compared to other nodes.


Reviewed By: xiaoxmeng

Differential Revision: D66261340

Pulled By: tanjialiang

tanjialiang force-pushed the pri_reclaim branch from 6dd01a9 to 078c12a Compare

December 3, 2024 23:41

Contributor

facebook-github-bot commented Dec 3, 2024

This pull request was exported from Phabricator. Differential Revision: D66261340

tanjialiang added a commit to tanjialiang/velox-1 that referenced this pull request


          feat: Add priority based memory reclaim framework (facebookincubator#…

2cd6fce

…11598)

Summary:
* Adds priority base reclaiming to memory reclaim framework. The priority determines which memory pool to reclaim first on the same level. This would help to make reclaim more application logic aware. 
* Make join node reclaim priority lower than others. This is because cost of reclaiming (spilling) on join node is high compared to other nodes.


Reviewed By: xiaoxmeng

Differential Revision: D66261340

Pulled By: tanjialiang

tanjialiang force-pushed the pri_reclaim branch from 078c12a to 2cd6fce Compare

December 4, 2024 01:20

Contributor

facebook-github-bot commented Dec 4, 2024

This pull request was exported from Phabricator. Differential Revision: D66261340

tanjialiang added a commit to tanjialiang/velox-1 that referenced this pull request


          feat: Add priority based memory reclaim framework (facebookincubator#…

49605f5

…11598)

Summary:
* Adds priority base reclaiming to memory reclaim framework. The priority determines which memory pool to reclaim first on the same level. This would help to make reclaim more application logic aware. 
* Make join node reclaim priority lower than others. This is because cost of reclaiming (spilling) on join node is high compared to other nodes.


Reviewed By: xiaoxmeng

Differential Revision: D66261340

Pulled By: tanjialiang

tanjialiang force-pushed the pri_reclaim branch from 2cd6fce to 49605f5 Compare

December 4, 2024 05:16

Contributor

facebook-github-bot commented Dec 4, 2024

This pull request was exported from Phabricator. Differential Revision: D66261340

tanjialiang added a commit to tanjialiang/velox-1 that referenced this pull request


          feat: Add priority based memory reclaim framework (facebookincubator#…

d9e761c

…11598)

Summary:
* Adds priority base reclaiming to memory reclaim framework. The priority determines which memory pool to reclaim first on the same level. This would help to make reclaim more application logic aware. 
* Make join node reclaim priority lower than others. This is because cost of reclaiming (spilling) on join node is high compared to other nodes.


Reviewed By: xiaoxmeng

Differential Revision: D66261340

Pulled By: tanjialiang

tanjialiang force-pushed the pri_reclaim branch from 49605f5 to d9e761c Compare

December 4, 2024 08:38

Contributor

facebook-github-bot commented Dec 4, 2024

This pull request was exported from Phabricator. Differential Revision: D66261340

tanjialiang added a commit to tanjialiang/velox-1 that referenced this pull request


          feat: Add priority based memory reclaim framework (facebookincubator#…

f9cf776

…11598)

Summary:
* Adds priority base reclaiming to memory reclaim framework. The priority determines which memory pool to reclaim first on the same level. This would help to make reclaim more application logic aware. 
* Make join node reclaim priority lower than others. This is because cost of reclaiming (spilling) on join node is high compared to other nodes.


Reviewed By: xiaoxmeng

Differential Revision: D66261340

Pulled By: tanjialiang

tanjialiang force-pushed the pri_reclaim branch from d9e761c to f9cf776 Compare

December 5, 2024 19:41

Contributor

facebook-github-bot commented Dec 5, 2024

This pull request was exported from Phabricator. Differential Revision: D66261340

tanjialiang force-pushed the pri_reclaim branch from f9cf776 to fb37a25 Compare

December 6, 2024 02:24

tanjialiang added a commit to tanjialiang/velox-1 that referenced this pull request


          feat: Add priority based memory reclaim framework (facebookincubator#…

fb37a25

…11598)

Summary:
* Adds priority base reclaiming to memory reclaim framework. The priority determines which memory pool to reclaim first on the same level. This would help to make reclaim more application logic aware. 
* Make join node reclaim priority lower than others. This is because cost of reclaiming (spilling) on join node is high compared to other nodes.


Reviewed By: xiaoxmeng

Differential Revision: D66261340

Pulled By: tanjialiang

Contributor

facebook-github-bot commented Dec 6, 2024

This pull request was exported from Phabricator. Differential Revision: D66261340


          feat: Add priority based memory reclaim framework (facebookincubator#…

f89c432

…11598)

Summary:
* Adds priority base reclaiming to memory reclaim framework. The priority determines which memory pool to reclaim first on the same level. This would help to make reclaim more application logic aware. 
* Make join node reclaim priority lower than others. This is because cost of reclaiming (spilling) on join node is high compared to other nodes.


Reviewed By: xiaoxmeng

Differential Revision: D66261340

Pulled By: tanjialiang

tanjialiang force-pushed the pri_reclaim branch from fb37a25 to f89c432 Compare

December 6, 2024 03:49

Contributor

facebook-github-bot commented Dec 6, 2024

This pull request was exported from Phabricator. Differential Revision: D66261340

tanjialiang mentioned this pull request

feat: Add priority based memory reclaim framework facebookincubator/nimble#111

Closed

tanjialiang added a commit to tanjialiang/nimble that referenced this pull request


          feat: Add priority based memory reclaim framework

5ec7d7f

Summary:
* Adds priority base reclaiming to memory reclaim framework. The priority determines which memory pool to reclaim first on the same level. This would help to make reclaim more application logic aware. 
* Make join node reclaim priority lower than others. This is because cost of reclaiming (spilling) on join node is high compared to other nodes.

X-link: facebookincubator/velox#11598

Reviewed By: xiaoxmeng

Differential Revision: D66261340

Pulled By: tanjialiang

facebook-github-bot pushed a commit to facebookincubator/nimble that referenced this pull request


          feat: Add priority based memory reclaim framework (#111)

ef2f2cf

Summary:
Pull Request resolved: #111

* Adds priority base reclaiming to memory reclaim framework. The priority determines which memory pool to reclaim first on the same level. This would help to make reclaim more application logic aware.
* Make join node reclaim priority lower than others. This is because cost of reclaiming (spilling) on join node is high compared to other nodes.

X-link: facebookincubator/velox#11598

Reviewed By: xiaoxmeng

Differential Revision: D66261340

Pulled By: tanjialiang

fbshipit-source-id: c03b2ef25b39dc8771f66321731d5a88da26638e

facebook-github-bot closed this in

939c102

facebook-github-bot added the Merged label

Contributor

facebook-github-bot commented Dec 6, 2024

@tanjialiang merged this pull request in 939c102.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed fb-exported Merged