impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5164: Fix flaky benchmarks
Date Mon, 22 May 2017 16:53:21 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5164: Fix flaky benchmarks
......................................................................


Patch Set 2:

(6 comments)

Thanks for doing this. The overall idea looks good, just some comments about a few details.

http://gerrit.cloudera.org:8080/#/c/6935/2/be/src/util/benchmark.cc
File be/src/util/benchmark.cc:

Line 35: double Benchmark::Measure(BenchmarkFunction function, void* args,
Can we change the behaviour so that benchmarks can opt in or out from the context switch test?

The new behaviour makes sense if the benchmark is measuring single-threaded efficiency but
doesn't make sense in some other cases. E.g. for lock-benchmark we want to understand what
the OS scheduler does when there are more threads than logical cores contending for a lcok.


Line 62:       throw std::runtime_error("Benchmark failed to complete due to context switching.");
We usually use LOG(FATAL) for fatal errors or Status for non-fatal errors. I think either
could make sense here.


Line 69:   }
Maybe we should reset 'sw' and 'lost_time' after doing the tuning step so we don't have unnecessary
lost time.


Line 90:       batch_size = max<int>(1, batch_size / (1+(ru_stop.ru_nivcsw - ru_start.ru_nivcsw)));
Maybe comment why we're scaling it down here?


PS2, Line 94: 10
Where does the 10 come from?


Line 95:     throw std::runtime_error("Benchmark failed to complete due to noisy measurements.");
Same comment here about using Status/LOG(FATAL).


-- 
To view, visit http://gerrit.cloudera.org:8080/6935
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7e0ddfaa9bd1340990ae2b6ff946d72bbd26c274
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <zamsden@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Zach Amsden <zamsden@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message