Correctly record the rawBytesRead_ metric in TableScan #8147

JkSelf · 2023-12-21T10:32:59Z

Based on @oerling design:
rawBytesRead is the quantized size of data read by a query. Read is the volume copied from storage. The second is 0 if data comes from caches. With DirectBufferedInput these are close but then read has unused coalesced columns counted where raw read does not. A column can be coalesced into IO but never hit by the query due to filtering.

Currently, DirectBufferInput only tracks rawBytesRead in the loadSync method. It does not track rawBytesRead in the asynchronous loadData method. This PR adds the rawBytesRead metric to the loadData method.

netlify · 2023-12-21T10:33:04Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`c4b4e78`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/65b1f9afe9924f0008aac693

FelixYBW · 2023-12-30T08:37:51Z

velox/dwio/common/DirectBufferedInput.cpp

@@ -274,6 +274,7 @@ std::vector<cache::CachePin> DirectCoalescedLoad::loadData(bool isPrefetch) {
  }
  input_->read(buffers, requests_[0].region.offset, LogType::FILE);
  ioStats_->read().increment(size);
+  ioStats_->incRawBytesRead(size);


Looks it's a duplicated metric. Can we use ioStats_->Read instead?

@FelixYBW Yes, the duplicated metric has been removed. Additionally, the getCompletedBytes method in HiveDataSource has been updated to retrieve the ioStats_->read() metric instead of the rawBytesRead_ metric. Here is a detailed explanation of the changes #8147 (comment)

@JkSelf do we want to increment incRawBytesRead here? From my discussion with @oerling, rawBytesRead counts the number of bytes read by query which shall only be incremented in loadSync? Thanks!

@xiaoxmeng Got it. Updated. Please help to review again. Thanks.

JkSelf · 2024-01-04T01:08:33Z

@mbasmanova Can you help to review? Thanks.

mbasmanova

@Yuhta Jimmy, would you help review this PR?

CC: @oerling @xiaoxmeng

FelixYBW · 2024-01-04T01:51:19Z

Looks we still have two metrics used, like here. Should we unified to one metric? Or the two metrics count different thing?

velox/velox/dwio/common/DirectInputStream.cpp

Lines 148 to 155 in f12b055

    
           ioStats_->incRawBytesRead(loadedRegion_.length); 
        
           auto ranges = makeRanges(loadedRegion_.length, data_, tinyData_); 
        
           uint64_t usecs = 0; 
        
           { 
        
             MicrosecondTimer timer(&usecs); 
        
             input_->read(ranges, loadedRegion_.offset, LogType::FILE); 
        
           } 
        
           ioStats_->read().increment(loadedRegion_.length);

velox/connectors/hive/HiveDataSource.h

JkSelf · 2024-01-05T01:13:11Z

Looks we still have two metrics used, like here. Should we unified to one metric? Or the two metrics count different thing?

velox/velox/dwio/common/DirectInputStream.cpp

Lines 148 to 155 in f12b055

ioStats_->incRawBytesRead(loadedRegion_.length);

auto ranges = makeRanges(loadedRegion_.length, data_, tinyData_);

uint64_t usecs = 0;

{

MicrosecondTimer timer(&usecs);

input_->read(ranges, loadedRegion_.offset, LogType::FILE);

}

ioStats_->read().increment(loadedRegion_.length);

@FelixYBW It seems the value ioStats_->incRawBytesRead(loadedRegion_.length) and ioStats_->read().increment(loadedRegion_.length) are same. But i don't know why adding two metrics. @xiaoxmeng Do you have any input? Thanks.

xiaoxmeng

@JkSelf LGTM. Thanks!

velox/exec/tests/TableScanTest.cpp

JkSelf · 2024-01-09T03:02:46Z

@oerling You added the read_ metric to the IoStats class https://github.com/facebookincubator/velox/blob/main/velox/common/io/IoStatistics.h#L152. I noticed that there is also an incRawBytesRead_ metric. It appears that both metrics count the same value when collecting the metric data

velox/velox/dwio/common/DirectInputStream.cpp

Lines 148 to 155 in f12b055

    
           ioStats_->incRawBytesRead(loadedRegion_.length); 
        
           auto ranges = makeRanges(loadedRegion_.length, data_, tinyData_); 
        
           uint64_t usecs = 0; 
        
           { 
        
             MicrosecondTimer timer(&usecs); 
        
             input_->read(ranges, loadedRegion_.offset, LogType::FILE); 
        
           } 
        
           ioStats_->read().increment(loadedRegion_.length);

. Could you provide some insight into the purpose or usage of these metrics? I would appreciate any input you have. Thank you

JkSelf · 2024-01-09T04:12:36Z

@xiaoxmeng @mbasmanova Can you help to merge if no further comment? Thanks for your help

facebook-github-bot · 2024-01-09T07:24:54Z

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

JkSelf · 2024-01-10T01:20:39Z

@xiaoxmeng @mbasmanova Can you help to merge? Thanks.

xiaoxmeng · 2024-01-10T01:33:44Z

