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

Expose planningCpu time in QueryStats #3269

Closed
wants to merge 1 commit into from
Closed

Expose planningCpu time in QueryStats #3269

wants to merge 1 commit into from

Conversation

puchengy
Copy link
Contributor

This is for #2579

I have a few questions, hope can have some ideas on what should be improved in this PR:

  1. Is this the right way to measure CPU? I borrowed it from other places in the repo. And the underlying assumption is the planning is performed by a single thread.

  2. Should we update this QueryCompleteEvent API to incorporate this as well? However planning time is not in the API as well.

  3. Do you think we should have a standalone unit test for this change (my answer is that it is not needed)?

@cla-bot cla-bot bot added the cla-signed label Mar 29, 2020
@puchengy puchengy requested a review from sopel39 March 30, 2020 03:25
@puchengy puchengy requested a review from sopel39 April 2, 2020 06:37
@puchengy puchengy requested a review from sopel39 April 3, 2020 03:22
@sopel39 sopel39 changed the title (WIP) Expose planningCpu time in QueryStats Expose planningCpu time in QueryStats Apr 3, 2020
Copy link
Member

@sopel39 sopel39 left a comment

Choose a reason for hiding this comment

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

Code looks fine. Please fix tests

@puchengy puchengy requested a review from sopel39 April 18, 2020 23:47
@puchengy
Copy link
Contributor Author

puchengy commented Apr 19, 2020

Ops, need to fix the test

@@ -36,6 +42,9 @@
private final AtomicReference<Long> beginResourceWaitingNanos = new AtomicReference<>();
private final AtomicReference<Long> beginDispatchingNanos = new AtomicReference<>();
private final AtomicReference<Long> beginPlanningNanos = new AtomicReference<>();
private final AtomicReference<Long> beginPlanningCpuNanos = new AtomicReference<>();
private final AtomicReference<Long> beginPlanningThreadId = new AtomicReference<>();
private final AtomicReference<Long> beginStartingThreadId = new AtomicReference<>();
Copy link
Member

Choose a reason for hiding this comment

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

you don't need to store starting thread id. Just beginPlanningThreadId is enough

planningTime.compareAndSet(null, nanosSince(beginPlanningNanos, now));
planningCpuTime.compareAndSet(null, nanosSince(beginPlanningCpuNanos, cpuNow));
beginStartingThreadId.compareAndSet(null, threadId);
if (!beginPlanningThreadId.get().equals(beginStartingThreadId.get())) {
Copy link
Member

Choose a reason for hiding this comment

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

Just use checkState as this should never happen

@sopel39
Copy link
Member

sopel39 commented Oct 31, 2020

@Terry93 could you rebase?

@sopel39
Copy link
Member

sopel39 commented Apr 14, 2021

@Terry93 will you have time to complete this PR?

@puchengy
Copy link
Contributor Author

@sopel39 Sorry about that, I completely forgot it. I will finish this one, thanks!

@colebow
Copy link
Member

colebow commented Oct 19, 2022

👋 @puchengy - this PR is inactive and doesn't seem to be under development. If you'd like to continue work on this at any point in the future, feel free to re-open.

@colebow colebow closed this Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants