misc: Use ioCounter for pageLoadTimeNs#15407
misc: Use ioCounter for pageLoadTimeNs#15407rui-mo wants to merge 1 commit intofacebookincubator:mainfrom
Conversation
✅ Deploy Preview for meta-velox canceled.
|
meta-cla
bot
added
the
CLA Signed
rui-mo
changed the title
ioCounter for pageLoadTimeNs
| virtual std::unique_ptr<RowReader> createRowReader( | ||
| const RowReaderOptions& options = {}) const = 0; | ||
| const RowReaderOptions& options = {}, | ||
| std::shared_ptr<io::IoStatistics> ioStats = nullptr) const = 0; |
There was a problem hiding this comment.
Ideally format reader should not be aware of such storage level stats. And other formats than Parquet does not need this. Is there a way to extract the IO stats from the BufferedInput in parquet reader, instead of drilling a hole through format layer?
There was a problem hiding this comment.
Thanks for reviewing! We discovered that simply changing the variable type to ioCounter is enough to gather these metrics.
rui-mo
changed the title
| @@ -73,6 +73,13 @@ class IoCounter { | |||
| casLoop(max_, other.max_, std::less()); | |||
| } | |||
|
|
|||
| void clear() { | |||
There was a problem hiding this comment.
Just do ioCounter = {} should be enough, there is no need to add an extra method for this
There was a problem hiding this comment.
Some tests and benchmarks need to reinitialize a RuntimeStatistics object. But after adding IoCounter to it, the assignment operator is implicitly deleted due to the use of atomic variables within IoCounter. To address this issue, clear() methods have been introduced as an alternative solution.
There was a problem hiding this comment.
Can you add a copy assignment for IoCounter then? Then there is no need to duplicate the initialization logic in all containing objects.
| stats.columnReaderStatistics.pageLoadTimeNs += | ||
| columnReaderStats_.pageLoadTimeNs; | ||
| stats.columnReaderStatistics.pageLoadTimeNs.increment( | ||
| columnReaderStats_.pageLoadTimeNs.sum()); |
There was a problem hiding this comment.
updateRuntimeStats is called by HiveDataSource::next and eventually at
velox/velox/exec/TableScan.cpp
Line 204 in 04faded
If using merge here, it's more like a batch-level updating for the min/max stats. Typically the other runtime stats are merged at task level at
velox/velox/exec/TableScan.cpp
Line 315 in 04faded
How do you think? Thanks.
There was a problem hiding this comment.
I see. So it's counted in columnreader level or page level. We care about single I/O on page level. So we should use merge.
| result.emplace( | ||
| "pageLoadTimeNs", | ||
| RuntimeMetric(columnReaderStatistics.pageLoadTimeNs)); | ||
| RuntimeMetric(columnReaderStatistics.pageLoadTimeNs.sum())); |
There was a problem hiding this comment.
Let's wait until #15408 merged, then update count,min,max
rui-mo
force-pushed
the
wip_page_load
branch
from
5d3967b to
76a9ef3
Compare
Yuhta
added
the
ready-to-merge
4 participants
This PR uses
ioCounterforpageLoadTimeNsto collect its min, max, count stats.