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

[MINOR] [VL] Enhance the gluten timer to support seconds, milliseconds, and microseconds #8231

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

kecookier
Copy link
Contributor

@kecookier kecookier commented Dec 13, 2024

What changes were proposed in this pull request?

Sometimes we need a timer in seconds, e.g., for measuring file write time. Enhance the gluten timer to support seconds, milliseconds, microseconds and nanoseconds.

How was this patch tested?

Exist CI.

@github-actions github-actions bot added the VELOX label Dec 13, 2024
Copy link

Thanks for opening a pull request!

Could you open an issue for this pull request on Github Issues?

https://github.com/apache/incubator-gluten/issues

Then could you also rename commit message and pull request title in the following format?

[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}

See also:

@kecookier kecookier requested a review from marin-ma December 13, 2024 09:00
@kecookier
Copy link
Contributor Author

@marin-ma Can you help review this?

@@ -310,7 +310,7 @@ arrow::Result<std::vector<std::shared_ptr<arrow::Buffer>>> BlockPayload::deseria
RETURN_NOT_OK(inputStream->Read(sizeof(uint32_t), &numRows));
uint32_t numBuffers;
RETURN_NOT_OK(inputStream->Read(sizeof(uint32_t), &numBuffers));
timer.reset();
deserializeTime += timer.realTimeUsed();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this change? timer.start() and timer.stop() is not called. the realTimeUsed would be zero

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marin-ma Thanks, Um.. I've added this to fix the compile error with std::unique_ptr<ScopedTimer>. It was a typo. Now, I have fixed it in another way.

@kecookier
Copy link
Contributor Author

@marin-ma The comments have been fixed. Could you please review them again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants