Merge pull request #14013 from vespa-engine/balder/do-not-compute-hash-on-temporaries

Do not compute the hash on temporary sets. Wait till done.

7 files changed, 34 insertions, 51 deletions

diff --git a/document/src/tests/fieldsettest.cpp b/document/src/tests/fieldsettest.cpp
index 2642bbe23ce..37c55385c0f 100644
--- a/document/src/tests/fieldsettest.cpp
+++ b/document/src/tests/fieldsettest.cpp
@@ -289,23 +289,14 @@ TEST(FieldCollectionTest, testHash ) {
     TestDocMan testDocMan;
     const DocumentTypeRepo& repo = testDocMan.getTypeRepo();
     const DocumentType & type = *repo.getDocumentType("testdoctype1");
-    FieldCollection fc(type);
-    EXPECT_EQ(0ul, fc.hash());
-    fc.insert(type.getField("headerval"));
-    EXPECT_EQ(0x548599858c77ef83ul, fc.hash());
-    fc.insert(type.getField("hstringval"));
-    EXPECT_EQ(0x4a7ff2406d36a9b0ul, fc.hash());
-    fc.insert(type.getField("headerval"));
-    EXPECT_EQ(0x4a7ff2406d36a9b0ul, fc.hash());
-
-    FieldCollection fc2(type);
-    EXPECT_EQ(0ul, fc2.hash());
-    fc2.insert(type.getField("hstringval"));
-    EXPECT_EQ(0x1e0918531b19734ul, fc2.hash());
-    fc2.insert(type.getField("headerval"));
-    EXPECT_EQ(fc.hash(), fc2.hash());
-    fc2.insert(type.getField("headerval"));
-    EXPECT_EQ(fc.hash(), fc2.hash());
+    Field::Set set;
+    EXPECT_EQ(0ul, FieldCollection(type, set).hash());
+    set.insert(&type.getField("headerval"));
+    EXPECT_EQ(0x548599858c77ef83ul, FieldCollection(type, set).hash());
+    set.insert(&type.getField("hstringval"));
+    EXPECT_EQ(0x4a7ff2406d36a9b0ul, FieldCollection(type, set).hash());
+    set.erase(&type.getField("headerval"));
+    EXPECT_EQ(0x1e0918531b19734ul, FieldCollection(type, set).hash());
 }
 
 } // document
diff --git a/document/src/vespa/document/fieldset/fieldsetrepo.cpp b/document/src/vespa/document/fieldset/fieldsetrepo.cpp
index 614a8740f90..c576624b026 100644
--- a/document/src/vespa/document/fieldset/fieldsetrepo.cpp
+++ b/document/src/vespa/document/fieldset/fieldsetrepo.cpp
@@ -44,20 +44,18 @@ parseFieldCollection(const DocumentTypeRepo& repo,
     const DocumentType& type(*typePtr);
 
     StringTokenizer tokenizer(fieldNames, ",");
-    auto collection = std::make_unique<FieldCollection>(type);
-
+    Field::Set fields;
     for (const auto & token : tokenizer) {
         const DocumentType::FieldSet * fs = type.getFieldSet(token);
         if (fs) {
             for (const auto & fieldName : fs->getFields()) {
-                collection->insert(type.getField(fieldName));
+                fields.insert(&type.getField(fieldName));
             }
         } else {
-            collection->insert(type.getField(token));
+            fields.insert(&type.getField(token));
         }
     }
-
-    return collection;
+    return std::make_unique<FieldCollection>(type, std::move(fields));
 }
 
 }
