From df1640de083b7b6f4a28f4b2ac7d5dd10cbb589b Mon Sep 17 00:00:00 2001 From: Henning Baldersheim Date: Tue, 20 Apr 2021 10:03:20 +0000 Subject: - Signal that we are using the default comaparator with no additional or mutable state. - Separate refering the stateless comparator, and creating new statefull comparators. - Keep the Comparator creation close. --- .../enum_comparator/enum_comparator_test.cpp | 17 ++++---- .../tests/attribute/enumstore/enumstore_test.cpp | 2 +- .../attribute/posting_store/posting_store_test.cpp | 4 +- .../src/vespa/searchlib/attribute/enumstore.cpp | 8 ++-- .../src/vespa/searchlib/attribute/enumstore.h | 25 ++++++------ .../src/vespa/searchlib/attribute/enumstore.hpp | 45 ++++++++++++++++------ .../attribute/multinumericpostattribute.hpp | 2 +- .../attribute/multistringpostattribute.hpp | 2 +- .../searchlib/attribute/postinglistattribute.cpp | 4 +- .../attribute/singlenumericpostattribute.h | 2 +- .../attribute/singlenumericpostattribute.hpp | 9 ++--- .../attribute/singlestringpostattribute.hpp | 2 +- 12 files changed, 69 insertions(+), 53 deletions(-) diff --git a/searchlib/src/tests/attribute/enum_comparator/enum_comparator_test.cpp b/searchlib/src/tests/attribute/enum_comparator/enum_comparator_test.cpp index a9efc22dc5a..3ab29797120 100644 --- a/searchlib/src/tests/attribute/enum_comparator/enum_comparator_test.cpp +++ b/searchlib/src/tests/attribute/enum_comparator/enum_comparator_test.cpp @@ -30,7 +30,7 @@ TEST("requireThatNumericLessIsWorking") NumericEnumStore es(false, DictionaryConfig::Type::BTREE); EnumIndex e1 = es.insert(10); EnumIndex e2 = es.insert(30); - auto cmp1 = es.make_comparator(); + const auto & cmp1 = es.get_comparator(); EXPECT_TRUE(cmp1.less(e1, e2)); EXPECT_FALSE(cmp1.less(e2, e1)); EXPECT_FALSE(cmp1.less(e1, e1)); @@ -44,7 +44,7 @@ TEST("requireThatNumericEqualIsWorking") NumericEnumStore es(false, DictionaryConfig::Type::BTREE); EnumIndex e1 = es.insert(10); EnumIndex e2 = es.insert(30); - auto cmp1 = es.make_comparator(); + const auto & cmp1 = es.get_comparator(); EXPECT_FALSE(cmp1.equal(e1, e2)); EXPECT_FALSE(cmp1.equal(e2, e1)); EXPECT_TRUE(cmp1.equal(e1, e1)); @@ -60,7 +60,7 @@ TEST("requireThatFloatLessIsWorking") EnumIndex e1 = es.insert(10.5); EnumIndex e2 = es.insert(30.5); EnumIndex e3 = es.insert(std::numeric_limits::quiet_NaN()); - auto cmp1 = es.make_comparator(); + const auto & cmp1 = es.get_comparator(); EXPECT_TRUE(cmp1.less(e1, e2)); EXPECT_FALSE(cmp1.less(e2, e1)); EXPECT_FALSE(cmp1.less(e1, e1)); @@ -78,7 +78,7 @@ TEST("requireThatFloatEqualIsWorking") EnumIndex e1 = es.insert(10.5); EnumIndex e2 = es.insert(30.5); EnumIndex e3 = es.insert(std::numeric_limits::quiet_NaN()); - auto cmp1 = es.make_comparator(); + const auto & cmp1 = es.get_comparator(); EXPECT_FALSE(cmp1.equal(e1, e2)); EXPECT_FALSE(cmp1.equal(e2, e1)); EXPECT_TRUE(cmp1.equal(e1, e1)); @@ -97,7 +97,7 @@ TEST("requireThatStringLessIsWorking") EnumIndex e1 = es.insert("Aa"); EnumIndex e2 = es.insert("aa"); EnumIndex e3 = es.insert("aB"); - auto cmp1 = es.make_comparator(); + const auto & cmp1 = es.get_comparator(); EXPECT_TRUE(cmp1.less(e1, e2)); // similar folded, fallback to regular EXPECT_FALSE(cmp1.less(e2, e1)); EXPECT_FALSE(cmp1.less(e1, e1)); @@ -114,7 +114,7 @@ TEST("requireThatStringEqualIsWorking") EnumIndex e1 = es.insert("Aa"); EnumIndex e2 = es.insert("aa"); EnumIndex e3 = es.insert("aB"); - auto cmp1 = es.make_comparator(); + const auto & cmp1 = es.get_comparator(); EXPECT_FALSE(cmp1.equal(e1, e2)); // similar folded, fallback to regular EXPECT_FALSE(cmp1.equal(e2, e1)); EXPECT_TRUE(cmp1.equal(e1, e1)); @@ -157,7 +157,7 @@ TEST("requireThatFoldedLessIsWorking") EnumIndex e2 = es.insert("aa"); EnumIndex e3 = es.insert("aB"); EnumIndex e4 = es.insert("Folded"); - auto cmp1 = es.make_folded_comparator(); + const auto & cmp1 = es.get_folded_comparator(); EXPECT_FALSE(cmp1.less(e1, e2)); // similar folded EXPECT_FALSE(cmp1.less(e2, e1)); // similar folded EXPECT_TRUE(cmp1.less(e2, e3)); // folded compare @@ -177,7 +177,7 @@ TEST("requireThatFoldedEqualIsWorking") EnumIndex e2 = es.insert("aa"); EnumIndex e3 = es.insert("aB"); EnumIndex e4 = es.insert("Folded"); - auto cmp1 = es.make_folded_comparator(); + const auto & cmp1 = es.get_folded_comparator(); EXPECT_TRUE(cmp1.equal(e1, e1)); // similar folded EXPECT_TRUE(cmp1.equal(e2, e1)); // similar folded EXPECT_TRUE(cmp1.equal(e2, e1)); @@ -191,7 +191,6 @@ TEST("requireThatFoldedEqualIsWorking") EXPECT_FALSE(cmp3.equal(EnumIndex(), e4)); // similar when prefix EXPECT_FALSE(cmp3.equal(e4, EnumIndex())); // similar when prefix EXPECT_TRUE(cmp3.equal(EnumIndex(), EnumIndex())); // similar when prefix - } } diff --git a/searchlib/src/tests/attribute/enumstore/enumstore_test.cpp b/searchlib/src/tests/attribute/enumstore/enumstore_test.cpp index 65d7fc4dcc1..ada39f9d2de 100644 --- a/searchlib/src/tests/attribute/enumstore/enumstore_test.cpp +++ b/searchlib/src/tests/attribute/enumstore/enumstore_test.cpp @@ -606,7 +606,7 @@ EnumStoreDictionaryTest::update_posting_idx(Enum { auto& dict = store.get_dictionary(); EntryRef old_posting_idx_check; - dict.update_posting_list(enum_idx, store.make_comparator(), [&old_posting_idx_check, new_posting_idx](EntryRef posting_idx) noexcept -> EntryRef { old_posting_idx_check = posting_idx; return new_posting_idx; }); + dict.update_posting_list(enum_idx, store.get_comparator(), [&old_posting_idx_check, new_posting_idx](EntryRef posting_idx) noexcept -> EntryRef { old_posting_idx_check = posting_idx; return new_posting_idx; }); EXPECT_EQ(old_posting_idx, old_posting_idx_check); } diff --git a/searchlib/src/tests/attribute/posting_store/posting_store_test.cpp b/searchlib/src/tests/attribute/posting_store/posting_store_test.cpp index ebaab3b4085..a502dbcaf79 100644 --- a/searchlib/src/tests/attribute/posting_store/posting_store_test.cpp +++ b/searchlib/src/tests/attribute/posting_store/posting_store_test.cpp @@ -132,8 +132,8 @@ PostingStoreTest::populate(uint32_t sequence_length) for (int i = 0; i < 9000; ++i) { refs.emplace_back(add_sequence(i + 6, i + 6 + sequence_length)); } - dictionary.update_posting_list(_value_store.insert(1), _value_store.make_comparator(), [this, sequence_length](EntryRef) { return add_sequence(4, 4 + sequence_length); }); - dictionary.update_posting_list(_value_store.insert(2), _value_store.make_comparator(), [this, sequence_length](EntryRef) { return add_sequence(5, 5 + sequence_length); }); + dictionary.update_posting_list(_value_store.insert(1), _value_store.get_comparator(), [this, sequence_length](EntryRef) { return add_sequence(4, 4 + sequence_length); }); + dictionary.update_posting_list(_value_store.insert(2), _value_store.get_comparator(), [this, sequence_length](EntryRef) { return add_sequence(5, 5 + sequence_length); }); for (int i = 9000; i < 11000; ++i) { refs.emplace_back(add_sequence(i + 6, i + 6 + sequence_length)); } diff --git a/searchlib/src/vespa/searchlib/attribute/enumstore.cpp b/searchlib/src/vespa/searchlib/attribute/enumstore.cpp index 3a008557676..9da3906ac85 100644 --- a/searchlib/src/vespa/searchlib/attribute/enumstore.cpp +++ b/searchlib/src/vespa/searchlib/attribute/enumstore.cpp @@ -21,9 +21,7 @@ EnumStoreT::write_value(BufferWriter& writer, Index idx) const template <> ssize_t -EnumStoreT::load_unique_value(const void* src, - size_t available, - Index& idx) +EnumStoreT::load_unique_value(const void* src, size_t available, Index& idx) { const char* value = static_cast(src); size_t slen = strlen(value); @@ -43,8 +41,8 @@ EnumStoreT::load_unique_value(const void* src, std::unique_ptr make_enum_store_dictionary(IEnumStore &store, bool has_postings, const search::DictionaryConfig & dict_cfg, - std::unique_ptr compare, - std::unique_ptr folded_compare) + std::unique_ptr compare, + std::unique_ptr folded_compare) { using NoBTreeDictionary = vespalib::datastore::NoBTreeDictionary; using ShardedHashMap = vespalib::datastore::ShardedHashMap; diff --git a/searchlib/src/vespa/searchlib/attribute/enumstore.h b/searchlib/src/vespa/searchlib/attribute/enumstore.h index 3d4f6d3e888..618faf84d9b 100644 --- a/searchlib/src/vespa/searchlib/attribute/enumstore.h +++ b/searchlib/src/vespa/searchlib/attribute/enumstore.h @@ -47,11 +47,14 @@ public: using EntryType = EntryT; using EnumStoreType = EnumStoreT; using EntryRef = vespalib::datastore::EntryRef; + using EntryComparator = vespalib::datastore::EntryComparator; using generation_t = vespalib::GenerationHandler::generation_t; private: UniqueStoreType _store; IEnumStoreDictionary* _dict; + ComparatorType _comparator; + ComparatorType _foldedComparator; vespalib::MemoryUsage _cached_values_memory_usage; vespalib::AddressSpace _cached_values_address_space_usage; vespalib::MemoryUsage _cached_dictionary_btree_usage; @@ -73,6 +76,8 @@ private: ssize_t load_unique_values_internal(const void* src, size_t available, IndexVector& idx); ssize_t load_unique_value(const void* src, size_t available, Index& idx); + std::unique_ptr allocate_optionally_folded_comparator(bool folded) const; + ComparatorType make_optionally_folded_comparator(bool folded) const; public: EnumStoreT(bool has_postings, const search::DictionaryConfig & dict_cfg); ~EnumStoreT() override; @@ -174,16 +179,16 @@ public: return BatchUpdater(*this); } - ComparatorType make_comparator() const { - return ComparatorType(_store.get_data_store()); + const EntryComparator & get_comparator() const { + return _comparator; } ComparatorType make_comparator(const EntryType& fallback_value) const { return ComparatorType(_store.get_data_store(), fallback_value); } - ComparatorType make_folded_comparator() const { - return ComparatorType(_store.get_data_store(), true); + const EntryComparator & get_folded_comparator() const { + return _foldedComparator; } void write_value(BufferWriter& writer, Index idx) const override; @@ -204,7 +209,7 @@ public: _store.get_allocator().get_data_store().inc_compaction_count(); } std::unique_ptr make_enumerator() const override; - std::unique_ptr allocate_comparator() const override; + std::unique_ptr allocate_comparator() const override; // Methods below are only relevant for strings, and are templated to only be instantiated on demand. template @@ -225,21 +230,13 @@ public: } }; -std::unique_ptr -make_enum_store_dictionary(IEnumStore &store, bool has_postings, const search::DictionaryConfig & dict_cfg, - std::unique_ptr compare, - std::unique_ptr folded_compare); - - template <> void EnumStoreT::write_value(BufferWriter& writer, Index idx) const; template <> ssize_t -EnumStoreT::load_unique_value(const void* src, - size_t available, - Index& idx); +EnumStoreT::load_unique_value(const void* src, size_t available, Index& idx); } diff --git a/searchlib/src/vespa/searchlib/attribute/enumstore.hpp b/searchlib/src/vespa/searchlib/attribute/enumstore.hpp index de5973dd4a1..ef9362174de 100644 --- a/searchlib/src/vespa/searchlib/attribute/enumstore.hpp +++ b/searchlib/src/vespa/searchlib/attribute/enumstore.hpp @@ -22,6 +22,13 @@ namespace search { +using vespalib::datastore::EntryComparator; + +std::unique_ptr +make_enum_store_dictionary(IEnumStore &store, bool has_postings, const search::DictionaryConfig & dict_cfg, + std::unique_ptr compare, + std::unique_ptr folded_compare); + template void EnumStoreT::free_value_if_unused(Index idx, IndexSet& unused) { @@ -34,9 +41,7 @@ void EnumStoreT::free_value_if_unused(Index idx, IndexSet& unused) template ssize_t -EnumStoreT::load_unique_values_internal(const void* src, - size_t available, - IndexVector& idx) +EnumStoreT::load_unique_values_internal(const void* src, size_t available, IndexVector& idx) { size_t left = available; const char* p = static_cast(src); @@ -69,14 +74,14 @@ template EnumStoreT::EnumStoreT(bool has_postings, const search::DictionaryConfig & dict_cfg) : _store(), _dict(), + _comparator(_store.get_data_store()), + _foldedComparator(make_optionally_folded_comparator(true)), _cached_values_memory_usage(), _cached_values_address_space_usage(0, 0, (1ull << 32)) { _store.set_dictionary(make_enum_store_dictionary(*this, has_postings, dict_cfg, - std::make_unique(_store.get_data_store()), - (has_string_type() ? - std::make_unique(_store.get_data_store(), true) : - std::unique_ptr()))); + allocate_comparator(), + allocate_optionally_folded_comparator(true))); _dict = static_cast(&_store.get_dictionary()); } @@ -150,7 +155,7 @@ template bool EnumStoreT::is_folded_change(Index idx1, Index idx2) const { - auto cmp = make_folded_comparator(); + const auto & cmp = get_folded_comparator(); assert(!cmp.less(idx2, idx1)); return cmp.less(idx1, idx2); } @@ -180,14 +185,14 @@ template void EnumStoreT::free_unused_values() { - _dict->free_unused_values(make_comparator()); + _dict->free_unused_values(get_comparator()); } template void EnumStoreT::free_unused_values(const IndexSet& to_remove) { - _dict->free_unused_values(to_remove, make_comparator()); + _dict->free_unused_values(to_remove, get_comparator()); } template @@ -265,10 +270,28 @@ EnumStoreT::make_enumerator() const } template -std::unique_ptr +std::unique_ptr EnumStoreT::allocate_comparator() const { return std::make_unique(_store.get_data_store()); } +template +std::unique_ptr +EnumStoreT::allocate_optionally_folded_comparator(bool folded) const +{ + return (has_string_type() && folded) + ? std::make_unique(_store.get_data_store(), true) + : std::unique_ptr(); +} + +template +EnumStoreT::ComparatorType +EnumStoreT::make_optionally_folded_comparator(bool folded) const +{ + return (has_string_type() && folded) + ? ComparatorType(_store.get_data_store(), true) + : ComparatorType(_store.get_data_store()); +} + } diff --git a/searchlib/src/vespa/searchlib/attribute/multinumericpostattribute.hpp b/searchlib/src/vespa/searchlib/attribute/multinumericpostattribute.hpp index 5b695c5751b..e3fbed1ecc2 100644 --- a/searchlib/src/vespa/searchlib/attribute/multinumericpostattribute.hpp +++ b/searchlib/src/vespa/searchlib/attribute/multinumericpostattribute.hpp @@ -30,7 +30,7 @@ MultiValueNumericPostingAttribute::applyValueChanges(const DocIndices& doc EnumIndexMapper mapper; PostingMap changePost(PostingChangeComputer::compute(this->getMultiValueMapping(), docIndices, - this->getEnumStore().make_comparator(), mapper)); + this->getEnumStore().get_comparator(), mapper)); this->updatePostings(changePost); MultiValueNumericEnumAttribute::applyValueChanges(docIndices, updater); } diff --git a/searchlib/src/vespa/searchlib/attribute/multistringpostattribute.hpp b/searchlib/src/vespa/searchlib/attribute/multistringpostattribute.hpp index d5064746cc2..381d5b6339b 100644 --- a/searchlib/src/vespa/searchlib/attribute/multistringpostattribute.hpp +++ b/searchlib/src/vespa/searchlib/attribute/multistringpostattribute.hpp @@ -47,7 +47,7 @@ applyValueChanges(const DocIndices& docIndices, EnumStoreBatchUpdater &updater) StringEnumIndexMapper mapper(dictionary); PostingMap changePost(PostingChangeComputer::compute(this->getMultiValueMapping(), docIndices, - enumStore.make_folded_comparator(), mapper)); + enumStore.get_folded_comparator(), mapper)); this->updatePostings(changePost); MultiValueStringAttributeT::applyValueChanges(docIndices, updater); } diff --git a/searchlib/src/vespa/searchlib/attribute/postinglistattribute.cpp b/searchlib/src/vespa/searchlib/attribute/postinglistattribute.cpp index 9a431be5a02..e988091db45 100644 --- a/searchlib/src/vespa/searchlib/attribute/postinglistattribute.cpp +++ b/searchlib/src/vespa/searchlib/attribute/postinglistattribute.cpp @@ -281,7 +281,7 @@ void PostingListAttributeSubBase:: updatePostings(PostingMap &changePost) { - updatePostings(changePost, _es.make_folded_comparator()); + updatePostings(changePost, _es.get_folded_comparator()); } @@ -291,7 +291,7 @@ void PostingListAttributeSubBase:: clearPostings(attribute::IAttributeVector::EnumHandle eidx, uint32_t fromLid, uint32_t toLid) { - clearPostings(eidx, fromLid, toLid, _es.make_folded_comparator()); + clearPostings(eidx, fromLid, toLid, _es.get_folded_comparator()); } diff --git a/searchlib/src/vespa/searchlib/attribute/singlenumericpostattribute.h b/searchlib/src/vespa/searchlib/attribute/singlenumericpostattribute.h index 634ac7ba024..4e7f8040f7f 100644 --- a/searchlib/src/vespa/searchlib/attribute/singlenumericpostattribute.h +++ b/searchlib/src/vespa/searchlib/attribute/singlenumericpostattribute.h @@ -63,7 +63,7 @@ private: void mergeMemoryStats(vespalib::MemoryUsage & total) override; void applyUpdateValueChange(const Change & c, EnumStore & enumStore, std::map & currEnumIndices); - void makePostingChange(const vespalib::datastore::EntryComparator *cmp, + void makePostingChange(const vespalib::datastore::EntryComparator &cmp, const std::map &currEnumIndices, PostingMap &changePost); diff --git a/searchlib/src/vespa/searchlib/attribute/singlenumericpostattribute.hpp b/searchlib/src/vespa/searchlib/attribute/singlenumericpostattribute.hpp index 29c4927efd4..f5ab855565c 100644 --- a/searchlib/src/vespa/searchlib/attribute/singlenumericpostattribute.hpp +++ b/searchlib/src/vespa/searchlib/attribute/singlenumericpostattribute.hpp @@ -53,7 +53,7 @@ SingleValueNumericPostingAttribute::applyUpdateValueChange(const Change & c, template void SingleValueNumericPostingAttribute:: -makePostingChange(const vespalib::datastore::EntryComparator *cmpa, +makePostingChange(const vespalib::datastore::EntryComparator &cmpa, const std::map &currEnumIndices, PostingMap &changePost) { @@ -63,11 +63,11 @@ makePostingChange(const vespalib::datastore::EntryComparator *cmpa, EnumIndex newIdx = elem.second; // add new posting - changePost[EnumPostingPair(newIdx, cmpa)].add(docId, 1); + changePost[EnumPostingPair(newIdx, &cmpa)].add(docId, 1); // remove old posting if ( oldIdx.valid()) { - changePost[EnumPostingPair(oldIdx, cmpa)].remove(docId); + changePost[EnumPostingPair(oldIdx, &cmpa)].remove(docId); } } } @@ -79,7 +79,6 @@ SingleValueNumericPostingAttribute::applyValueChanges(EnumStoreBatchUpdater& { EnumStore & enumStore = this->getEnumStore(); IEnumStoreDictionary& dictionary = enumStore.get_dictionary(); - auto cmp = enumStore.make_comparator(); PostingMap changePost; // used to make sure several arithmetic operations on the same document in a single commit works @@ -110,7 +109,7 @@ SingleValueNumericPostingAttribute::applyValueChanges(EnumStoreBatchUpdater& } } - makePostingChange(&cmp, currEnumIndices, changePost); + makePostingChange(enumStore.get_comparator(), currEnumIndices, changePost); this->updatePostings(changePost); SingleValueNumericEnumAttribute::applyValueChanges(updater); diff --git a/searchlib/src/vespa/searchlib/attribute/singlestringpostattribute.hpp b/searchlib/src/vespa/searchlib/attribute/singlestringpostattribute.hpp index df6982660f8..39ad8d71021 100644 --- a/searchlib/src/vespa/searchlib/attribute/singlestringpostattribute.hpp +++ b/searchlib/src/vespa/searchlib/attribute/singlestringpostattribute.hpp @@ -101,7 +101,7 @@ SingleValueStringPostingAttributeT::applyValueChanges(EnumStoreBatchUpdater& } } - makePostingChange(enumStore.make_folded_comparator(), dictionary, currEnumIndices, changePost); + makePostingChange(enumStore.get_folded_comparator(), dictionary, currEnumIndices, changePost); this->updatePostings(changePost); -- cgit v1.2.3