@oerling You added the read_ metric to the IoStats class https://github.com/facebookincubator/velox/blob/main/velox/common/io/IoStatistics.h#L152. I noticed that there is also an incRawBytesRead_ metric. It appears that both metrics count the same value when collecting the metric data

velox/velox/dwio/common/DirectInputStream.cpp

Lines 148 to 155 in f12b055

ioStats_->incRawBytesRead(loadedRegion_.length);

auto ranges = makeRanges(loadedRegion_.length, data_, tinyData_);

uint64_t usecs = 0;

{

MicrosecondTimer timer(&usecs);

input_->read(ranges, loadedRegion_.offset, LogType::FILE);

}

ioStats_->read().increment(loadedRegion_.length);

. Could you provide some insight into the purpose or usage of these metrics? I would appreciate any input you have. Thank you

@JkSelf I synced with @oerling offline and we might need to first make a refactor on our side about these different counters as these are also used by Meta internal projects. The meaning of these read counters are a bit different with and without cache and we will update with better documentation on these to avoid the confusion. Thanks!

FelixYBW · 2024-01-10T01:41:06Z

@oerling You added the read_ metric to the IoStats class https://github.com/facebookincubator/velox/blob/main/velox/common/io/IoStatistics.h#L152. I noticed that there is also an incRawBytesRead_ metric. It appears that both metrics count the same value when collecting the metric data

velox/velox/dwio/common/DirectInputStream.cpp

Lines 148 to 155 in f12b055

ioStats_->incRawBytesRead(loadedRegion_.length);

auto ranges = makeRanges(loadedRegion_.length, data_, tinyData_);

uint64_t usecs = 0;

{

MicrosecondTimer timer(&usecs);

input_->read(ranges, loadedRegion_.offset, LogType::FILE);

}

ioStats_->read().increment(loadedRegion_.length);

. Could you provide some insight into the purpose or usage of these metrics? I would appreciate any input you have. Thank you

@JkSelf I synced with @oerling offline and we might need to first make a refactor on our side about these different counters as these are also used by Meta internal projects. The meaning of these read counters are a bit different with and without cache and we will update with better documentation on these to avoid the confusion. Thanks!

From @oerling :
"rawBytesRead is the quantized size of data read by a query. Read is the volume copied from storage. The second is 0 if data comes from caches. With DirectBufferedInput these are close but then read has unused coalesced columns counted where raw read does not. A column can be coalesced into IO but never hit by the query due to filtering."

JkSelf · 2024-01-10T03:03:08Z

@xiaoxmeng @FelixYBW Thank you for providing the crucial information. As said by @oerling, i think it is more accurate to use rawBytesRead instead of read() to represent the actual number of bytes read. What are your thoughts on this? I have made the change to rawBytesRead in the latest commit. Can you help to review? Thank you.

JkSelf · 2024-01-11T01:30:10Z

@xiaoxmeng Do you have any further comment? Thanks.

JkSelf · 2024-01-12T23:24:47Z

@xiaoxmeng Can you help to review again? Thanks.

velox/connectors/hive/HiveDataSource.h

Yuhta · 2024-01-17T17:33:59Z

velox/dwio/common/DirectBufferedInput.cpp

@@ -274,6 +274,7 @@ std::vector<cache::CachePin> DirectCoalescedLoad::loadData(bool isPrefetch) {
  }
  input_->read(buffers, requests_[0].region.offset, LogType::FILE);
  ioStats_->read().increment(size);
+  ioStats_->incRawBytesRead(size - overread);


@JkSelf Can you also fix this in DwioCoalescedLoadBase::updateStats? Thanks

@Yuhta The rawBytesRead already updated in loadSync and loadData. It seems no need updated in DwioCoalescedLoadBase::updateStats.

Where is it updated in DwioCoalescedLoad::loadData and SsdLoad::loadData? I cannot find it

I have reviewed the code and found that the update of rawBytesRead for CachedInputBuffer is done in the loadSync function. loadSync is executed unconditionally after loadOrFuture here. However, in the case of DirectInputBuffer, the execution of loadSync is conditional here, so we need to update rawBytesRead in the loadData function instead.

@Yuhta Do you have any further comment? Thanks.

Thanks for the explanation. Can you add comment to those 2 places saying the counter is increased in loadSync unconditionally so we don't need to do it in loadData?

@Yuhta Added the comments. Please help to review again. Thanks.

facebook-github-bot · 2024-01-25T05:56:08Z

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

facebook-github-bot · 2024-01-25T06:04:24Z

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

facebook-github-bot · 2024-01-25T09:00:21Z

@xiaoxmeng merged this pull request in 6d2f0d4.

conbench-facebook · 2024-01-25T09:22:54Z

Conbench analyzed the 1 benchmark run on commit 6d2f0d40.

There weren't enough matching historic benchmark results to make a call on whether there were regressions.

The full Conbench report has more details.

facebook-github-bot · 2024-01-25T19:49:06Z

This pull request has been reverted by 0f96c1c.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 21, 2023

FelixYBW reviewed Dec 30, 2023

View reviewed changes

