Commit f296a1d0 authored by Sushil Patil's avatar Sushil Patil Committed by Facebook Github Bot

Replacing `folly::AtomicBitset<N>` with `folly::ConcurrentBitSet<N>`

Summary:
`folly::AtomicBitSet<N>` used `N` to denote the number of blocks
(32-bit or 64-bit each) instead of number of bits as used by
`std::bitset`. This confusion of unit led to unintentional allocation
of extra storage in many cases.

A simple fix would be to make `N` denote number of bits. However,
changing meaning of `N`could lead to silent breakage of code during
run time for some users who used `N` to denote number of blocks.

Therefore, to make the new meaning of `N` more explicit,
`AtomicBitSet` is renamed to `ConcurrentBitSet` & `N` now denotes the
number of bits.

Reviewed By: al13n321

Differential Revision: D18866751

fbshipit-source-id: 17741646862776e1a080eaac7877f495bb56b656
parent 73ccc053
...@@ -803,7 +803,6 @@ if (BUILD_TESTS) ...@@ -803,7 +803,6 @@ if (BUILD_TESTS)
TEST ahm_int_stress_test SOURCES AHMIntStressTest.cpp TEST ahm_int_stress_test SOURCES AHMIntStressTest.cpp
TEST arena_smartptr_test SOURCES ArenaSmartPtrTest.cpp TEST arena_smartptr_test SOURCES ArenaSmartPtrTest.cpp
TEST ascii_check_test SOURCES AsciiCaseInsensitiveTest.cpp TEST ascii_check_test SOURCES AsciiCaseInsensitiveTest.cpp
TEST atomic_bit_set_test SOURCES AtomicBitSetTest.cpp
TEST atomic_hash_array_test SOURCES AtomicHashArrayTest.cpp TEST atomic_hash_array_test SOURCES AtomicHashArrayTest.cpp
TEST atomic_hash_map_test HANGING TEST atomic_hash_map_test HANGING
SOURCES AtomicHashMapTest.cpp SOURCES AtomicHashMapTest.cpp
...@@ -811,6 +810,7 @@ if (BUILD_TESTS) ...@@ -811,6 +810,7 @@ if (BUILD_TESTS)
TEST atomic_unordered_map_test SOURCES AtomicUnorderedMapTest.cpp TEST atomic_unordered_map_test SOURCES AtomicUnorderedMapTest.cpp
TEST cacheline_padded_test SOURCES CachelinePaddedTest.cpp TEST cacheline_padded_test SOURCES CachelinePaddedTest.cpp
TEST clock_gettime_wrappers_test SOURCES ClockGettimeWrappersTest.cpp TEST clock_gettime_wrappers_test SOURCES ClockGettimeWrappersTest.cpp
TEST concurrent_bit_set_test SOURCES ConcurrentBitSetTest.cpp
TEST concurrent_skip_list_test SOURCES ConcurrentSkipListTest.cpp TEST concurrent_skip_list_test SOURCES ConcurrentSkipListTest.cpp
TEST conv_test SOURCES ConvTest.cpp TEST conv_test SOURCES ConvTest.cpp
TEST cpu_id_test SOURCES CpuIdTest.cpp TEST cpu_id_test SOURCES CpuIdTest.cpp
......
...@@ -28,17 +28,20 @@ namespace folly { ...@@ -28,17 +28,20 @@ namespace folly {
/** /**
* An atomic bitset of fixed size (specified at compile time). * An atomic bitset of fixed size (specified at compile time).
*
* Formerly known as AtomicBitSet. It was renamed while fixing a bug
* to avoid any silent breakages during run time.
*/ */
template <size_t N> template <size_t N>
class AtomicBitSet { class ConcurrentBitSet {
public: public:
/** /**
* Construct an AtomicBitSet; all bits are initially false. * Construct a ConcurrentBitSet; all bits are initially false.
*/ */
AtomicBitSet(); ConcurrentBitSet();
AtomicBitSet(const AtomicBitSet&) = delete; ConcurrentBitSet(const ConcurrentBitSet&) = delete;
AtomicBitSet& operator=(const AtomicBitSet&) = delete; ConcurrentBitSet& operator=(const ConcurrentBitSet&) = delete;
/** /**
* Set bit idx to true, using the given memory order. Returns the * Set bit idx to true, using the given memory order. Returns the
...@@ -116,43 +119,44 @@ class AtomicBitSet { ...@@ -116,43 +119,44 @@ class AtomicBitSet {
// avoid casts // avoid casts
static constexpr BlockType kOne = 1; static constexpr BlockType kOne = 1;
static constexpr size_t kNumBlocks = (N + kBitsPerBlock - 1) / kBitsPerBlock;
std::array<AtomicBlockType, N> data_; std::array<AtomicBlockType, kNumBlocks> data_;
}; };
// value-initialize to zero // value-initialize to zero
template <size_t N> template <size_t N>
inline AtomicBitSet<N>::AtomicBitSet() : data_() {} inline ConcurrentBitSet<N>::ConcurrentBitSet() : data_() {}
template <size_t N> template <size_t N>
inline bool AtomicBitSet<N>::set(size_t idx, std::memory_order order) { inline bool ConcurrentBitSet<N>::set(size_t idx, std::memory_order order) {
assert(idx < N * kBitsPerBlock); assert(idx < N);
BlockType mask = kOne << bitOffset(idx); BlockType mask = kOne << bitOffset(idx);
return data_[blockIndex(idx)].fetch_or(mask, order) & mask; return data_[blockIndex(idx)].fetch_or(mask, order) & mask;
} }
template <size_t N> template <size_t N>
inline bool AtomicBitSet<N>::reset(size_t idx, std::memory_order order) { inline bool ConcurrentBitSet<N>::reset(size_t idx, std::memory_order order) {
assert(idx < N * kBitsPerBlock); assert(idx < N);
BlockType mask = kOne << bitOffset(idx); BlockType mask = kOne << bitOffset(idx);
return data_[blockIndex(idx)].fetch_and(~mask, order) & mask; return data_[blockIndex(idx)].fetch_and(~mask, order) & mask;
} }
template <size_t N> template <size_t N>
inline bool inline bool
AtomicBitSet<N>::set(size_t idx, bool value, std::memory_order order) { ConcurrentBitSet<N>::set(size_t idx, bool value, std::memory_order order) {
return value ? set(idx, order) : reset(idx, order); return value ? set(idx, order) : reset(idx, order);
} }
template <size_t N> template <size_t N>
inline bool AtomicBitSet<N>::test(size_t idx, std::memory_order order) const { inline bool ConcurrentBitSet<N>::test(size_t idx, std::memory_order order)
assert(idx < N * kBitsPerBlock); const {
assert(idx < N);
BlockType mask = kOne << bitOffset(idx); BlockType mask = kOne << bitOffset(idx);
return data_[blockIndex(idx)].load(order) & mask; return data_[blockIndex(idx)].load(order) & mask;
} }
template <size_t N> template <size_t N>
inline bool AtomicBitSet<N>::operator[](size_t idx) const { inline bool ConcurrentBitSet<N>::operator[](size_t idx) const {
return test(idx); return test(idx);
} }
......
...@@ -14,7 +14,7 @@ ...@@ -14,7 +14,7 @@
* limitations under the License. * limitations under the License.
*/ */
#include <folly/AtomicBitSet.h> #include <folly/ConcurrentBitSet.h>
#include <folly/portability/GFlags.h> #include <folly/portability/GFlags.h>
#include <folly/portability/GTest.h> #include <folly/portability/GTest.h>
...@@ -24,9 +24,9 @@ ...@@ -24,9 +24,9 @@
namespace folly { namespace folly {
namespace test { namespace test {
TEST(AtomicBitSet, Simple) { TEST(ConcurrentBitSet, Simple) {
constexpr size_t kSize = 1000; constexpr size_t kSize = 1000;
AtomicBitSet<kSize> bs; ConcurrentBitSet<kSize> bs;
EXPECT_EQ(kSize, bs.size()); EXPECT_EQ(kSize, bs.size());
...@@ -53,6 +53,16 @@ TEST(AtomicBitSet, Simple) { ...@@ -53,6 +53,16 @@ TEST(AtomicBitSet, Simple) {
for (size_t i = 0; i < kSize; ++i) { for (size_t i = 0; i < kSize; ++i) {
EXPECT_FALSE(bs[i]); EXPECT_FALSE(bs[i]);
} }
bs.set(268);
for (size_t i = 0; i < kSize; ++i) {
EXPECT_EQ(i == 268, bs[i]);
}
bs.reset(268);
for (size_t i = 0; i < kSize; ++i) {
EXPECT_FALSE(bs[i]);
}
} }
} // namespace test } // namespace test
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment