You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ta...@apache.org on 2017/08/05 17:26:42 UTC
incubator-impala git commit: IMPALA-5031: unsafe random number
generation in buffer-pool-test
Repository: incubator-impala
Updated Branches:
refs/heads/master a98b90bd3 -> 6b7425a57
IMPALA-5031: unsafe random number generation in buffer-pool-test
The bug is that ConcurrentRegistration shares one random number
generator between all the threads. This isn't safe and UBSAN was
unhappy with it.
The fix is to create one RNG per thread. We already do that in a
different test so the code is factored out into a utility function.
Testing:
Ran buffer-pool-test under UBSAN
Change-Id: I0eb3bec152a58d9ec39413780cb2c431dd8d4fa8
Reviewed-on: http://gerrit.cloudera.org:8080/7596
Reviewed-by: Sailesh Mukil <sa...@cloudera.com>
Tested-by: Impala Public Jenkins
Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/6b7425a5
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/6b7425a5
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/6b7425a5
Branch: refs/heads/master
Commit: 6b7425a574b7d5c37562c6fb0d3d1a148cf21ea0
Parents: a98b90b
Author: Tim Armstrong <ta...@cloudera.com>
Authored: Fri Aug 4 16:31:56 2017 -0700
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Sat Aug 5 04:40:01 2017 +0000
----------------------------------------------------------------------
be/src/runtime/bufferpool/buffer-pool-test.cc | 22 ++++++++++++----------
be/src/testutil/rand-util.h | 11 ++++++++++-
2 files changed, 22 insertions(+), 11 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/6b7425a5/be/src/runtime/bufferpool/buffer-pool-test.cc
----------------------------------------------------------------------
diff --git a/be/src/runtime/bufferpool/buffer-pool-test.cc b/be/src/runtime/bufferpool/buffer-pool-test.cc
index c98ba74..14446e7 100644
--- a/be/src/runtime/bufferpool/buffer-pool-test.cc
+++ b/be/src/runtime/bufferpool/buffer-pool-test.cc
@@ -98,9 +98,10 @@ class BufferPoolTest : public ::testing::Test {
const static int64_t TEST_BUFFER_LEN = 1024;
/// Test helper to simulate registering then deregistering a number of queries with
- /// the given initial reservation and reservation limit.
+ /// the given initial reservation and reservation limit. 'rng' is used to generate
+ /// any random numbers needed.
void RegisterQueriesAndClients(BufferPool* pool, int query_id_hi, int num_queries,
- int64_t initial_query_reservation, int64_t query_reservation_limit);
+ int64_t initial_query_reservation, int64_t query_reservation_limit, mt19937* rng);
/// Create and destroy a page multiple times.
void CreatePageLoop(BufferPool* pool, TmpFileMgr::FileGroup* file_group,
@@ -389,7 +390,8 @@ class BufferPoolTest : public ::testing::Test {
const int64_t BufferPoolTest::TEST_BUFFER_LEN;
void BufferPoolTest::RegisterQueriesAndClients(BufferPool* pool, int query_id_hi,
- int num_queries, int64_t initial_query_reservation, int64_t query_reservation_limit) {
+ int num_queries, int64_t initial_query_reservation, int64_t query_reservation_limit,
+ mt19937* rng) {
Status status;
int clients_per_query = 32;
@@ -416,7 +418,7 @@ void BufferPoolTest::RegisterQueriesAndClients(BufferPool* pool, int query_id_hi
initial_query_reservation / clients_per_query + j
< initial_query_reservation % clients_per_query;
// Reservation limit can be anything greater or equal to the initial reservation.
- int64_t client_reservation_limit = initial_client_reservation + rng_() % 100000;
+ int64_t client_reservation_limit = initial_client_reservation + (*rng)() % 100000;
string name = Substitute("Client $0 for query $1", j, query_id);
EXPECT_OK(pool->RegisterClient(name, NULL, query_reservation, NULL,
client_reservation_limit, NewProfile(), &clients[i][j]));
@@ -453,8 +455,8 @@ TEST_F(BufferPoolTest, BasicRegistration) {
BufferPool pool(TEST_BUFFER_LEN, total_mem);
- RegisterQueriesAndClients(
- &pool, 0, num_concurrent_queries, sum_initial_reservations, reservation_limit);
+ RegisterQueriesAndClients(&pool, 0, num_concurrent_queries, sum_initial_reservations,
+ reservation_limit, &rng_);
ASSERT_EQ(global_reservations_.GetUsedReservation(), 0);
ASSERT_EQ(global_reservations_.GetChildReservations(), 0);
@@ -475,12 +477,13 @@ TEST_F(BufferPoolTest, ConcurrentRegistration) {
global_reservations_.InitRootTracker(NewProfile(), total_mem);
BufferPool pool(TEST_BUFFER_LEN, total_mem);
-
+ vector<mt19937> thread_rngs = RandTestUtil::CreateThreadLocalRngs(num_threads, &rng_);
// Launch threads, each with a different set of query IDs.
thread_group workers;
for (int i = 0; i < num_threads; ++i) {
workers.add_thread(new thread(bind(&BufferPoolTest::RegisterQueriesAndClients, this,
- &pool, i, queries_per_thread, sum_initial_reservations, reservation_limit)));
+ &pool, i, queries_per_thread, sum_initial_reservations, reservation_limit,
+ &thread_rngs[i])));
}
workers.join_all();
@@ -1767,9 +1770,8 @@ void BufferPoolTest::TestRandomInternalMulti(
MemTracker global_tracker(TOTAL_MEM);
FileGroup* shared_file_group = NewFileGroup();
thread_group workers;
- vector<mt19937> rngs(num_threads);
+ vector<mt19937> rngs = RandTestUtil::CreateThreadLocalRngs(num_threads, &rng_);
for (int i = 0; i < num_threads; ++i) {
- rngs[i].seed(rng_()); // Seed the thread-local rngs.
workers.add_thread(new thread(
[this, &pool, shared_file_group, &global_tracker, &rngs, i, multiple_pins]() {
TestRandomInternalImpl(
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/6b7425a5/be/src/testutil/rand-util.h
----------------------------------------------------------------------
diff --git a/be/src/testutil/rand-util.h b/be/src/testutil/rand-util.h
index 6cdbed4..255fcb0 100644
--- a/be/src/testutil/rand-util.h
+++ b/be/src/testutil/rand-util.h
@@ -37,11 +37,20 @@ class RandTestUtil {
if (seed_str != nullptr) {
seed = atoi(seed_str);
} else {
- seed = time(nullptr);
+ seed = std::random_device()();
}
LOG(INFO) << "Random seed (overridable with " << env_var << "): " << seed;
rng->seed(seed);
}
+
+ /// Create 'num_threads' rngs, seeded using 'seed_rng'. Used when we want each thread
+ /// to have its own RNG (since they are not thread-safe).
+ static vector<std::mt19937> CreateThreadLocalRngs(
+ int num_threads, std::mt19937* seed_rng) {
+ vector<std::mt19937> rngs(num_threads);
+ for (std::mt19937& rng : rngs) rng.seed((*seed_rng)());
+ return rngs;
+ }
};
}