diff --git a/src/data_structures/concurrent/common.hpp b/src/data_structures/concurrent/common.hpp index 633c69cc6..feec09a33 100644 --- a/src/data_structures/concurrent/common.hpp +++ b/src/data_structures/concurrent/common.hpp @@ -75,5 +75,5 @@ class AccessorBase { size_t size() const { return accessor.size(); } protected: - typename list::Accessor accessor; + typename list::template Accessor> accessor; }; diff --git a/src/data_structures/concurrent/skiplist.hpp b/src/data_structures/concurrent/skiplist.hpp index 9a0cb1c4d..39b3edcdd 100644 --- a/src/data_structures/concurrent/skiplist.hpp +++ b/src/data_structures/concurrent/skiplist.hpp @@ -3,6 +3,7 @@ #include #include #include +#include #include "utils/assert.hpp" #include "utils/crtp.hpp" @@ -451,10 +452,11 @@ class SkipList : private Lockable { friend class Accessor; + template class Accessor { friend class SkipList; - Accessor(SkipList *skiplist) + Accessor(TSkipList *skiplist) : skiplist(skiplist), status_(skiplist->gc.CreateNewAccessor()) { debug_assert(skiplist != nullptr, "Skiplist is nullptr."); } @@ -473,15 +475,37 @@ class SkipList : private Lockable { status_.alive_ = false; } - Iterator begin() { return skiplist->begin(); } + // TODO(dgleich): Remove after C++17 compile flag. + template + struct negation : std::integral_constant {}; - ConstIterator begin() const { return skiplist->cbegin(); } + template + Iterator begin(typename std::enable_if< + negation>::value>::type * = 0) { + return skiplist->begin(); + } + + template + ConstIterator begin( + typename std::enable_if::value>::type * = + 0) const { + return skiplist->cbegin(); + } ConstIterator cbegin() const { return skiplist->cbegin(); } - Iterator end() { return skiplist->end(); } + template + Iterator end(typename std::enable_if< + negation>::value>::type * = 0) { + return skiplist->end(); + } - ConstIterator end() const { return skiplist->cend(); } + template + ConstIterator end( + typename std::enable_if::value>::type * = + 0) const { + return skiplist->cend(); + } ConstIterator cend() const { return skiplist->cend(); } @@ -517,25 +541,25 @@ class SkipList : private Lockable { return skiplist->reverse(item); } - /** - * Returns an iterator pointing to the element equal to - * item, or the first larger element. - * - * @param item An item that is comparable to skiplist element type. - * @tparam TItem item type - */ + /** + * Returns an iterator pointing to the element equal to + * item, or the first larger element. + * + * @param item An item that is comparable to skiplist element type. + * @tparam TItem item type + */ template Iterator find_or_larger(const TItem &item) { - return skiplist->find_or_larger(item); + return skiplist->template find_or_larger(item); } - /** - * Returns an iterator pointing to the element equal to - * item, or the first larger element. - * - * @param item An item that is comparable to skiplist element type. - * @tparam TItem item type - */ + /** + * Returns an iterator pointing to the element equal to + * item, or the first larger element. + * + * @param item An item that is comparable to skiplist element type. + * @tparam TItem item type + */ template ConstIterator find_or_larger(const TItem &item) const { return static_cast(*skiplist) @@ -621,7 +645,7 @@ class SkipList : private Lockable { // now we need to estimate the count of elements equal to item // we'll do that by looking for the first element that is greater // than item, and counting how far we have to look - + // first find the rightmost (highest) succ that has value == item int count_level = 0; for (int i = position_level; i >= 0; i--) @@ -669,14 +693,16 @@ class SkipList : private Lockable { } private: - SkipList *skiplist; + TSkipList *skiplist; Node *preds[H], *succs[H]; typename SkipListGC::AccessorStatus &status_; }; - Accessor access() { return Accessor(this); } + Accessor access() { return Accessor(this); } - const Accessor access() const { return Accessor(this); } + Accessor caccess() const { + return Accessor(this); + } private: using guard_t = std::unique_lock; @@ -1022,5 +1048,5 @@ class SkipList : private Lockable { */ std::atomic count{0}; Node *header; - SkipListGC gc; + mutable SkipListGC gc; }; diff --git a/src/database/indexes/index_common.hpp b/src/database/indexes/index_common.hpp index 08440e90b..65120cfab 100644 --- a/src/database/indexes/index_common.hpp +++ b/src/database/indexes/index_common.hpp @@ -17,7 +17,7 @@ namespace IndexUtils { * iterating, i.e. it allows us to iterate over the suffix of some skiplist * hence the name SkipListSuffix. */ -template +template class SkipListSuffix { public: class Iterator { @@ -39,15 +39,16 @@ class SkipListSuffix { TIterator current_; }; - SkipListSuffix(const TIterator begin, - typename SkipList::Accessor &&accessor) + SkipListSuffix( + const TIterator begin, + typename SkipList::template Accessor &&accessor) : begin_(begin), accessor_(std::move(accessor)) {} Iterator begin() const { return Iterator(begin_); } Iterator end() { return Iterator(accessor_.end()); } TIterator begin_; - typename SkipList::Accessor accessor_; + typename SkipList::template Accessor accessor_; }; /** @@ -67,21 +68,24 @@ class SkipListSuffix { * ignored). * @Tparam TIndexEntry - index entry inside skiplist * @Tparam TRecord - type of record under index (edge/vertex usually.) + * @Tparam TAccessor - type of accessor to use (const skiplist/non const + * skiplist). * @return iterable collection of distinct vlist records for which * exists function evaluates as true */ -template +template static auto GetVlists( - typename SkipList::Accessor &&skiplist_accessor, + typename SkipList::template Accessor + &&skiplist_accessor, TIterator begin, const std::function predicate, const tx::Transaction &t, const std::function exists, bool current_state = false) { TIndexEntry *prev = nullptr; - auto range = - iter::takewhile(predicate, SkipListSuffix( - begin, std::move(skiplist_accessor))); + auto range = iter::takewhile( + predicate, SkipListSuffix( + begin, std::move(skiplist_accessor))); auto filtered = iter::filter( [&t, exists, prev, current_state](TIndexEntry &entry) mutable { // Check if the current entry could offer new possible return value @@ -176,4 +180,4 @@ static void Refresh( } } } -}; // IndexUtils +}; // namespace IndexUtils diff --git a/src/database/indexes/label_property_index.hpp b/src/database/indexes/label_property_index.hpp index 008df6ace..f6c950444 100644 --- a/src/database/indexes/label_property_index.hpp +++ b/src/database/indexes/label_property_index.hpp @@ -86,7 +86,7 @@ class LabelPropertyIndex { * be populated with, and can be used from this moment forward without missing * any records. * @param key - index which finished being populated. - */ + */ void IndexFinishedBuilding(const Key &key) { ready_for_use_.access().insert(key); } @@ -172,7 +172,7 @@ class LabelPropertyIndex { auto access = GetKeyStorage(key)->access(); auto begin = access.begin(); return IndexUtils::GetVlists::Iterator, - IndexEntry, Vertex>( + IndexEntry, Vertex, SkipList>( std::move(access), begin, [](const IndexEntry &) { return true; }, t, [key](const IndexEntry &entry, const Vertex *const vertex) { return LabelPropertyIndex::Exists(key, entry.value_, vertex); @@ -269,10 +269,9 @@ class LabelPropertyIndex { auto access = GetKeyStorage(key)->access(); // create the iterator startpoint based on the lower bound - auto start_iter = lower - ? access.find_or_larger(make_index_bound( - lower, lower.value().IsInclusive())) - : access.begin(); + auto start_iter = lower ? access.find_or_larger(make_index_bound( + lower, lower.value().IsInclusive())) + : access.begin(); // a function that defines if an entry staisfies the filtering predicate. // since we already handled the lower bound, we only need to deal with the @@ -394,8 +393,7 @@ class LabelPropertyIndex { */ std::vector Keys() { std::vector keys; - for (auto &kv : indices_.access()) - keys.push_back(kv.first); + for (auto &kv : indices_.access()) keys.push_back(kv.first); return keys; } diff --git a/tests/unit/skiplist_access.cpp b/tests/unit/skiplist_access.cpp new file mode 100644 index 000000000..3482b8e0f --- /dev/null +++ b/tests/unit/skiplist_access.cpp @@ -0,0 +1,47 @@ +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +#include + +#include "data_structures/concurrent/skiplist.hpp" + +TEST(SkipList, Access) { + SkipList input; + { + auto accessor = input.access(); + accessor.insert(1); + accessor.insert(2); + accessor.insert(3); + } + + auto accessor = input.access(); + std::vector results; + for (auto it = accessor.begin(); it != accessor.end(); ++it) + results.push_back(*it); + + EXPECT_THAT(results, testing::ElementsAre(1, 2, 3)); +} + +TEST(SkipList, ConstAccess) { + SkipList input; + { + auto accessor = input.access(); + accessor.insert(1); + accessor.insert(2); + accessor.insert(3); + } + + const SkipList &skiplist = input; + auto accessor = skiplist.caccess(); + + std::vector results; + for (auto it = accessor.begin(); it != accessor.end(); ++it) + results.push_back(*it); + + EXPECT_THAT(results, testing::ElementsAre(1, 2, 3)); +} + +int main(int argc, char **argv) { + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} diff --git a/tests/unit/skiplist_suffix.cpp b/tests/unit/skiplist_suffix.cpp index c6131ed20..649a98208 100644 --- a/tests/unit/skiplist_suffix.cpp +++ b/tests/unit/skiplist_suffix.cpp @@ -17,8 +17,9 @@ int Count(TIterable &collection) { TEST(SkipListSuffix, EmptyRange) { SkipList V; auto access = V.access(); - auto r1 = IndexUtils::SkipListSuffix::Iterator, int>( - access.begin(), std::move(access)); + auto r1 = IndexUtils::SkipListSuffix::Iterator, int, + SkipList>(access.begin(), + std::move(access)); EXPECT_EQ(Count(r1), 0); } @@ -28,8 +29,9 @@ TEST(SkipListSuffix, NonEmptyRange) { access.insert(1); access.insert(5); access.insert(3); - auto r1 = IndexUtils::SkipListSuffix::Iterator, int>( - access.begin(), std::move(access)); + auto r1 = IndexUtils::SkipListSuffix::Iterator, int, + SkipList>(access.begin(), + std::move(access)); EXPECT_EQ(Count(r1), 3); auto iter = r1.begin(); EXPECT_EQ(*iter, 1);