JkSelf force-pushed the scan-metric branch from 38b2558 to 1a65207 Compare January 4, 2024 00:09

JkSelf changed the title ~~Add the raw bytes read metric in scan~~ Correctly record the rawInputBytes metric in TableScan Jan 4, 2024

JkSelf changed the title ~~Correctly record the rawInputBytes metric in TableScan~~ Correctly record the rawBytesRead_ metric in TableScan Jan 4, 2024

mbasmanova requested review from Yuhta and xiaoxmeng January 4, 2024 01:09

mbasmanova reviewed Jan 4, 2024

View reviewed changes

xiaoxmeng reviewed Jan 4, 2024

View reviewed changes

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

xiaoxmeng approved these changes Jan 8, 2024

View reviewed changes

velox/exec/tests/TableScanTest.cpp Outdated Show resolved Hide resolved

JkSelf force-pushed the scan-metric branch from 1a65207 to 167ec1a Compare January 9, 2024 02:38

JkSelf force-pushed the scan-metric branch from 167ec1a to c3b8668 Compare January 10, 2024 02:50

JkSelf force-pushed the scan-metric branch from c3b8668 to 25be5d6 Compare January 10, 2024 06:42

Yuhta reviewed Jan 15, 2024

View reviewed changes

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

JkSelf force-pushed the scan-metric branch from 25be5d6 to f08ab75 Compare January 17, 2024 08:45

Yuhta reviewed Jan 17, 2024

View reviewed changes

JkSelf force-pushed the scan-metric branch from 3a51719 to f08ab75 Compare January 18, 2024 02:26

Yuhta approved these changes Jan 24, 2024

View reviewed changes

JkSelf force-pushed the scan-metric branch from f08ab75 to c4b4e78 Compare January 25, 2024 06:03

facebook-github-bot closed this in 6d2f0d4 Jan 25, 2024

facebook-github-bot added the Merged label Jan 25, 2024

JkSelf added 2 commits January 25, 2024 21:14

Correctly record the rawInputBytes metric in TableScan

50c5b7a

Resolve comments

c4b4e78

facebook-github-bot added the Reverted label Jan 25, 2024

JkSelf mentioned this pull request Jan 26, 2024

Correctly record the rawInputBytes metric in TableScan #8545

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly record the rawBytesRead_ metric in TableScan #8147

Correctly record the rawBytesRead_ metric in TableScan #8147

JkSelf commented Dec 21, 2023 •

edited

Loading

netlify bot commented Dec 21, 2023 •

edited

Loading

FelixYBW Dec 30, 2023

JkSelf Jan 4, 2024

xiaoxmeng Jan 10, 2024

JkSelf Jan 10, 2024

JkSelf commented Jan 4, 2024

mbasmanova left a comment

FelixYBW commented Jan 4, 2024

JkSelf commented Jan 5, 2024

xiaoxmeng left a comment

JkSelf commented Jan 9, 2024

JkSelf commented Jan 9, 2024

facebook-github-bot commented Jan 9, 2024

JkSelf commented Jan 10, 2024

xiaoxmeng commented Jan 10, 2024

FelixYBW commented Jan 10, 2024

JkSelf commented Jan 10, 2024

JkSelf commented Jan 11, 2024

JkSelf commented Jan 12, 2024

Yuhta Jan 17, 2024

JkSelf Jan 18, 2024

Yuhta Jan 18, 2024

JkSelf Jan 19, 2024

JkSelf Jan 24, 2024

Yuhta Jan 24, 2024

JkSelf Jan 25, 2024

facebook-github-bot commented Jan 25, 2024

facebook-github-bot commented Jan 25, 2024

facebook-github-bot commented Jan 25, 2024

conbench-facebook bot commented Jan 25, 2024

facebook-github-bot commented Jan 25, 2024

Correctly record the rawBytesRead_ metric in TableScan #8147

Correctly record the rawBytesRead_ metric in TableScan #8147

Conversation

JkSelf commented Dec 21, 2023 • edited Loading

netlify bot commented Dec 21, 2023 • edited Loading

✅ Deploy Preview for meta-velox canceled.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JkSelf commented Jan 4, 2024

mbasmanova left a comment

Choose a reason for hiding this comment

FelixYBW commented Jan 4, 2024

JkSelf commented Jan 5, 2024

xiaoxmeng left a comment

Choose a reason for hiding this comment

JkSelf commented Jan 9, 2024

JkSelf commented Jan 9, 2024

facebook-github-bot commented Jan 9, 2024

JkSelf commented Jan 10, 2024

xiaoxmeng commented Jan 10, 2024

FelixYBW commented Jan 10, 2024

JkSelf commented Jan 10, 2024

JkSelf commented Jan 11, 2024

JkSelf commented Jan 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jan 25, 2024

facebook-github-bot commented Jan 25, 2024

facebook-github-bot commented Jan 25, 2024

conbench-facebook bot commented Jan 25, 2024

facebook-github-bot commented Jan 25, 2024

JkSelf commented Dec 21, 2023 •

edited

Loading

netlify bot commented Dec 21, 2023 •

edited

Loading