You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Lars Volker (Code Review)" <ge...@cloudera.org> on 2016/09/23 20:19:02 UTC
[Impala-ASF-CR] WIP: Issue warnings when benchmarks run with sub-optimal system settings
Lars Volker has uploaded a new change for review.
http://gerrit.cloudera.org:8080/4528
Change subject: WIP: Issue warnings when benchmarks run with sub-optimal system settings
......................................................................
WIP: Issue warnings when benchmarks run with sub-optimal system settings
This is related to IMPALA-4193 but should probably have its own Jira if
we decide to move forward with it.
Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
---
M be/src/util/benchmark.cc
M be/src/util/cpu-info.cc
M be/src/util/cpu-info.h
3 files changed, 42 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/4528/1
--
To view, visit http://gerrit.cloudera.org:8080/4528
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.
Change subject: IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
......................................................................
Patch Set 7: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/4528
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] WIP: Issue warnings when benchmarks run with sub-optimal system settings
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has uploaded a new patch set (#2).
Change subject: WIP: Issue warnings when benchmarks run with sub-optimal system settings
......................................................................
WIP: Issue warnings when benchmarks run with sub-optimal system settings
This is related to IMPALA-4193 but should probably have its own Jira if
we decide to move forward with it.
Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
---
M be/src/util/benchmark.cc
M be/src/util/cpu-info.cc
M be/src/util/cpu-info.h
3 files changed, 47 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/4528/2
--
To view, visit http://gerrit.cloudera.org:8080/4528
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
[Impala-ASF-CR] IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
......................................................................
Patch Set 5:
(3 comments)
Thanks for the review, please see PS5.
http://gerrit.cloudera.org:8080/#/c/4528/3/be/src/util/cpu-info.cc
File be/src/util/cpu-info.cc:
PS3, Line 86: bool
> Nobody is checking this.
Indeed, fixed it.
PS3, Line 90: (
> The ifstream destructor does this automatically, doesn't it?
It does, removed it here and elsewhere in the file.
PS3, Line 172:
: void CpuInfo::VerifyTurboDisabled() {
: WarnIfFileNotEqual("/sys/devices/system/cpu/intel_pstate/no_turbo", "1",
: "WARNING: CPU turbo is enabled. This setting can change the clock "
: "frequency of CPU cores during the benchmark run, which can lead to "
: "inaccurate results. You can disable CPU turbo by writing a 1 to"
: "/sys/devices/system/cpu/intel_pstate/no_turbo");
: }
> Even this whole thing can be a helper function now,
Done
--
To view, visit http://gerrit.cloudera.org:8080/4528
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
......................................................................
Patch Set 6:
(4 comments)
Thank you. Please see PS6.
http://gerrit.cloudera.org:8080/#/c/4528/5/be/src/util/cpu-info.cc
File be/src/util/cpu-info.cc:
Line 86: // values contains a list of space-seperated flags. check to see if the flags we
> now this is only called one place, so you can just inline it there.
Done
Line 90: int64_t flags = 0;
> Why trim?
Habit, to get rid of any whitespace. It is not needed here, removed it.
PS5, Line 94:
> " as its first line"
Done
Line 178: DCHECK(initialized_);
I noticed that setting the governor to performance will set the no_turbo flag back to 0. Should we mention this here?
--
To view, visit http://gerrit.cloudera.org:8080/4528
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has uploaded a new patch set (#7).
Change subject: IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
......................................................................
IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
---
M be/src/util/benchmark.cc
M be/src/util/cpu-info.cc
M be/src/util/cpu-info.h
3 files changed, 46 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/4528/7
--
To view, visit http://gerrit.cloudera.org:8080/4528
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has submitted this change and it was merged.
Change subject: IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
......................................................................
IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
Reviewed-on: http://gerrit.cloudera.org:8080/4528
Reviewed-by: Jim Apple <jb...@cloudera.com>
Tested-by: Internal Jenkins
---
M be/src/util/benchmark.cc
M be/src/util/cpu-info.cc
M be/src/util/cpu-info.h
3 files changed, 46 insertions(+), 2 deletions(-)
Approvals:
Jim Apple: Looks good to me, approved
Internal Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4528
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4528
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/4528/3/be/src/util/cpu-info.cc
File be/src/util/cpu-info.cc:
PS3, Line 86: bool
Nobody is checking this.
PS3, Line 90: .
The ifstream destructor does this automatically, doesn't it?
PS3, Line 172: string turbo_file = "/sys/devices/system/cpu/intel_pstate/no_turbo";
: string turbo_off;
: ReadLine(turbo_file, &turbo_off);
: if (turbo_off != "1")
: LOG(ERROR) << "WARNING: CPU turbo is enabled. This setting can change the clock "
: << "frequency of CPU cores during the benchmark run, which can lead to "
: << "inaccurate results. You can disable CPU turbo by writing a 1 to"
: << "/sys/devices/system/cpu/intel_pstate/no_turbo";
Even this whole thing can be a helper function now,
WarnIfFileContents(string filename, string expected, string warning_text)
--
To view, visit http://gerrit.cloudera.org:8080/4528
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
......................................................................
Patch Set 7:
Thanks, Jim, for the review. Started the GVO.
--
To view, visit http://gerrit.cloudera.org:8080/4528
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has uploaded a new patch set (#6).
Change subject: IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
......................................................................
IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
---
M be/src/util/benchmark.cc
M be/src/util/cpu-info.cc
M be/src/util/cpu-info.h
3 files changed, 43 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/4528/6
--
To view, visit http://gerrit.cloudera.org:8080/4528
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has uploaded a new patch set (#5).
Change subject: IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
......................................................................
IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
---
M be/src/util/benchmark.cc
M be/src/util/cpu-info.cc
M be/src/util/cpu-info.h
3 files changed, 47 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/4528/5
--
To view, visit http://gerrit.cloudera.org:8080/4528
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has uploaded a new patch set (#3).
Change subject: IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
......................................................................
IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
---
M be/src/util/benchmark.cc
M be/src/util/cpu-info.cc
M be/src/util/cpu-info.h
3 files changed, 47 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/4528/3
--
To view, visit http://gerrit.cloudera.org:8080/4528
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
[Impala-ASF-CR] IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
......................................................................
Patch Set 5:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/4528/5/be/src/util/cpu-info.cc
File be/src/util/cpu-info.cc:
Line 86: bool ReadLine(const string& filename, string* out) {
now this is only called one place, so you can just inline it there.
Line 90: trim(*out);
Why trim?
Line 94: // Helper function to warn if a given file does not contain an expected string. If the
Use anonymous namespace, since this is not called elsewhere https://google.github.io/styleguide/cppguide.html#Namespaces
PS5, Line 94: n expected string
" as its first line"
Line 100: if (line != expected) LOG(ERROR) << warning_text;
"Expected " << expected << ", actual " << line << endl << warning_text
Line 165: string governor_file =
const, here and below
--
To view, visit http://gerrit.cloudera.org:8080/4528
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4528/5/be/src/util/cpu-info.cc
File be/src/util/cpu-info.cc:
Line 178: DCHECK(initialized_);
> I noticed that setting the governor to performance will set the no_turbo fl
Either way is OK with me, though if you mention it one place, you might want to mention it both places.
--
To view, visit http://gerrit.cloudera.org:8080/4528
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] WIP: Issue warnings when benchmarks run with sub-optimal system settings
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: WIP: Issue warnings when benchmarks run with sub-optimal system settings
......................................................................
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/4528/1//COMMIT_MSG
Commit Message:
PS1, Line 9: This is related to IMPALA-4193 but should probably have its own Jira if
Doesn't this solve that issue?
http://gerrit.cloudera.org:8080/#/c/4528/1/be/src/util/cpu-info.cc
File be/src/util/cpu-info.cc:
PS1, Line 149: ios
This is the default, right?
PS1, Line 151: if (!governor_file) continue;
:
: string line;
: getline(governor_file, line);
: governor_file.close();
: trim(line);
This is in the other method, too; can you factor it out?
Line 158: if (strcmp(line.c_str(), "performance") != 0) {
Can you use a C==-style == here?
Line 172: if (atoi(line.c_str()) != 1) {
here also, you could compare to "1", right?
Line 173: LOG(ERROR) << "WARNING: CPU turbo is enabled.";
Explain why this matters - it can ramp-up and down performance dynamically
--
To view, visit http://gerrit.cloudera.org:8080/4528
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has uploaded a new patch set (#4).
Change subject: IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
......................................................................
IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
---
M be/src/util/benchmark.cc
M be/src/util/cpu-info.cc
M be/src/util/cpu-info.h
3 files changed, 47 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/28/4528/4
--
To view, visit http://gerrit.cloudera.org:8080/4528
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
[Impala-ASF-CR] IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4528/5/be/src/util/cpu-info.cc
File be/src/util/cpu-info.cc:
Line 178: }
> Either way is OK with me, though if you mention it one place, you might wan
Added comments, as I suspect this will bite people otherwise.
--
To view, visit http://gerrit.cloudera.org:8080/4528
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-4193: Warn when benchmarks run with sub-optimal CPU settings
......................................................................
Patch Set 3:
(6 comments)
Thanks for the review. Please see PS3.
http://gerrit.cloudera.org:8080/#/c/4528/1//COMMIT_MSG
Commit Message:
PS1, Line 9: Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
> Doesn't this solve that issue?
You're right, it does.
http://gerrit.cloudera.org:8080/#/c/4528/1/be/src/util/cpu-info.cc
File be/src/util/cpu-info.cc:
PS1, Line 149:
> This is the default, right?
It is, removed it here and elsewhere in the file.
PS1, Line 151: LOG(ERROR) << "CPU does not support the Supplemental SSE3 (SSSE3) instruction set. "
: << "This setup is generally unsupported and Impala might be unstable.";
: }
: }
:
: void CpuInfo::V
> This is in the other method, too; can you factor it out?
Done
Line 158: string governor_file =
> Can you use a C==-style == here?
Done
Line 172: string turbo_file = "/sys/devices/system/cpu/intel_pstate/no_turbo";
> here also, you could compare to "1", right?
Yes, done.
Line 173: string turbo_off;
> Explain why this matters - it can ramp-up and down performance dynamically
Done
--
To view, visit http://gerrit.cloudera.org:8080/4528
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5e879cb35cf736f6112c1caed829722a38849794
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-HasComments: Yes