diff options
author | Tor Brede Vekterli <vekterli@oath.com> | 2018-05-29 12:25:09 +0000 |
---|---|---|
committer | Tor Brede Vekterli <vekterli@oath.com> | 2018-07-12 13:30:25 +0000 |
commit | 063c7b3c4308a23b9e76d3831bf1c063168e7a3d (patch) | |
tree | 0b083c5b8ba364e3a342affd205a6a97aa1b56b6 /persistence/src | |
parent | c014337d5c7c11b06d7b29abefb94ccf97f28537 (diff) |
Support concurrent get/iterate/createIterator in dummy persistence
Diffstat (limited to 'persistence/src')
3 files changed, 51 insertions, 30 deletions
diff --git a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp index d3883744229..25c6b71f7ff 100644 --- a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp +++ b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.cpp @@ -528,7 +528,7 @@ DummyPersistence::get(const Bucket& b, b.toString().c_str(), did.toString().c_str()); assert(b.getBucketSpace() == FixedBucketSpaces::default_space()); - BucketContentGuard::UP bc(acquireBucketWithLock(b)); + BucketContentGuard::UP bc(acquireBucketWithLock(b, LockMode::Shared)); if (!bc.get()) { } else { DocEntry::SP entry((*bc)->getEntry(did)); @@ -568,7 +568,7 @@ DummyPersistence::createIterator( "Got invalid/unparseable document selection string"); } } - BucketContentGuard::UP bc(acquireBucketWithLock(b)); + BucketContentGuard::UP bc(acquireBucketWithLock(b, LockMode::Shared)); if (!bc.get()) { return CreateIteratorResult(Result::TRANSIENT_ERROR, "Bucket not found"); } @@ -656,7 +656,7 @@ DummyPersistence::iterate(IteratorId id, uint64_t maxByteSize, Context& ctx) con it = iter->second.get(); } - BucketContentGuard::UP bc(acquireBucketWithLock(it->_bucket)); + BucketContentGuard::UP bc(acquireBucketWithLock(it->_bucket, LockMode::Shared)); if (!bc.get()) { ctx.trace(9, "finished iterate(); bucket not found"); return IterateResult(Result::TRANSIENT_ERROR, "Bucket not found"); @@ -942,11 +942,11 @@ DummyPersistence::isActive(const Bucket& b) const BucketContentGuard::~BucketContentGuard() { - _persistence.releaseBucketNoLock(_content); + _persistence.releaseBucketNoLock(_content, _lock_mode); } BucketContentGuard::UP -DummyPersistence::acquireBucketWithLock(const Bucket& b) const +DummyPersistence::acquireBucketWithLock(const Bucket& b, LockMode lock_mode) const { assert(b.getBucketSpace() == FixedBucketSpaces::default_space()); vespalib::MonitorGuard lock(_monitor); @@ -955,28 +955,32 @@ DummyPersistence::acquireBucketWithLock(const Bucket& b) const if (it == ncp._content[b.getPartition()].end()) { return BucketContentGuard::UP(); } - // Sanity check that SPI-level locking is doing its job correctly. - // Atomic CAS might be a bit overkill, but since we "release" the bucket - // outside of the mutex, we want to ensure the write is visible across all - // threads. - bool my_false(false); - bool bucketNotInUse(it->second->_inUse.compare_exchange_strong(my_false, true)); - if (!bucketNotInUse) { - LOG(error, "Attempted to acquire %s, but it was already marked as being in use!", - b.toString().c_str()); - LOG_ABORT("should not reach here"); + if (lock_mode == LockMode::Exclusive) { + // Sanity check that SPI-level locking is doing its job correctly. + // Atomic CAS might be a bit overkill, but since we "release" the bucket + // outside of the mutex, we want to ensure the write is visible across all + // threads. + bool my_false(false); + bool bucketNotInUse(it->second->_inUse.compare_exchange_strong(my_false, true)); + if (!bucketNotInUse) { + LOG(error, "Attempted to acquire %s, but it was already marked as being in use!", + b.toString().c_str()); + LOG_ABORT("dummy persistence bucket locking invariant violation"); + } } - return BucketContentGuard::UP(new BucketContentGuard(ncp, *it->second)); + return std::make_unique<BucketContentGuard>(ncp, *it->second, lock_mode); } void -DummyPersistence::releaseBucketNoLock(const BucketContent& bc) const +DummyPersistence::releaseBucketNoLock(const BucketContent& bc, LockMode lock_mode) const noexcept { - bool my_true(true); - bool bucketInUse(bc._inUse.compare_exchange_strong(my_true, false)); - assert(bucketInUse); - (void) bucketInUse; + if (lock_mode == LockMode::Exclusive) { + bool my_true(true); + bool bucketInUse(bc._inUse.compare_exchange_strong(my_true, false)); + assert(bucketInUse); + (void) bucketInUse; + } } } diff --git a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.h b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.h index c93b7fd22c7..c97aab822ac 100644 --- a/persistence/src/vespa/persistence/dummyimpl/dummypersistence.h +++ b/persistence/src/vespa/persistence/dummyimpl/dummypersistence.h @@ -24,13 +24,18 @@ class DocumentTypeRepo; namespace storage::spi::dummy { +enum class LockMode { + Exclusive, + Shared +}; + struct BucketEntry { DocEntry::SP entry; GlobalId gid; BucketEntry(DocEntry::SP e, const GlobalId& g) - : entry(e), + : entry(std::move(e)), gid(g) { } }; @@ -98,30 +103,33 @@ class BucketContentGuard BucketContentGuard(const BucketContentGuard&); BucketContentGuard& operator=(const BucketContentGuard&); public: - typedef std::unique_ptr<BucketContentGuard> UP; + using UP = std::unique_ptr<BucketContentGuard>; BucketContentGuard(DummyPersistence& persistence, - BucketContent& content) + BucketContent& content, + LockMode lock_mode) : _persistence(persistence), - _content(content) + _content(content), + _lock_mode(lock_mode) { } ~BucketContentGuard(); - BucketContent& getContent() { + BucketContent& getContent() noexcept { return _content; } - BucketContent* operator->() { + BucketContent* operator->() noexcept { return &_content; } - BucketContent& operator*() { + BucketContent& operator*() noexcept { return _content; } private: DummyPersistence& _persistence; BucketContent& _content; + LockMode _lock_mode; }; class DummyPersistence : public AbstractPersistenceProvider @@ -207,8 +215,8 @@ public: private: friend class BucketContentGuard; // Const since funcs only alter mutable field in BucketContent - BucketContentGuard::UP acquireBucketWithLock(const Bucket& b) const; - void releaseBucketNoLock(const BucketContent& bc) const; + BucketContentGuard::UP acquireBucketWithLock(const Bucket& b, LockMode lock_mode = LockMode::Exclusive) const; + void releaseBucketNoLock(const BucketContent& bc, LockMode lock_mode = LockMode::Exclusive) const noexcept; mutable bool _initialized; std::shared_ptr<const document::DocumentTypeRepo> _repo; diff --git a/persistence/src/vespa/persistence/spi/persistenceprovider.h b/persistence/src/vespa/persistence/spi/persistenceprovider.h index b5f2fc198c4..96b3d385b87 100644 --- a/persistence/src/vespa/persistence/spi/persistenceprovider.h +++ b/persistence/src/vespa/persistence/spi/persistenceprovider.h @@ -232,6 +232,9 @@ struct PersistenceProvider * document id. If no versions were found, or the document was removed, * the result should be successful, but contain no document (see GetResult). * + * Concurrency note: may be called concurrently with other read-only + * operations. + * * @param fieldSet A set of fields that should be retrieved. * @param id The document id to retrieve. */ @@ -253,6 +256,9 @@ struct PersistenceProvider * iteration progress and selection criteria. destroyIterator will NOT * be called when createIterator returns an error. * + * Concurrency note: may be called concurrently with other read-only + * operations. + * * @param selection Selection criteria used to limit the subset of * the bucket's documents that will be returned by the iterator. The * provider implementation may use these criteria to optimize its @@ -323,6 +329,9 @@ struct PersistenceProvider * iterator must only set this flag on the result and return without any * documents. * + * Concurrency note: may be called concurrently with other read-only + * operations. + * * @param id An iterator ID returned by a previous call to createIterator * @param maxByteSize An indication of the maximum number of bytes that * should be returned. |