Merge pull request #9512 from vespa-engine/toregge/refactor-handle-recorder

Refactor handle recorder.

5 files changed, 39 insertions, 36 deletions

diff --git a/searchcore/src/tests/proton/matching/handle_recorder/handle_recorder_test.cpp b/searchcore/src/tests/proton/matching/handle_recorder/handle_recorder_test.cpp
index 60caffd82d6..4675c09768e 100644
--- a/searchcore/src/tests/proton/matching/handle_recorder/handle_recorder_test.cpp
+++ b/searchcore/src/tests/proton/matching/handle_recorder/handle_recorder_test.cpp
@@ -11,7 +11,11 @@ using search::fef::MatchDataDetails;
 using search::fef::TermFieldHandle;
 using namespace proton::matching;
 
-using HandleSet = HandleRecorder::HandleSet;
+using HandleMap = HandleRecorder::HandleMap;
+
+constexpr MatchDataDetails NormalMask = MatchDataDetails::Normal;
+constexpr MatchDataDetails CheapMask = MatchDataDetails::Cheap;
+constexpr MatchDataDetails BothMask = static_cast<MatchDataDetails>(static_cast<int>(NormalMask) | static_cast<int>(CheapMask));
 
 void
 register_normal_handle(TermFieldHandle handle)
@@ -34,8 +38,7 @@ TEST(HandleRecorderTest, can_record_both_normal_and_cheap_handles)
         register_cheap_handle(5);
         register_normal_handle(7);
     }
-    EXPECT_EQ(HandleSet({3, 7}), recorder.get_normal_handles());
-    EXPECT_EQ(HandleSet({5}), recorder.get_cheap_handles());
+    EXPECT_EQ(HandleMap({{3, NormalMask}, {5, CheapMask}, {7, NormalMask}}), recorder.get_handles());
     EXPECT_EQ("normal: [3,7], cheap: [5]", recorder.to_string());
 }
 
@@ -47,8 +50,7 @@ TEST(HandleRecorderTest, the_same_handle_can_be_in_both_normal_and_cheap_set)
         register_normal_handle(3);
         register_cheap_handle(3);
     }
-    EXPECT_EQ(HandleSet({3}), recorder.get_normal_handles());
-    EXPECT_EQ(HandleSet({3}), recorder.get_cheap_handles());
+    EXPECT_EQ(HandleMap({{3, BothMask}}), recorder.get_handles());
 }
 
 GTEST_MAIN_RUN_ALL_TESTS()
diff --git a/searchcore/src/vespa/searchcore/proton/matching/handlerecorder.cpp b/searchcore/src/vespa/searchcore/proton/matching/handlerecorder.cpp
index a4f3d519311..ab1c8e3af49 100644
--- a/searchcore/src/vespa/searchcore/proton/matching/handlerecorder.cpp
+++ b/searchcore/src/vespa/searchcore/proton/matching/handlerecorder.cpp
@@ -4,6 +4,9 @@
 #include <vespa/vespalib/stllike/asciistream.h>
 #include <algorithm>
 #include <cassert>
+#include <vespa/vespalib/stllike/hash_map.hpp>
+#include <vespa/vespalib/stllike/hash_map_equal.hpp>
+#include <vespa/vespalib/util/array_equal.hpp>
 
 using search::fef::MatchDataDetails;
 using search::fef::TermFieldHandle;
@@ -22,20 +25,21 @@ namespace {
 }
 
 HandleRecorder::HandleRecorder() :
