diff options
7 files changed, 23 insertions, 68 deletions
diff --git a/searchlib/src/vespa/searchlib/attribute/attributemanager.cpp b/searchlib/src/vespa/searchlib/attribute/attributemanager.cpp index 5aabad6fa02..4b87435dd2e 100644 --- a/searchlib/src/vespa/searchlib/attribute/attributemanager.cpp +++ b/searchlib/src/vespa/searchlib/attribute/attributemanager.cpp @@ -120,16 +120,6 @@ uint64_t AttributeManager::getMemoryFootprint() const return sum; } -bool AttributeManager::hasReaders() const -{ - for(AttributeMap::const_iterator it(_attributes.begin()), mt(_attributes.end()); it != mt; it++) { - if (it->second->hasReaders()) - return true; - } - - return false; -} - const AttributeManager::VectorHolder * AttributeManager::findAndLoadAttribute(const string & name) const { diff --git a/searchlib/src/vespa/searchlib/attribute/attributemanager.h b/searchlib/src/vespa/searchlib/attribute/attributemanager.h index 7e390d5dd19..daa6c725908 100644 --- a/searchlib/src/vespa/searchlib/attribute/attributemanager.h +++ b/searchlib/src/vespa/searchlib/attribute/attributemanager.h @@ -54,7 +54,6 @@ public: const Snapshot & getSnapshot() const { return _snapShot; } const string & getBaseDir() const { return _baseDir; } void setBaseDir(const string & base); - bool hasReaders() const; uint64_t getMemoryFootprint() const; protected: diff --git a/searchlib/src/vespa/searchlib/attribute/attributevector.h b/searchlib/src/vespa/searchlib/attribute/attributevector.h index 8b42b19cc60..5df8a2aa768 100644 --- a/searchlib/src/vespa/searchlib/attribute/attributevector.h +++ b/searchlib/src/vespa/searchlib/attribute/attributevector.h @@ -623,13 +623,6 @@ public: } /** - * Returns true if we might still have readers. False positives - * are possible if writer hasn't updated first used generation - * after last reader left. - */ - bool hasReaders() const { return _genHandler.hasReaders(); } - - /** * Add reserved initial document with docId 0 and undefined value. */ void addReservedDoc(); diff --git a/vespalib/src/tests/util/generationhandler/generationhandler_test.cpp b/vespalib/src/tests/util/generationhandler/generationhandler_test.cpp index 07805a6942c..f269fe729fa 100644 --- a/vespalib/src/tests/util/generationhandler/generationhandler_test.cpp +++ b/vespalib/src/tests/util/generationhandler/generationhandler_test.cpp @@ -97,18 +97,18 @@ Test::requireThatTheFirstUsedGenerationIsCorrect() { GenGuard g1 = gh.takeGuard(); gh.incGeneration(); - EXPECT_EQUAL(true, gh.hasReaders()); + EXPECT_EQUAL(1u, gh.getGenerationRefCount()); EXPECT_EQUAL(1u, gh.getFirstUsedGeneration()); } EXPECT_EQUAL(1u, gh.getFirstUsedGeneration()); gh.updateFirstUsedGeneration(); // Only writer should call this - EXPECT_EQUAL(false, gh.hasReaders()); + EXPECT_EQUAL(0u, gh.getGenerationRefCount()); EXPECT_EQUAL(2u, gh.getFirstUsedGeneration()); { GenGuard g1 = gh.takeGuard(); gh.incGeneration(); gh.incGeneration(); - EXPECT_EQUAL(true, gh.hasReaders()); + EXPECT_EQUAL(1u, gh.getGenerationRefCount()); EXPECT_EQUAL(2u, gh.getFirstUsedGeneration()); { GenGuard g2 = gh.takeGuard(); @@ -117,7 +117,7 @@ Test::requireThatTheFirstUsedGenerationIsCorrect() } EXPECT_EQUAL(2u, gh.getFirstUsedGeneration()); gh.updateFirstUsedGeneration(); // Only writer should call this - EXPECT_EQUAL(false, gh.hasReaders()); + EXPECT_EQUAL(0u, gh.getGenerationRefCount()); EXPECT_EQUAL(4u, gh.getFirstUsedGeneration()); } diff --git a/vespalib/src/tests/util/generationhandler_stress/generation_handler_stress_test.cpp b/vespalib/src/tests/util/generationhandler_stress/generation_handler_stress_test.cpp index 8cea96e0f68..fa2c525b518 100644 --- a/vespalib/src/tests/util/generationhandler_stress/generation_handler_stress_test.cpp +++ b/vespalib/src/tests/util/generationhandler_stress/generation_handler_stress_test.cpp @@ -13,7 +13,7 @@ using vespalib::ThreadStackExecutor; struct WorkContext { - uint64_t _generation; + std::atomic<uint64_t> _generation; WorkContext() noexcept : _generation(0) @@ -84,7 +84,7 @@ Fixture::readWork(const WorkContext &context) for (i = 0; i < cnt && _stopRead.load() == 0; ++i) { auto guard = _generationHandler.takeGuard(); - auto generation = context._generation; + auto generation = context._generation.load(std::memory_order_relaxed); EXPECT_GREATER_EQUAL(generation, guard.getGeneration()); } _doneReadWork += i; @@ -96,7 +96,7 @@ void Fixture::writeWork(uint32_t cnt, WorkContext &context) { for (uint32_t i = 0; i < cnt; ++i) { - context._generation = _generationHandler.getNextGeneration(); + context._generation.store(_generationHandler.getNextGeneration(), std::memory_order_relaxed); _generationHandler.incGeneration(); } _doneWriteWork += cnt; diff --git a/vespalib/src/vespa/vespalib/util/generationhandler.cpp b/vespalib/src/vespa/vespalib/util/generationhandler.cpp index 23e9c9ce8ac..c38ad8431e6 100644 --- a/vespalib/src/vespa/vespalib/util/generationhandler.cpp +++ b/vespalib/src/vespa/vespalib/util/generationhandler.cpp @@ -114,7 +114,7 @@ void GenerationHandler::updateFirstUsedGeneration() { for (;;) { - if (_first == _last) + if (_first == _last.load(std::memory_order_relaxed)) break; // No elements can be freed if (!_first->setInvalid()) { break; // First element still in use @@ -122,9 +122,6 @@ GenerationHandler::updateFirstUsedGeneration() GenerationHold *toFree = _first; assert(toFree->_next != nullptr); _first = toFree->_next; - // Must ensure _first is updated before changing next pointer to - // avoid temporarily inconsistent state (breaks hasReaders()) - std::atomic_thread_fence(std::memory_order_release); toFree->_next = _free; _free = toFree; } @@ -141,14 +138,14 @@ GenerationHandler::GenerationHandler() { _last = _first = new GenerationHold; ++_numHolds; - _last->_generation = _generation; - _last->setValid(); + _first->_generation.store(_generation, std::memory_order_relaxed); + _first->setValid(); } GenerationHandler::~GenerationHandler(void) { updateFirstUsedGeneration(); - assert(_first == _last); + assert(_first == _last.load(std::memory_order_relaxed)); while (_free != nullptr) { GenerationHold *toFree = _free; _free = toFree->_next; @@ -162,17 +159,16 @@ GenerationHandler::~GenerationHandler(void) GenerationHandler::Guard GenerationHandler::takeGuard() const { - Guard guard(_last); + Guard guard(_last.load(std::memory_order_acquire)); for (;;) { // Must check valid() after increasing refcount - std::atomic_thread_fence(std::memory_order_acquire); if (guard.valid()) break; // Might still be marked invalid, that's OK /* * Clashed with writer freeing entry. Must abandon current * guard and try again. */ - guard = Guard(_last); + guard = Guard(_last.load(std::memory_order_acquire)); } // Guard has been valid after bumping refCount return guard; @@ -183,13 +179,12 @@ GenerationHandler::incGeneration() { generation_t ngen = getNextGeneration(); - std::atomic_thread_fence(std::memory_order_seq_cst); - if (_last->getRefCount() == 0) { + auto last = _last.load(std::memory_order_relaxed); + if (last->getRefCount() == 0) { // Last generation is unused, morph it to new generation. This is // the typical case when no readers are present. _generation = ngen; - _last->_generation = ngen; - std::atomic_thread_fence(std::memory_order_release); + last->_generation.store(ngen, std::memory_order_release); updateFirstUsedGeneration(); return; } @@ -201,21 +196,12 @@ GenerationHandler::incGeneration() nhold = _free; _free = nhold->_next; } - nhold->_generation = ngen; + nhold->_generation.store(ngen, std::memory_order_relaxed); nhold->_next = nullptr; nhold->setValid(); - - // new hold must be updated before next pointer is updated - std::atomic_thread_fence(std::memory_order_release); - _last->_next = nhold; - - // next pointer must be updated before _last is updated - std::atomic_thread_fence(std::memory_order_release); + last->_next = nhold; _generation = ngen; - _last = nhold; - - // _last must be updated before _first is changed - std::atomic_thread_fence(std::memory_order_release); + _last.store(nhold, std::memory_order_release); updateFirstUsedGeneration(); } @@ -227,7 +213,7 @@ GenerationHandler::getGenerationRefCount(generation_t gen) const if (static_cast<sgeneration_t>(_firstUsedGeneration - gen) > 0) return 0u; for (GenerationHold *hold = _first; hold != nullptr; hold = hold->_next) { - if (hold->_generation == gen) + if (hold->_generation.load(std::memory_order_relaxed) == gen) return hold->getRefCount(); } return 0u; @@ -243,10 +229,4 @@ GenerationHandler::getGenerationRefCount(void) const return ret; } -bool -GenerationHandler::hasReaders(void) const -{ - return (_first != _last) ? true : (_first->getRefCount() > 0); -} - } diff --git a/vespalib/src/vespa/vespalib/util/generationhandler.h b/vespalib/src/vespa/vespalib/util/generationhandler.h index e03a3d24734..0c4b49a2d5b 100644 --- a/vespalib/src/vespa/vespalib/util/generationhandler.h +++ b/vespalib/src/vespa/vespalib/util/generationhandler.h @@ -30,7 +30,7 @@ public: static bool valid(uint32_t refCount) { return (refCount & 1) == 0u; } public: - generation_t _generation; + std::atomic<generation_t> _generation; GenerationHold *_next; // next free element or next newer element. GenerationHold(); @@ -68,13 +68,13 @@ public: bool valid() const { return _hold != nullptr; } - generation_t getGeneration() const { return _hold->_generation; } + generation_t getGeneration() const { return _hold->_generation.load(std::memory_order_relaxed); } }; private: generation_t _generation; generation_t _firstUsedGeneration; - GenerationHold *_last; // Points to "current generation" entry + std::atomic<GenerationHold *> _last; // Points to "current generation" entry GenerationHold *_first; // Points to "firstUsedGeneration" entry GenerationHold *_free; // List of free entries uint32_t _numHolds; // Number of allocated generation hold entries @@ -134,13 +134,6 @@ public: * Should be called by the writer thread. */ uint64_t getGenerationRefCount() const; - - /** - * Returns true if we still have readers. False positives and - * negatives are possible if readers come and go while writer - * updates generations. - */ - bool hasReaders() const; }; } |