impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
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:


Thanks for doing this. The overall idea looks good, just some comments about a few details.
File be/src/util/

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I7e0ddfaa9bd1340990ae2b6ff946d72bbd26c274
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Reviewer: Zach Amsden <>
Gerrit-HasComments: Yes

View raw message