You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@quickstep.apache.org by hbdeshmukh <gi...@git.apache.org> on 2016/06/14 19:38:08 UTC
[GitHub] incubator-quickstep pull request #32: QUICKSTEP-21 Measure execution time of...
GitHub user hbdeshmukh opened a pull request:
https://github.com/apache/incubator-quickstep/pull/32
QUICKSTEP-21 Measure execution time of normal WorkOrders.
This PR includes the following changes:
- Measure normal work order execution time
- Split the WorkOrderCompletion proto message in two: Normal and Rebuild work orders.
- Add execution time field in the NormalWorkOrderComplete proto message.
- Include the recorded time in NormalWorkOrderComplete proto message that is sent back to Foreman.
Note that I didn't add the execution time field in RebuildWorkOrder completion message because rebuilding storage block is pretty cheap for the storage layouts used by default. Therefore the time taken by rebuild work order is quite low. If in the future, we need this time, we can easily add it.
Also note that, the CPU overhead for measuring this time is pretty low. In terms of memory, we pay a cost of an additional 8 bytes in each completion message (only for normal work orders) sent from each worker to the Foreman.
This feature can be quite helpful for performance monitoring. We can take a look at individual work order execution time of different operators. I am planning to create another PR which can produce reports consisting of average/max/min/variance of execution times grouped by different operators.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/incubator-quickstep record-wo-execution-time
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-quickstep/pull/32.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #32
----
commit ba57e68333527e2a9c4376ab784beb42630887e2
Author: Harshad Deshmukh <hb...@apache.org>
Date: 2016-06-14T19:02:45Z
Measure execution time of normal WorkOrders.
- Measure normal work order execution time
- Split the WorkOrderCompletion proto message in two: Normal and Rebuild
work orders.
- Add execution time field in the NormalWorkOrderComplete proto message.
- Include the recorded time in NormalWorkOrderComplete proto message
that is sent back to Foreman.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #32: QUICKSTEP-21 Measure execution time of normal...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/32
Even for the info you mentioned, `RebuildWorkOrder` could also have these stored in `proto`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #32: QUICKSTEP-21 Measure execution time of normal...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on the issue:
https://github.com/apache/incubator-quickstep/pull/32
Hi @pateljm @zuyu
I can get the execution time for RebuildWorkOrder in the same PR. Thanks for your comments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #32: QUICKSTEP-21 Measure execution time of WorkOr...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on the issue:
https://github.com/apache/incubator-quickstep/pull/32
Hi @pateljm @zuyu
I have added the execution time field in rebuild work order completion proto message. There are detailed comments about the rationale for splitting the original class in two types. I have also added a TODO regarding @zuyu's suggestion on message classes extension, which we can address in a future PR. That PR should leave the Foreman and PolicyEnforcer classes untouched.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #32: QUICKSTEP-21 Measure execution time of normal...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on the issue:
https://github.com/apache/incubator-quickstep/pull/32
Hi @zuyu
You are right that at this point we can keep the two proto message classes combined. However in the future, in order to add more information to the normal WorkOrder completion message (e.g. resources consumed during the WorkOrder execution, number of tuples touched etc.) I think it makes more sense to split the two classes early enough.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #32: QUICKSTEP-21 Measure execution time of normal...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/32
@hbdeshmukh Since the time field in `WorkOrderCompletion` proto is optional, we still do not need to split into two.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #32: QUICKSTEP-21 Measure execution time of normal...
Posted by pateljm <gi...@git.apache.org>.
Github user pateljm commented on the issue:
https://github.com/apache/incubator-quickstep/pull/32
@hbdeshmukh Can you rebase this again? I can then merge. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #32: QUICKSTEP-21 Measure execution time of...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/32#discussion_r67089282
--- Diff: query_execution/QueryExecutionMessages.proto ---
@@ -20,11 +20,16 @@ package quickstep.serialization;
message EmptyMessage {
}
-// Used for both Normal WorkOrders and RebuildWorkOrders.
-// NOTE(zuyu): we might need to seperate the completion messages to contain
-// run-time information for Foreman to make better decisions on scheduling
-// WorkOrders.
-message WorkOrderCompletionMessage {
+// A message sent upon completion of a normal (not rebuild) WorkOrder execution.
+message NormalWorkOrderCompletionMessage {
+ required uint64 operator_index = 1;
+ required uint64 worker_thread_index = 2;
+ required uint64 query_id = 3;
+ optional uint64 execution_time_us = 4; // Execution time in micro seconds.
--- End diff --
Changed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #32: QUICKSTEP-21 Measure execution time of...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/32#discussion_r67067803
--- Diff: query_execution/Worker.cpp ---
@@ -47,13 +49,32 @@ void Worker::run() {
}
ClientIDMap *thread_id_map = ClientIDMap::Instance();
thread_id_map->addValue(worker_client_id_);
+ std::chrono::time_point<std::chrono::steady_clock> start, end;
for (;;) {
// Receive() is a blocking call, causing this thread to sleep until next
// message is received.
const AnnotatedMessage annotated_msg = bus_->Receive(worker_client_id_, 0, true);
const TaggedMessage &tagged_message = annotated_msg.tagged_message;
switch (tagged_message.message_type()) {
- case kWorkOrderMessage: // Fall through.
+ case kWorkOrderMessage: {
+ WorkerMessage message(*static_cast<const WorkerMessage*>(tagged_message.message()));
+ DCHECK(message.getWorkOrder() != nullptr);
+ // Start measuring the execution time.
+ start = std::chrono::steady_clock::now();
+ message.getWorkOrder()->execute();
+ end = std::chrono::steady_clock::now();
+ const std::size_t query_id_for_workorder =
+ message.getWorkOrder()->getQueryID();
+ delete message.getWorkOrder();
+ const uint64_t time_us =
--- End diff --
Either use `std::uint64_t`, or add `using std::uint64_t;` before the namespace.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #32: QUICKSTEP-21 Measure execution time of WorkOr...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/32
@pateljm Please also delete the branch after merging, as a part of the workflow in the wiki. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #32: QUICKSTEP-21 Measure execution time of normal...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/32
@pateljm Separating into two proto messages is overkill to me. In fact, the most part in this PR is to duplicate the logics to process two identical proto messages, except for `normal` and `rebuild`. Note that doing such measurement only needs about 10 lines of code changes.
On the other hand, we do differentiate two messages using two distinct `TMB` message types.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #32: QUICKSTEP-21 Measure execution time of...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/32#discussion_r67067611
--- Diff: query_execution/QueryExecutionMessages.proto ---
@@ -20,11 +20,16 @@ package quickstep.serialization;
message EmptyMessage {
}
-// Used for both Normal WorkOrders and RebuildWorkOrders.
-// NOTE(zuyu): we might need to seperate the completion messages to contain
-// run-time information for Foreman to make better decisions on scheduling
-// WorkOrders.
-message WorkOrderCompletionMessage {
+// A message sent upon completion of a normal (not rebuild) WorkOrder execution.
+message NormalWorkOrderCompletionMessage {
+ required uint64 operator_index = 1;
+ required uint64 worker_thread_index = 2;
+ required uint64 query_id = 3;
+ optional uint64 execution_time_us = 4; // Execution time in micro seconds.
--- End diff --
Prefer to `execution_time_in_microseconds`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #32: QUICKSTEP-21 Measure execution time of normal...
Posted by pateljm <gi...@git.apache.org>.
Github user pateljm commented on the issue:
https://github.com/apache/incubator-quickstep/pull/32
Good discussion guys. If I could weigh in, I think keeping the two proto classes makes sense. I'd like to close this later on today if the discussion here has converged. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #32: QUICKSTEP-21 Measure execution time of normal...
Posted by pateljm <gi...@git.apache.org>.
Github user pateljm commented on the issue:
https://github.com/apache/incubator-quickstep/pull/32
@zuyu @hbdeshmukh Ok -- looking at the code, I do think that having two protos make sense. Down the line `RebuildWorkOrderCompletionMessage` may need to report back compression ratio and/or block under-fill stats (for compressed column stores). These won't make sense to include in the proto definition for `NormalWorkOrderCompletionMessage`. So, let's keep these two protos separate. We could do a union, but that is ugly and confusing code.
@zuyu has a good point that (if I read part of his concern correctly) that we do need to record and report back `execution_time_in_microseconds` even for `RebuildWorkOrderCompletionMessage`. So let's do that.
Also let's make a note in the comments around the proto definition about why we have two protos (i.e. note that in the future `RebuildWorkOrderCompletionMessage` will report back additional stats), so that the reader is not confused.
I recommend @hbdeshmukh decide if he wants to fix this issue in this PR, or open a separate PR relatively soon. If the latter is chosen, please make a note here in the discussion thread for this PR.
In general, I'd recommend in situations like this to have a healthy discussion. Then, if we are in a gray area, let us document the concern, and give discretion to the person opening the PR.
I want to thank @zuyu for the diligent review -- without such careful review, the code base could quickly become unmanageable. So keep it coming, and don't hold back in the future.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #32: QUICKSTEP-21 Measure execution time of...
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/32#discussion_r67089293
--- Diff: query_execution/Worker.cpp ---
@@ -47,13 +49,32 @@ void Worker::run() {
}
ClientIDMap *thread_id_map = ClientIDMap::Instance();
thread_id_map->addValue(worker_client_id_);
+ std::chrono::time_point<std::chrono::steady_clock> start, end;
for (;;) {
// Receive() is a blocking call, causing this thread to sleep until next
// message is received.
const AnnotatedMessage annotated_msg = bus_->Receive(worker_client_id_, 0, true);
const TaggedMessage &tagged_message = annotated_msg.tagged_message;
switch (tagged_message.message_type()) {
- case kWorkOrderMessage: // Fall through.
+ case kWorkOrderMessage: {
+ WorkerMessage message(*static_cast<const WorkerMessage*>(tagged_message.message()));
+ DCHECK(message.getWorkOrder() != nullptr);
+ // Start measuring the execution time.
+ start = std::chrono::steady_clock::now();
+ message.getWorkOrder()->execute();
+ end = std::chrono::steady_clock::now();
+ const std::size_t query_id_for_workorder =
+ message.getWorkOrder()->getQueryID();
+ delete message.getWorkOrder();
+ const uint64_t time_us =
--- End diff --
Changed. Thanks for pointing out.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #32: QUICKSTEP-21 Measure execution time of normal...
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/32
@pateljm I agree that if we would go with the split option, we do w/ extensions as following.
```
message WorkOrderCompletionMessage {
...
}
message NormalWorkOrderCompletionMessage {
extend WorkOrderCompletionMessage {
...
}
}
message RebuildWorkOrderCompletionMessage {
extend WorkOrderCompletionMessage {
...
}
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep pull request #32: QUICKSTEP-21 Measure execution time of...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-quickstep/pull/32
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-quickstep issue #32: QUICKSTEP-21 Measure execution time of WorkOr...
Posted by pateljm <gi...@git.apache.org>.
Github user pateljm commented on the issue:
https://github.com/apache/incubator-quickstep/pull/32
Thanks @hbdeshmukh. Merging now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---