Commit 587e0c18 authored by Nathan Bronson's avatar Nathan Bronson Committed by Facebook Github Bot

fix copy constructor in rare collision+erase case

Summary:
F14NodeMap and F14ValueMap copy construction could fail to copy
the chunk overflow count in a rare case where there is a suffix of chunks
that suffers an overflow, then all of the non-overflowed keys are removed,
then the map is copied. In the copy the remaining overflowed key isn't
find()able, but it is still present on the iteration sequence. This diff
fixes the problem.

Reviewed By: WillerZ, shixiao

Differential Revision: D15105104

fbshipit-source-id: cff2073246bc301054996d4bb7e8baf9ee8c0021
parent fe13e6a5
......@@ -1573,12 +1573,12 @@ class F14Table : public Policy {
chunks_->setCapacityScale(scale);
}
} else {
std::size_t maxChunkIndex = src.lastOccupiedChunk() - src.chunks_;
// happy path, no rehash but pack items toward bottom of chunk and
// use copy constructor
auto srcChunk = &src.chunks_[maxChunkIndex];
Chunk* dstChunk = &chunks_[maxChunkIndex];
// Happy path, no rehash but pack items toward bottom of chunk
// and use copy constructor. Don't try to optimize by using
// lastOccupiedChunk() because there may be higher unoccupied chunks
// with the overflow bit set.
auto srcChunk = &src.chunks_[chunkMask_];
Chunk* dstChunk = &chunks_[chunkMask_];
do {
dstChunk->copyOverflowInfoFrom(*srcChunk);
......@@ -1607,6 +1607,7 @@ class F14Table : public Policy {
// reset doesn't care about packedBegin, so we don't fix it until the end
if (kEnableItemIteration) {
std::size_t maxChunkIndex = src.lastOccupiedChunk() - src.chunks_;
sizeAndPackedBegin_.packedBegin() =
ItemIter{chunks_ + maxChunkIndex,
chunks_[maxChunkIndex].lastOccupied().index()}
......
......@@ -1797,3 +1797,45 @@ TEST(F14Map, eraseIf) {
///////////////////////////////////
#endif // FOLLY_F14_VECTOR_INTRINSICS_AVAILABLE
///////////////////////////////////
namespace {
template <std::size_t N>
struct DivideBy {
// this is a lie for testing purposes
using folly_is_avalanching = std::true_type;
std::size_t operator()(std::size_t v) const {
return v / N;
}
};
} // namespace
template <template <class...> class TMap>
void testCopyAfterRemovedCollisions() {
// Insert 11 things into chunks 0, 1, and 2, 15 into chunk 3, then
// remove all but the last one from chunk 1 and see if we can find that
// one in a copy of the map.
TMap<std::size_t, bool, DivideBy<16>> map;
map.reserve(48);
for (std::size_t k = 0; k < 11; ++k) {
map[k] = true;
map[k + 16] = true;
map[k + 32] = true;
}
for (std::size_t k = 0; k < 14; ++k) {
map[k + 48] = true;
}
map[14 + 48] = true;
for (std::size_t k = 0; k < 14; ++k) {
map.erase(k + 48);
}
auto copy = map;
EXPECT_EQ(copy.count(14 + 48), 1);
}
TEST(F14Map, copyAfterRemovedCollisions) {
testCopyAfterRemovedCollisions<F14ValueMap>();
testCopyAfterRemovedCollisions<F14VectorMap>();
testCopyAfterRemovedCollisions<F14NodeMap>();
testCopyAfterRemovedCollisions<F14FastMap>();
}
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