diff --git a/document/src/vespa/document/fieldset/fieldsets.cpp b/document/src/vespa/document/fieldset/fieldsets.cpp
index d878d38be14..586cef0e5f4 100644
--- a/document/src/vespa/document/fieldset/fieldsets.cpp
+++ b/document/src/vespa/document/fieldset/fieldsets.cpp
@@ -12,6 +12,8 @@ namespace {
 
 uint64_t
 computeHash(const Field::Set & set) {
+    if (set.empty()) return 0ul;
+
     vespalib::asciistream os;
     for (const Field * field : set) {
         os << field->getName() << ':';
@@ -21,12 +23,13 @@ computeHash(const Field::Set & set) {
 
 }
 
-FieldCollection::FieldCollection(const DocumentType& type)
-    : _set(),
-      _hash(0),
+FieldCollection::FieldCollection(const DocumentType& type, Field::Set set)
+    : _set(std::move(set)),
+      _hash(computeHash(_set)),
       _docType(type)
-{
-}
+{ }
+
+FieldCollection::FieldCollection(const FieldCollection&) = default;
 
 FieldCollection::~FieldCollection() = default;
 
@@ -66,13 +69,6 @@ FieldCollection::contains(const FieldSet& fields) const
 }
 
 void
-FieldCollection::insert(const Field& f)
-{
-    _set.insert(&f);
-    _hash = computeHash(_set);
-}
-
-void
 FieldSet::copyFields(Document& dest, const Document& src, const FieldSet& fields)
 {
     if (fields.getType() == Type::ALL) {
diff --git a/document/src/vespa/document/fieldset/fieldsets.h b/document/src/vespa/document/fieldset/fieldsets.h
index eac35244012..d37d72d6109 100644
--- a/document/src/vespa/document/fieldset/fieldsets.h
+++ b/document/src/vespa/document/fieldset/fieldsets.h
@@ -41,7 +41,9 @@ class FieldCollection : public FieldSet
 public:
     typedef std::unique_ptr<FieldCollection> UP;
 
-    FieldCollection(const DocumentType& docType);
+    FieldCollection(const DocumentType& docType, Field::Set set);
+    FieldCollection(const FieldCollection &);
+    FieldCollection(FieldCollection&&) = default;
     ~FieldCollection() override;
 
     bool contains(const FieldSet& fields) const override;
@@ -55,11 +57,6 @@ public:
     }
 
     /**
-     * Inserts the given field into the collection.
-     */
-    void insert(const Field& f);
-
-    /**
      * Returns all the fields contained in this collection.
      */
     const Field::Set& getFields() const { return _set; }
diff --git a/searchcore/src/tests/proton/document_iterator/document_iterator_test.cpp b/searchcore/src/tests/proton/document_iterator/document_iterator_test.cpp
index f52ca606900..64e67672f0f 100644
--- a/searchcore/src/tests/proton/document_iterator/document_iterator_test.cpp
+++ b/searchcore/src/tests/proton/document_iterator/document_iterator_test.cpp
@@ -748,8 +748,9 @@ TEST("require that document selection and timestamp range works together") {
 }
 
 TEST("require that fieldset limits fields returned") {
-    document::FieldCollection limited(getDocType());
-    limited.insert(getDocType().getField("header"));
+    document::Field::Set fields;
+    fields.insert(&getDocType().getField("header"));
+    document::FieldCollection limited(getDocType(), std::move(fields));
     DocumentIterator itr(bucket(5), limited, selectAll(), newestV(), -1, false);
     itr.add(doc_with_fields("id:ns:foo::xxx1", Timestamp(1),  bucket(5)));
     IterateResult res = itr.iterate(largeNum);
diff --git a/storage/src/vespa/storage/persistence/fieldvisitor.cpp b/storage/src/vespa/storage/persistence/fieldvisitor.cpp
index df9d3df6379..0071d7a010f 100644
--- a/storage/src/vespa/storage/persistence/fieldvisitor.cpp
+++ b/storage/src/vespa/storage/persistence/fieldvisitor.cpp
@@ -6,10 +6,10 @@
 
 namespace storage {
 
-FieldVisitor::~FieldVisitor() {}
+FieldVisitor::~FieldVisitor() = default;
 
 void FieldVisitor::visitFieldValueNode(const document::select::FieldValueNode & node) {
-    _fields.insert(_docType.getField(node.getRealFieldName()));
+    _fields.insert(&_docType.getField(node.getRealFieldName()));
 }
 
 void FieldVisitor::visitComparison(const document::select::Compare & node) {
diff --git a/storage/src/vespa/storage/persistence/fieldvisitor.h b/storage/src/vespa/storage/persistence/fieldvisitor.h
index 4b47c68e33b..f2b07e193d6 100644
--- a/storage/src/vespa/storage/persistence/fieldvisitor.h
+++ b/storage/src/vespa/storage/persistence/fieldvisitor.h
@@ -16,17 +16,17 @@ namespace storage {
 class FieldVisitor : public document::select::Visitor {
 private:
     document::DocumentType _docType;
-    document::FieldCollection _fields;
+    document::Field::Set _fields;
     
 public:
-    FieldVisitor(const document::DocumentType & docType)
+    explicit FieldVisitor(const document::DocumentType & docType)
         : _docType(docType),
-          _fields(_docType)
+          _fields()
     {}
-    ~FieldVisitor();
+    ~FieldVisitor() override;
 
-    const document::FieldSet & getFieldSet() {
-        return _fields;
+    document::FieldCollection getFieldSet() {
+        return document::FieldCollection(_docType, std::move(_fields));
     }
 
     void visitFieldValueNode(const document::select::FieldValueNode &) override;