Skip to content
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

Properly report lazy loaded inputBytes #11097

Closed
wants to merge 1 commit into from

Conversation

pedroerp
Copy link
Contributor

Summary:
Whenever there is lazy loading, TableScan would come out with zero
input and output bytes, and they would be attributed to the operator that in
fact loaded the lazy vector. Using the existing mechanism to save it locally
and periodically apply to the correct source operator.

Differential Revision: D63414708

@facebook-github-bot 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 Sep 26, 2024
Copy link

netlify bot commented Sep 26, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit faaccef
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/66f6cde3c431090008ecf726

@facebook-github-bot
Copy link
Contributor

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

@pedroerp pedroerp requested review from xiaoxmeng, mbasmanova and Yuhta and removed request for xiaoxmeng September 26, 2024 00:58
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pedroerp LGTM. Thanks for the improvement % nits.

auto task = assertQuery(op, {filePath}, "select c0 % 3 from tmp");

// Ensure lazy stats are attributed to table scan.
auto stats = task->taskStats();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: const auto stats

@@ -41,7 +45,9 @@ void VectorLoader::load(
vector_size_t resultSize,
VectorPtr* result) {
{
DeltaCpuWallTimer timer([&](auto& delta) { writeIOTiming(delta); });
DeltaCpuWallTimer lazyIoRecorder([&](auto& cpuDelta) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a new class for this as it now does delta cpu and io stats tracking. Maybe LazyIoStatsRecorder? We could just define in LazyVector.cpp? Thanks!

int64_t inputBytesDelta = 0;
it = lockStats->runtimeStats.find(LazyVector::kInputBytes);
if (it != lockStats->runtimeStats.end()) {
int64_t inputBytes = it->second.sum;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const int64_t inputBytes

@facebook-github-bot
Copy link
Contributor

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

pedroerp added a commit to pedroerp/velox-1 that referenced this pull request Sep 26, 2024
Summary:
Pull Request resolved: facebookincubator#11097

Whenever there is lazy loading, TableScan would come out with zero
input and output bytes, and they would be attributed to the operator that in
fact loaded the lazy vector. Using the existing mechanism to save it locally
and periodically apply to the correct source operator.

Reviewed By: xiaoxmeng

Differential Revision: D63414708
}

private:
// NOTE: Put `wallTimeStart_` before `cpuTimeStart_`, so that wall-time starts
// counting earlier than cpu-time.
const std::chrono::steady_clock::time_point wallTimeStart_;
const uint64_t cpuTimeStart_;
};

// Composes delta CpuWallTiming upon destruction and passes it to the user
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// for public comment. Thanks!

@@ -58,18 +58,13 @@ class CpuWallTimer {
};

// Keeps track of elapsed CPU and wall time from construction time.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@facebook-github-bot
Copy link
Contributor

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

pedroerp added a commit to pedroerp/velox-1 that referenced this pull request Sep 27, 2024
Summary:
Pull Request resolved: facebookincubator#11097

Whenever there is lazy loading, TableScan would come out with zero
input and output bytes, and they would be attributed to the operator that in
fact loaded the lazy vector. Using the existing mechanism to save it locally
and periodically apply to the correct source operator.

Reviewed By: xiaoxmeng

Differential Revision: D63414708
@facebook-github-bot
Copy link
Contributor

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

pedroerp added a commit to pedroerp/velox-1 that referenced this pull request Sep 27, 2024
Summary:
Pull Request resolved: facebookincubator#11097

Whenever there is lazy loading, TableScan would come out with zero
input and output bytes, and they would be attributed to the operator that in
fact loaded the lazy vector. Using the existing mechanism to save it locally
and periodically apply to the correct source operator.

Reviewed By: xiaoxmeng

Differential Revision: D63414708
Summary:
Pull Request resolved: facebookincubator#11097

Whenever there is lazy loading, TableScan would come out with zero
input and output bytes, and they would be attributed to the operator that in
fact loaded the lazy vector. Using the existing mechanism to save it locally
and periodically apply to the correct source operator.

Reviewed By: xiaoxmeng

Differential Revision: D63414708
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 42940c8.

Copy link

Conbench analyzed the 1 benchmark run on commit 42940c8b.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

athmaja-n pushed a commit to athmaja-n/velox that referenced this pull request Jan 10, 2025
Summary:
Pull Request resolved: facebookincubator#11097

Whenever there is lazy loading, TableScan would come out with zero
input and output bytes, and they would be attributed to the operator that in
fact loaded the lazy vector. Using the existing mechanism to save it locally
and periodically apply to the correct source operator.

Reviewed By: xiaoxmeng

Differential Revision: D63414708

fbshipit-source-id: d1bbc64fc7ec1ed952593aaeece0b02daed60038
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants