-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
Add memory and cache stats to PeriodicStatsReporter #9723
Add memory and cache stats to PeriodicStatsReporter #9723
Conversation
✅ Deploy Preview for meta-velox canceled.
|
1ec0204
to
37549d5
Compare
@@ -39,13 +41,22 @@ class PeriodicStatsReporter { | |||
struct Options { | |||
Options() {} | |||
|
|||
const memory::MemoryArbitrator* arbitrator{nullptr}; | |||
const velox::memory::MemoryAllocator* const allocator{nullptr}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const velox::memory::MemoryAllocator* allocator{nullptr};
const velox::cache::AsyncDataCache* cache{nullptr};
if (auto* mmapAllocator = | ||
dynamic_cast<const velox::memory::MmapAllocator*>(allocator_)) { | ||
RECORD_METRIC_VALUE( | ||
kMetricMmapRawAllocBytesSmall, (mmapAllocator->numMallocBytes())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/kMetricMmapRawAllocBytesSmall/kMetricMmapDelegatedAllocBytes/?
const auto cacheStats = cache_->refreshStats(); | ||
|
||
// Snapshots. | ||
RECORD_METRIC_VALUE(kMetricMemoryCacheNumEntries, cacheStats.numEntries); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a velox change and you update SSD cache production with these non-average stats? Or you first report average stats and does the non-average stats in followup? Thanks!
2d391c5
to
0fb06c7
Compare
@tanjialiang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
a340965
to
cf8ebda
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tanjialiang LGTM % minors. Thanks!
46da2b2
to
05e9cd4
Compare
@tanjialiang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tanjialiang LGTM. Thanks!
@tanjialiang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks.
velox/common/base/Counters.cpp
Outdated
// Total number of bytes written to SSD. | ||
DEFINE_METRIC(kMetricSsdCacheWrittenBytes, facebook::velox::StatType::SUM); | ||
|
||
// Cumulative number of SsdCache entries that are aged out and evicted given |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cumulative -> Total to be consistent with other comments?
velox/common/base/Counters.cpp
Outdated
// configured TTL. | ||
DEFINE_METRIC(kMetricSsdCacheAgedOutEntries, facebook::velox::StatType::SUM); | ||
|
||
// Cumulative number of SsdCache regions that are aged out and evicted given |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
…r#9723) Summary: Added memory allocator and cache stats to PeriodicStatsReporter. Added documentations to monitoring doc. Reviewed By: xiaoxmeng, zacw7 Differential Revision: D57085696 Pulled By: tanjialiang
05e9cd4
to
06c7837
Compare
This pull request was exported from Phabricator. Differential Revision: D57085696 |
…r#9723) Summary: Added memory allocator and cache stats to PeriodicStatsReporter. Added documentations to monitoring doc. Reviewed By: xiaoxmeng, zacw7 Differential Revision: D57085696 Pulled By: tanjialiang
06c7837
to
72a371a
Compare
This pull request was exported from Phabricator. Differential Revision: D57085696 |
@tanjialiang merged this pull request in b8a26ce. |
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
…r#9723) Summary: Added memory allocator and cache stats to PeriodicStatsReporter. Added documentations to monitoring doc. Pull Request resolved: facebookincubator#9723 Reviewed By: xiaoxmeng, zacw7 Differential Revision: D57085696 Pulled By: tanjialiang fbshipit-source-id: 7dbc3791d314212e0c7658d0d868dac074f34fcf
…r#9723) Summary: Added memory allocator and cache stats to PeriodicStatsReporter. Added documentations to monitoring doc. Pull Request resolved: facebookincubator#9723 Reviewed By: xiaoxmeng, zacw7 Differential Revision: D57085696 Pulled By: tanjialiang fbshipit-source-id: 7dbc3791d314212e0c7658d0d868dac074f34fcf
Added memory allocator and cache stats to PeriodicStatsReporter.
Added documentations to monitoring doc.