You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@quickstep.apache.org by navsan <gi...@git.apache.org> on 2016/06/12 21:09:19 UTC
[GitHub] incubator-quickstep pull request #28: Add option to enable Google Profiling.
GitHub user navsan opened a pull request:
https://github.com/apache/incubator-quickstep/pull/28
Add option to enable Google Profiling.
[Thanks @hbdeshmukh for the pointers on how to do this.]
This PR makes profiling pretty easy. Steps to take:
* Install dependencies: Google Perftools must be installed already. This PR doesn't use the third_party perftools.
* Build Quickstep with the ENABLE_GOOGLE_PROFILER flag turned on. You probably want to use Build Type RelWithDebInfo.
* When running the CLI shell, pass the profiler_file_name flag to start profiling starting from the second run of the query, and dump profile output to the file name specified.
* Alternatively, set the environment variable CPUPROFILE to turn on profiling for the entire run (including first run, preloading, foreman and worker setup, etc.).
* Use `google-pprof --dot profile.prof > profile.dot` or something similar to get a usable output.
Tested with flag turned on and off, with RelWithDebInfo, with Clang on CloudLab machine. Dependencies were pre-installed using `sudo apt-get install -y libgoogle-perftools-dev google-perftools`.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/incubator-quickstep google_profiler
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-quickstep/pull/28.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 #28
----
commit 7acd6434ddf335dd8c1a4ded562f5c6643fb1382
Author: Navneet Potti <na...@apache.org>
Date: 2016-06-12T20:57:39Z
Add option to enable Google Profiling.
----
---
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 #28: Add option to enable Google Profiler.
Posted by navsan <gi...@git.apache.org>.
Github user navsan commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/28#discussion_r66828325
--- Diff: cli/QuickstepCli.cpp ---
@@ -150,6 +154,14 @@ DEFINE_bool(initialize_db, false, "If true, initialize a database.");
DEFINE_bool(print_query, false,
"Print each input query statement. This is useful when running a "
"large number of queries in a batch.");
+DEFINE_string(profile_file_name, "",
--- End diff --
Actually, I\u2019d argue that this is a *feature*. We can build with profiling included, but enable it only when we want to (rather than doing so for every run).
> On Jun 13, 2016, at 11:38, Harshad Deshmukh <no...@github.com> wrote:
>
> In cli/QuickstepCli.cpp <https://github.com/apache/incubator-quickstep/pull/28#discussion_r66825014>:
>
> > @@ -150,6 +154,14 @@ DEFINE_bool(initialize_db, false, "If true, initialize a database.");
> > DEFINE_bool(print_query, false,
> > "Print each input query statement. This is useful when running a "
> > "large number of queries in a batch.");
> > +DEFINE_string(profile_file_name, "",
> One thing which we can do is generate a default profile name if none is specified. The default profile name can be something like "quickstep-profile-.prof", so that name collisions are avoided. That way, we can get rid of the check !quickstep::FLAGS_profile_file_name.empty() in Line 450. It's ok if we don't do it right away, in which case you can leave a TODO note.
>
> \u2014
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub <https://github.com/apache/incubator-quickstep/pull/28/files/00955f19652fe54468377e932365d77a13cc69c8#r66825014>, or mute the thread <https://github.com/notifications/unsubscribe/ACZB6-MkaeSHdvw6rXb4W8-5AHNsAr4iks5qLYeEgaJpZM4Iz4mC>.
>
---
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 #28: Add option to enable Google Profiler.
Posted by navsan <gi...@git.apache.org>.
Github user navsan commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/28#discussion_r66727857
--- Diff: cli/QuickstepCli.cpp ---
@@ -430,6 +445,13 @@ int main(int argc, char* argv[]) {
reset_parser = true;
break;
}
+#ifdef QUICKSTEP_ENABLE_GOOGLE_PROFILER
+ // Profile only if profile_file_name flag is set
+ if (!query_num && !quickstep::FLAGS_profile_file_name.empty()) {
--- End diff --
I'm not sure I understand.
---
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 #28: Add option to enable Google Profiler.
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/28#discussion_r66851636
--- Diff: cli/QuickstepCli.cpp ---
@@ -150,6 +154,14 @@ DEFINE_bool(initialize_db, false, "If true, initialize a database.");
DEFINE_bool(print_query, false,
"Print each input query statement. This is useful when running a "
"large number of queries in a batch.");
+DEFINE_string(profile_file_name, "",
--- End diff --
We can merge this one now and discuss the comments in a future PR.
---
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 #28: Add option to enable Google Profiler.
Posted by navsan <gi...@git.apache.org>.
Github user navsan commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/28#discussion_r66734268
--- Diff: cli/QuickstepCli.cpp ---
@@ -430,6 +445,13 @@ int main(int argc, char* argv[]) {
reset_parser = true;
break;
}
+#ifdef QUICKSTEP_ENABLE_GOOGLE_PROFILER
+ // Profile only if profile_file_name flag is set
+ if (!query_num && !quickstep::FLAGS_profile_file_name.empty()) {
--- End diff --
@zuyu, Sorry, was AFK. The latest version does something similar.
---
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 #28: Add option to enable Google Profiler.
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/28#discussion_r66826982
--- Diff: cli/QuickstepCli.cpp ---
@@ -430,6 +445,13 @@ int main(int argc, char* argv[]) {
reset_parser = true;
break;
}
+#ifdef QUICKSTEP_ENABLE_GOOGLE_PROFILER
+ // Profile only if profile_file_name flag is set
+ if (!started_profiling && !quickstep::FLAGS_profile_file_name.empty()) {
--- End diff --
Thanks for the reply.
Then, we should also put the explanation as a comment in the file, and we'll be good to 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 issue #28: Add option to enable Google Profiler.
Posted by hbdeshmukh <gi...@git.apache.org>.
Github user hbdeshmukh commented on the issue:
https://github.com/apache/incubator-quickstep/pull/28
LGTM. We can merge after the conflicts are resolved.
---
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 #28: Add option to enable Google Profiler.
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/28#discussion_r66733198
--- Diff: cli/QuickstepCli.cpp ---
@@ -430,6 +445,13 @@ int main(int argc, char* argv[]) {
reset_parser = true;
break;
}
+#ifdef QUICKSTEP_ENABLE_GOOGLE_PROFILER
+ // Profile only if profile_file_name flag is set
+ if (!query_num && !quickstep::FLAGS_profile_file_name.empty()) {
--- End diff --
@navsan Ping again this issue. I did not see we use a bool variable here. Do we go with any alternative options?
---
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 #28: Add option to enable Google Profiler.
Posted by navsan <gi...@git.apache.org>.
Github user navsan commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/28#discussion_r66727854
--- Diff: cli/QuickstepCli.cpp ---
@@ -440,6 +462,13 @@ int main(int argc, char* argv[]) {
}
}
+#ifdef QUICKSTEP_ENABLE_GOOGLE_PROFILER
+ if (!query_num && !quickstep::FLAGS_profile_file_name.empty()) {
--- End diff --
You're right. I'll fix. The profiler produces an output even if you don't explicitly stop the profiler, which is why I didn't catch it. Good catch!
---
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 #28: Add option to enable Google Profiler.
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/28#discussion_r66730358
--- Diff: cli/QuickstepCli.cpp ---
@@ -430,6 +445,13 @@ int main(int argc, char* argv[]) {
reset_parser = true;
break;
}
+#ifdef QUICKSTEP_ENABLE_GOOGLE_PROFILER
+ // Profile only if profile_file_name flag is set
+ if (!query_num && !quickstep::FLAGS_profile_file_name.empty()) {
--- End diff --
This is related to the bug we fixed above.
I mean, we should have a bool variable to indicate whether we start profiling, and use that variable to stop profiling, instead of https://github.com/apache/incubator-quickstep/pull/28/files#diff-451b38b1d63242a7d708a368d3c79e21R466.
```
bool use_profiler = false;
if (!query_num && !quickstep::FLAGS_profile_file_name.empty()) {
use_profiler = true;
ProfilerStart(...);
}
if (use_profiler) {
ProfilerStop();
ProfilerFlush();
}
```
---
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 #28: Add option to enable Google Profiler.
Posted by navsan <gi...@git.apache.org>.
Github user navsan commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/28#discussion_r66851286
--- Diff: cli/QuickstepCli.cpp ---
@@ -150,6 +154,14 @@ DEFINE_bool(initialize_db, false, "If true, initialize a database.");
DEFINE_bool(print_query, false,
"Print each input query statement. This is useful when running a "
"large number of queries in a batch.");
+DEFINE_string(profile_file_name, "",
--- End diff --
Thanks @hbdeshmukh for those suggestions. I'm not sure I completely agree with them, but before we discuss them, are you suggesting that we make these changes in the current PR or put it off to another one? If the latter, let's merge and discuss this on another thread (maybe a JIRA issue?).
---
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 #28: Add option to enable Google Profiler.
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/28#discussion_r66727823
--- Diff: cli/QuickstepCli.cpp ---
@@ -430,6 +445,13 @@ int main(int argc, char* argv[]) {
reset_parser = true;
break;
}
+#ifdef QUICKSTEP_ENABLE_GOOGLE_PROFILER
+ // Profile only if profile_file_name flag is set
+ if (!query_num && !quickstep::FLAGS_profile_file_name.empty()) {
--- End diff --
Question: Do we try to use a singleton pattern here?
---
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 #28: Add option to enable Google Profiler.
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-quickstep/pull/28
---
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 #28: Add option to enable Google Profiler.
Posted by navsan <gi...@git.apache.org>.
Github user navsan commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/28#discussion_r66728616
--- Diff: cli/QuickstepCli.cpp ---
@@ -440,6 +462,13 @@ int main(int argc, char* argv[]) {
}
}
+#ifdef QUICKSTEP_ENABLE_GOOGLE_PROFILER
+ if (!query_num && !quickstep::FLAGS_profile_file_name.empty()) {
--- End diff --
Fixed 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.
---
[GitHub] incubator-quickstep issue #28: Add option to enable Google Profiler.
Posted by navsan <gi...@git.apache.org>.
Github user navsan commented on the issue:
https://github.com/apache/incubator-quickstep/pull/28
OK, it should be ready now, I think.
---
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 #28: Add option to enable Google Profiler.
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/28#discussion_r66727801
--- Diff: cli/QuickstepCli.cpp ---
@@ -440,6 +462,13 @@ int main(int argc, char* argv[]) {
}
}
+#ifdef QUICKSTEP_ENABLE_GOOGLE_PROFILER
+ if (!query_num && !quickstep::FLAGS_profile_file_name.empty()) {
--- End diff --
Look like a bug to me: `query_num` would be `non-zero` if we start to profile.
---
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 #28: Add option to enable Google Profiler.
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/28#discussion_r66825014
--- Diff: cli/QuickstepCli.cpp ---
@@ -150,6 +154,14 @@ DEFINE_bool(initialize_db, false, "If true, initialize a database.");
DEFINE_bool(print_query, false,
"Print each input query statement. This is useful when running a "
"large number of queries in a batch.");
+DEFINE_string(profile_file_name, "",
--- End diff --
One thing which we can do is generate a default profile name if none is specified. The default profile name can be something like "quickstep-profile-<timestamp>.prof", so that name collisions are avoided. That way, we can get rid of the check ``!quickstep::FLAGS_profile_file_name.empty()`` in Line 450. It's ok if we don't do it right away, in which case you can leave a TODO note.
---
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 #28: Add option to enable Google Profiler.
Posted by navsan <gi...@git.apache.org>.
Github user navsan commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/28#discussion_r66853219
--- Diff: cli/QuickstepCli.cpp ---
@@ -430,6 +445,13 @@ int main(int argc, char* argv[]) {
reset_parser = true;
break;
}
+#ifdef QUICKSTEP_ENABLE_GOOGLE_PROFILER
+ // Profile only if profile_file_name flag is set
+ if (!started_profiling && !quickstep::FLAGS_profile_file_name.empty()) {
--- End diff --
Added the explanation in the the QuickstepCli.cpp file. Are we good to merge?
Thanks @zuyu, @hbdeshmukh for the thorough 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 pull request #28: Add option to enable Google Profiler.
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/28#discussion_r66830297
--- Diff: cli/QuickstepCli.cpp ---
@@ -150,6 +154,14 @@ DEFINE_bool(initialize_db, false, "If true, initialize a database.");
DEFINE_bool(print_query, false,
"Print each input query statement. This is useful when running a "
"large number of queries in a batch.");
+DEFINE_string(profile_file_name, "",
--- End diff --
Alright. So, if I get it right, we need to provide three parameters, BUILD_TYPE as RelWithDebInfo, ENABLE_GOOGLE_PROFILER as ON and a profile_file_name string for getting the profiling output. There are two options, which if achievable, can make things a little bit simpler.
1. When someone says ENABLE_GOOGLE_PROFILER is ON, we automatically change the BUILD_TYPE to RelWithDebInfo OR
2. Create a new BUILD_TYPE as "Profile", which internally takes care of enabling the profiling, ensuring that the necessary debug symbols are present in the code and optionally choosing a default profiling output filename, if one not specified. Once the quickstep process terminates, we can just print a message displaying the filename of the profiling output.
---
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 #28: Add option to enable Google Profiler.
Posted by navsan <gi...@git.apache.org>.
Github user navsan commented on the issue:
https://github.com/apache/incubator-quickstep/pull/28
Thanks. I'll resolve and resubmit. I seem to have messed something up in my local branch.
---
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 #28: Add option to enable Google Profiler.
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/28#discussion_r66737198
--- Diff: cli/QuickstepCli.cpp ---
@@ -430,6 +445,13 @@ int main(int argc, char* argv[]) {
reset_parser = true;
break;
}
+#ifdef QUICKSTEP_ENABLE_GOOGLE_PROFILER
+ // Profile only if profile_file_name flag is set
+ if (!started_profiling && !quickstep::FLAGS_profile_file_name.empty()) {
--- End diff --
Now we are using the singleton design pattern.
I still have a question: why we start profiling after one query finishes execution, instead of before any query starts? 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 #28: Add option to enable Google Profiler.
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/28
LGTM.
---
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 #28: Add option to enable Google Profiler.
Posted by navsan <gi...@git.apache.org>.
Github user navsan commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/28#discussion_r66870439
--- Diff: cli/QuickstepCli.cpp ---
@@ -430,6 +445,13 @@ int main(int argc, char* argv[]) {
reset_parser = true;
break;
}
+#ifdef QUICKSTEP_ENABLE_GOOGLE_PROFILER
+ // Profile only if profile_file_name flag is set
+ if (!started_profiling && !quickstep::FLAGS_profile_file_name.empty()) {
--- End diff --
It\u2019s at the top, below the definition of the flag. It\u2019s more useful there, since it\u2019s really a comment about how to use the flag. But it\u2019s too long to be part of the flag description itself.
> On Jun 13, 2016, at 16:18, Zuyu ZHANG <no...@github.com> wrote:
>
> In cli/QuickstepCli.cpp <https://github.com/apache/incubator-quickstep/pull/28#discussion_r66870133>:
>
> > @@ -430,6 +445,13 @@ int main(int argc, char* argv[]) {
> > reset_parser = true;
> > break;
> > }
> > +#ifdef QUICKSTEP_ENABLE_GOOGLE_PROFILER
> > + // Profile only if profile_file_name flag is set
> > + if (!started_profiling && !quickstep::FLAGS_profile_file_name.empty()) {
> @navsan <https://github.com/navsan> I did not see the detailed explanation beyond Profile only if profile_file_name flag is set. Did I miss something?
>
> \u2014
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub <https://github.com/apache/incubator-quickstep/pull/28/files/00955f19652fe54468377e932365d77a13cc69c8#r66870133>, or mute the thread <https://github.com/notifications/unsubscribe/ACZB66ciwFxRTrf6q0ZQ4aw1JZqljhuQks5qLckqgaJpZM4Iz4mC>.
>
---
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 #28: Add option to enable Google Profiler.
Posted by navsan <gi...@git.apache.org>.
Github user navsan commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/28#discussion_r66734229
--- Diff: cli/QuickstepCli.cpp ---
@@ -335,6 +347,9 @@ int main(int argc, char* argv[]) {
std::unique_ptr<SqlParserWrapper> parser_wrapper(new SqlParserWrapper());
std::chrono::time_point<std::chrono::steady_clock> start, end;
+#ifdef QUICKSTEP_ENABLE_GOOGLE_PROFILER
+ unsigned int query_num = 0;
--- End diff --
See the latest version. There's a bool variable declared here instead. We need the declaration to be outside the for loop, so there'll still be a code change here.
---
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 #28: Add option to enable Google Profiler.
Posted by zuyu <gi...@git.apache.org>.
Github user zuyu commented on the issue:
https://github.com/apache/incubator-quickstep/pull/28
Merged!
---
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 #28: Add option to enable Google Profiler.
Posted by navsan <gi...@git.apache.org>.
Github user navsan commented on the issue:
https://github.com/apache/incubator-quickstep/pull/28
Thanks Zuyu!
> On Jun 13, 2016, at 16:56, Zuyu ZHANG <no...@github.com> wrote:
>
> Merged!
>
> \u2014
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub <https://github.com/apache/incubator-quickstep/pull/28#issuecomment-225720999>, or mute the thread <https://github.com/notifications/unsubscribe/ACZB66ieTRoH8yq2MvGgrIGkSMU7MFzNks5qLdIagaJpZM4Iz4mC>.
>
---
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 #28: Add option to enable Google Profiler.
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/28#discussion_r66870133
--- Diff: cli/QuickstepCli.cpp ---
@@ -430,6 +445,13 @@ int main(int argc, char* argv[]) {
reset_parser = true;
break;
}
+#ifdef QUICKSTEP_ENABLE_GOOGLE_PROFILER
+ // Profile only if profile_file_name flag is set
+ if (!started_profiling && !quickstep::FLAGS_profile_file_name.empty()) {
--- End diff --
@navsan I did not see the detailed explanation beyond `Profile only if profile_file_name flag is set`. Did I miss something?
---
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 #28: Add option to enable Google Profiler.
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/28#discussion_r66733141
--- Diff: cli/QuickstepCli.cpp ---
@@ -335,6 +347,9 @@ int main(int argc, char* argv[]) {
std::unique_ptr<SqlParserWrapper> parser_wrapper(new SqlParserWrapper());
std::chrono::time_point<std::chrono::steady_clock> start, end;
+#ifdef QUICKSTEP_ENABLE_GOOGLE_PROFILER
+ unsigned int query_num = 0;
--- End diff --
We could remove it then.
---
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 #28: Add option to enable Google Profiler.
Posted by navsan <gi...@git.apache.org>.
Github user navsan commented on a diff in the pull request:
https://github.com/apache/incubator-quickstep/pull/28#discussion_r66740805
--- Diff: cli/QuickstepCli.cpp ---
@@ -430,6 +445,13 @@ int main(int argc, char* argv[]) {
reset_parser = true;
break;
}
+#ifdef QUICKSTEP_ENABLE_GOOGLE_PROFILER
+ // Profile only if profile_file_name flag is set
+ if (!started_profiling && !quickstep::FLAGS_profile_file_name.empty()) {
--- End diff --
Good question. It\u2019s because, unless you\u2019ve preloaded the buffer pool (not always a good idea), the first run of the query results in disk I/O and other overhead that significantly skews the profiling results. It\u2019s the same reason we don\u2019t include the first run time in our benchmarking: when profiling query execution, it makes more sense to get numbers using a warm buffer pool and warm caches. This is not *always* the right thing to do: it\u2019s obviously wrong for profiling the TextScan operator. In those cases, you might want to put in your own Profiler probes (just follow the start/stop pattern used in this PR) or just run quickstep with the CPUPROFILE environment variable set (as per gperftools documentation) to get the full profile for the entire execution.
I\u2019ve provided the explanation in the flag definition, CMake comments as well as in the PR header comment.
> On Jun 12, 2016, at 22:49, Zuyu ZHANG <no...@github.com> wrote:
>
> In cli/QuickstepCli.cpp <https://github.com/apache/incubator-quickstep/pull/28#discussion_r66737198>:
>
> > @@ -430,6 +445,13 @@ int main(int argc, char* argv[]) {
> > reset_parser = true;
> > break;
> > }
> > +#ifdef QUICKSTEP_ENABLE_GOOGLE_PROFILER
> > + // Profile only if profile_file_name flag is set
> > + if (!started_profiling && !quickstep::FLAGS_profile_file_name.empty()) {
> Now we are using the singleton design pattern.
>
> I still have a question: why we start profiling after one query finishes execution, instead of before any query starts? Thanks!
>
> \u2014
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub <https://github.com/apache/incubator-quickstep/pull/28/files/00955f19652fe54468377e932365d77a13cc69c8#r66737198>, or mute the thread <https://github.com/notifications/unsubscribe/ACZB62yfZt1S-zIV8u5HZq-Dryl6gVHLks5qLNNHgaJpZM4Iz4mC>.
>
---
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.
---