You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by kw...@apache.org on 2016/12/01 18:53:14 UTC
[5/5] incubator-impala git commit: IMPALA-4557: Fix flakiness with
FLAGS_stress_free_pool_alloc
IMPALA-4557: Fix flakiness with FLAGS_stress_free_pool_alloc
FLAGS_stress_free_pool_alloc is a gflag for simulating malloc
failure in debug builds. If set, FreePool::Allocate() and
FreePool::Reallocate() will return NULL on every
FLAGS_stress_free_pool_alloc allocations. The problem is that
the counter for tracking number of allocations is updated
racily by multiple threads, leading to non-determinism and
flaky tests. This change fixes the problem by making the counter
an AtomicInt32.
Change-Id: I373035d209a0f3477650336ee6da458fa289dbe0
Reviewed-on: http://gerrit.cloudera.org:8080/5281
Reviewed-by: Jim Apple <jb...@apache.org>
Reviewed-by: Dan Hecht <dh...@cloudera.com>
Tested-by: Internal 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/b1edca2a
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/b1edca2a
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/b1edca2a
Branch: refs/heads/master
Commit: b1edca2a5b6eb66fc5c316157f968b267ee36b48
Parents: 467642f
Author: Michael Ho <kw...@cloudera.com>
Authored: Tue Nov 29 15:27:56 2016 -0800
Committer: Internal Jenkins <cl...@gerrit.cloudera.org>
Committed: Thu Dec 1 12:03:17 2016 +0000
----------------------------------------------------------------------
be/src/common/atomic.h | 5 +++--
be/src/runtime/CMakeLists.txt | 1 +
be/src/runtime/free-pool.cc | 28 ++++++++++++++++++++++++++++
be/src/runtime/free-pool.h | 13 +++++++++----
4 files changed, 41 insertions(+), 6 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/b1edca2a/be/src/common/atomic.h
----------------------------------------------------------------------
diff --git a/be/src/common/atomic.h b/be/src/common/atomic.h
index 1afdedf..784be11 100644
--- a/be/src/common/atomic.h
+++ b/be/src/common/atomic.h
@@ -71,8 +71,9 @@ template<typename T>
class AtomicInt {
public:
AtomicInt(T initial = 0) : value_(initial) {
- DCHECK(sizeof(T) == sizeof(base::subtle::Atomic32) ||
- sizeof(T) == sizeof(base::subtle::Atomic64));
+ static_assert(sizeof(T) == sizeof(base::subtle::Atomic32) ||
+ sizeof(T) == sizeof(base::subtle::Atomic64),
+ "Only AtomicInt32 and AtomicInt64 are implemented");
}
/// Atomic load with "acquire" memory-ordering semantic.
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/b1edca2a/be/src/runtime/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/be/src/runtime/CMakeLists.txt b/be/src/runtime/CMakeLists.txt
index 5114ff9..6602f07 100644
--- a/be/src/runtime/CMakeLists.txt
+++ b/be/src/runtime/CMakeLists.txt
@@ -37,6 +37,7 @@ add_library(Runtime
disk-io-mgr-scan-range.cc
disk-io-mgr-stress.cc
exec-env.cc
+ free-pool.cc
hbase-table.cc
hbase-table-factory.cc
hdfs-fs-cache.cc
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/b1edca2a/be/src/runtime/free-pool.cc
----------------------------------------------------------------------
diff --git a/be/src/runtime/free-pool.cc b/be/src/runtime/free-pool.cc
new file mode 100644
index 0000000..df0569d
--- /dev/null
+++ b/be/src/runtime/free-pool.cc
@@ -0,0 +1,28 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "runtime/free-pool.h"
+
+#include "common/names.h"
+
+using namespace impala;
+
+#ifndef NDEBUG
+
+AtomicInt32 FreePool::alloc_counts_(0);
+
+#endif
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/b1edca2a/be/src/runtime/free-pool.h
----------------------------------------------------------------------
diff --git a/be/src/runtime/free-pool.h b/be/src/runtime/free-pool.h
index 746b649..98b1ebe 100644
--- a/be/src/runtime/free-pool.h
+++ b/be/src/runtime/free-pool.h
@@ -23,6 +23,7 @@
#include <string.h>
#include <string>
#include <sstream>
+#include "common/atomic.h"
#include "common/logging.h"
#include "gutil/bits.h"
#include "runtime/mem-pool.h"
@@ -60,9 +61,8 @@ class FreePool {
uint8_t* Allocate(int64_t size) {
DCHECK_GE(size, 0);
#ifndef NDEBUG
- static int32_t alloc_counts = 0;
if (FLAGS_stress_free_pool_alloc > 0 &&
- (++alloc_counts % FLAGS_stress_free_pool_alloc) == 0) {
+ (alloc_counts_.Add(1) % FLAGS_stress_free_pool_alloc) == 0) {
return NULL;
}
#endif
@@ -121,9 +121,8 @@ class FreePool {
/// free the memory buffer pointed to by "ptr" in this case.
uint8_t* Reallocate(uint8_t* ptr, int64_t size) {
#ifndef NDEBUG
- static int32_t alloc_counts = 0;
if (FLAGS_stress_free_pool_alloc > 0 &&
- (++alloc_counts % FLAGS_stress_free_pool_alloc) == 0) {
+ (alloc_counts_.Add(1) % FLAGS_stress_free_pool_alloc) == 0) {
return NULL;
}
#endif
@@ -209,6 +208,12 @@ class FreePool {
/// Diagnostic counter that tracks (# Allocates - # Frees)
int64_t net_allocations_;
+
+#ifndef NDEBUG
+ /// Counter for tracking the number of allocations. Used only if the
+ /// the stress flag FLAGS_stress_free_pool_alloc is set.
+ static AtomicInt32 alloc_counts_;
+#endif
};
}