[native]Update Prestissimo to report the actual used task memory #22833
Conversation
| @@ -654,8 +654,8 @@ TEST_P(PrestoExchangeSourceTest, slowProducer) { | |||
| auto queue = makeSingleSourceQueue(); | |||
| auto exchangeSource = makeExchangeSource(producerAddress, useHttps, 3, queue); | |||
|
|
|||
| size_t beforePoolSize = pool_->currentBytes(); | |||
| size_t beforeQueueSize = queue->totalBytes(); | |||
| const size_t beforePoolSize = pool_->usedBytes(); | |||
| size_t beforePoolSize = pool_->currentBytes(); | ||
| size_t beforeQueueSize = queue->totalBytes(); | ||
| const size_t beforePoolSize = pool_->usedBytes(); | ||
| const size_t beforeQueueSize = queue->totalBytes(); |
| size_t deltaPool = pool_->currentBytes() - beforePoolSize; | ||
| size_t deltaQueue = queue->totalBytes() - beforeQueueSize; | ||
| EXPECT_EQ(deltaPool, deltaQueue); | ||
| const size_t deltaPoolBytes = pool_->usedBytes() - beforePoolSize; |
xiaoxmeng
force-pushed
the
advance
branch
5 times, most recently
from
7116c80
to
36ceb0c
Compare
There was a problem hiding this comment.
Looks good from my end. @tanjialiang pleast take another look if auto is a blocker. Otherwise lets accept and land it.
There was a problem hiding this comment.
@xiaoxmeng
Thank you for the change!
Let's update the PR description to explain what we are trying to achieve.
Also, left few questions, because it is not clear why we making these changes.
| const auto currentBytes = task->pool()->usedBytes(); | ||
| prestoTaskStats.userMemoryReservationInBytes = currentBytes; |
There was a problem hiding this comment.
Could we get 'currentBytes' from 'veloxTaskMemStats'?
they are both received from the pool.
There was a problem hiding this comment.
stats() still use currentBytes(), and I will switch to use usedBytes in stats() after deprecate currentBytes.
| @@ -1199,7 +1199,7 @@ protocol::NodeStatus PrestoServer::fetchNodeStatus() { | |||
| (int)std::thread::hardware_concurrency(), | |||
| cpuLoadPct, | |||
| cpuLoadPct, | |||
| pool_ ? pool_->currentBytes() : 0, | |||
| pool_ ? pool_->usedBytes() : 0, | |||
There was a problem hiding this comment.
What's the diff between currentBytes and usedBytes?
There was a problem hiding this comment.
currentBytes is deprecated. I will remove from velox later.
Changes to report the cumulative bytes using usedBytes from non-leaf pool instead of reserved bytes so as to track the actual memory usage better.
Changes to report the cumulative bytes using usedBytes from non-leaf pool instead of
reserved bytes so as to track the actual memory usage better.