-    _normal_handles(),
-    _cheap_handles()
+    _handles()
 {
 }
 
 namespace {
 
 vespalib::string
-handles_to_string(const HandleRecorder::HandleSet& handles)
+handles_to_string(const HandleRecorder::HandleMap& handles, MatchDataDetails requested_details)
 {
     vespalib::asciistream os;
     std::vector<TermFieldHandle> sorted;
-    for (TermFieldHandle handle : handles) {
-        sorted.push_back(handle);
+    for (const auto &handle : handles) {
+        if ((static_cast<int>(handle.second) & static_cast<int>(requested_details)) != 0) {
+            sorted.push_back(handle.first);
+        }
     }
     std::sort(sorted.begin(), sorted.end());
     if ( !sorted.empty() ) {
@@ -53,8 +57,8 @@ vespalib::string
 HandleRecorder::to_string() const
 {
     vespalib::asciistream os;
-    os << "normal: [" << handles_to_string(_normal_handles) << "], ";
-    os << "cheap: [" << handles_to_string(_cheap_handles) << "]";
+    os << "normal: [" << handles_to_string(_handles, MatchDataDetails::Normal) << "], ";
+    os << "cheap: [" << handles_to_string(_handles, MatchDataDetails::Cheap) << "]";
     return os.str();
 }
 
@@ -100,13 +104,14 @@ HandleRecorder::add(TermFieldHandle handle,
                     MatchDataDetails requested_details)
 
 {
-    if (requested_details == MatchDataDetails::Normal) {
-        _normal_handles.insert(handle);
-    } else if (requested_details == MatchDataDetails::Cheap) {
-        _cheap_handles.insert(handle);
+    if (requested_details == MatchDataDetails::Normal ||
+        requested_details == MatchDataDetails::Cheap) {
+        _handles[handle] = static_cast<MatchDataDetails>(static_cast<int>(_handles[handle]) | static_cast<int>(requested_details));
     } else {
         abort();
     }
 }
 
 }
+
+VESPALIB_HASH_MAP_INSTANTIATE(search::fef::TermFieldHandle, search::fef::MatchDataDetails);
diff --git a/searchcore/src/vespa/searchcore/proton/matching/handlerecorder.h b/searchcore/src/vespa/searchcore/proton/matching/handlerecorder.h
index 54a30a0ed3d..88a21fa5d7b 100644
--- a/searchcore/src/vespa/searchcore/proton/matching/handlerecorder.h
+++ b/searchcore/src/vespa/searchcore/proton/matching/handlerecorder.h
@@ -4,7 +4,7 @@
 
 #include <vespa/searchlib/fef/handle.h>
 #include <vespa/searchlib/fef/match_data_details.h>
-#include <vespa/vespalib/stllike/hash_set.h>
+#include <vespa/vespalib/stllike/hash_map.h>
 #include <vespa/vespalib/util/noncopyable.hpp>
 
 namespace proton::matching {
@@ -20,7 +20,7 @@ namespace proton::matching {
 class HandleRecorder
 {
 public:
-    using HandleSet = vespalib::hash_set<search::fef::TermFieldHandle>;
+    using HandleMap = vespalib::hash_map<search::fef::TermFieldHandle, search::fef::MatchDataDetails>;
     class Binder : public vespalib::noncopyable {
     public:
         Binder(HandleRecorder & recorder);
@@ -33,16 +33,14 @@ public:
     };
     HandleRecorder();
     ~HandleRecorder();
-    const HandleSet& get_normal_handles() const { return _normal_handles; }
-    const HandleSet& get_cheap_handles() const { return _cheap_handles; }
+    const HandleMap& get_handles() const { return _handles; }
     static void register_handle(search::fef::TermFieldHandle handle,
                                 search::fef::MatchDataDetails requested_details);
     vespalib::string to_string() const;
 private:
     void add(search::fef::TermFieldHandle handle,
              search::fef::MatchDataDetails requested_details);
-    HandleSet _normal_handles;
-    HandleSet _cheap_handles;
+    HandleMap _handles;
 };
 
 }
diff --git a/searchcore/src/vespa/searchcore/proton/matching/match_tools.cpp b/searchcore/src/vespa/searchcore/proton/matching/match_tools.cpp
index e6dab8cbea4..bc87ea11335 100644
--- a/searchcore/src/vespa/searchcore/proton/matching/match_tools.cpp
+++ b/searchcore/src/vespa/searchcore/proton/matching/match_tools.cpp
@@ -26,18 +26,20 @@ namespace proton::matching {
 
 namespace {
 
-bool contains_all(const HandleRecorder::HandleSet &old_set,
-                  const HandleRecorder::HandleSet &new_set)
+bool contains_all(const HandleRecorder::HandleMap &old_map,
+                  const HandleRecorder::HandleMap &new_map)
 {
-    for (TermFieldHandle handle: new_set) {
-        if (old_set.find(handle) == old_set.end()) {
+    for (const auto &handle: new_map) {
+        const auto old_itr = old_map.find(handle.first);
+        if (old_itr == old_map.end() ||
+            ((static_cast<int>(handle.second) & ~static_cast<int>(old_itr->second)) != 0)) {
             return false;
         }
     }
     return true;
 }
 
-void tag_match_data(const HandleRecorder::HandleSet &handles, MatchData &match_data) {
+void tag_match_data(const HandleRecorder::HandleMap &handles, MatchData &match_data) {
     // TODO: Move tagging to separate component (for testing) and tag normal and cheap.
     for (TermFieldHandle handle = 0; handle < match_data.getNumTermFields(); ++handle) {
         if (handles.find(handle) == handles.end()) {
@@ -82,14 +84,12 @@ MatchTools::setup(search::fef::RankProgram::UP rank_program, double termwise_lim
         _rank_program->setup(*_match_data, _queryEnv, _featureOverrides);
     }
     bool can_reuse_search = (_search && !_search_has_changed &&
-            contains_all(_used_normal_handles, recorder.get_normal_handles()) &&
-            contains_all(_used_cheap_handles, recorder.get_cheap_handles()));
+            contains_all(_used_handles, recorder.get_handles()));
     if (!can_reuse_search) {
-        tag_match_data(recorder.get_normal_handles(), *_match_data);
+        tag_match_data(recorder.get_handles(), *_match_data);
         _match_data->set_termwise_limit(termwise_limit);
         _search = _query.createSearch(*_match_data);
-        _used_normal_handles = recorder.get_normal_handles();
-        _used_cheap_handles = recorder.get_cheap_handles();
+        _used_handles = recorder.get_handles();
         _search_has_changed = false;
     }
 }
@@ -114,8 +114,7 @@ MatchTools::MatchTools(QueryLimiter & queryLimiter,
       _match_data(mdl.createMatchData()),
       _rank_program(),
       _search(),
-      _used_normal_handles(),
-      _used_cheap_handles(),
+      _used_handles(),
       _search_has_changed(false)
 {
 }
diff --git a/searchcore/src/vespa/searchcore/proton/matching/match_tools.h b/searchcore/src/vespa/searchcore/proton/matching/match_tools.h
index 10770554187..bbdf30939db 100644
--- a/searchcore/src/vespa/searchcore/proton/matching/match_tools.h
+++ b/searchcore/src/vespa/searchcore/proton/matching/match_tools.h
@@ -35,8 +35,7 @@ private:
     search::fef::MatchData::UP             _match_data;
     search::fef::RankProgram::UP           _rank_program;
     search::queryeval::SearchIterator::UP  _search;
-    HandleRecorder::HandleSet              _used_normal_handles;
-    HandleRecorder::HandleSet              _used_cheap_handles;
+    HandleRecorder::HandleMap              _used_handles;
     bool                                   _search_has_changed;
     void setup(search::fef::RankProgram::UP, double termwise_limit = 1.0);
 public: