Merge pull request #6413 from vespa-engine/balder/prevent-large-adjustments

Balder/prevent large adjustments

4 files changed, 89 insertions, 15 deletions

diff --git a/searchcore/src/tests/proton/matching/matching_stats_test.cpp b/searchcore/src/tests/proton/matching/matching_stats_test.cpp
index 8b58a9e271c..e467c4b58b7 100644
--- a/searchcore/src/tests/proton/matching/matching_stats_test.cpp
+++ b/searchcore/src/tests/proton/matching/matching_stats_test.cpp
@@ -240,6 +240,65 @@ TEST("requireThatPartitionsAreAddedCorrectly") {
     EXPECT_EQUAL(1.0, all1.getPartition(1).wait_time_max());
 }
 
+TEST("requireThatSoftDoomIsSetAndAdded") {
+    MatchingStats stats;
+    MatchingStats stats2;
+    EXPECT_EQUAL(0ul, stats.softDoomed());
+    EXPECT_EQUAL(0.5, stats.softDoomFactor());
+    stats.softDoomFactor(0.7);
+    stats.softDoomed(3);
+    EXPECT_EQUAL(3ul, stats.softDoomed());
+    EXPECT_EQUAL(0.7, stats.softDoomFactor());
+    stats2.add(stats);
+    EXPECT_EQUAL(3ul, stats2.softDoomed());
+    EXPECT_EQUAL(0.5, stats2.softDoomFactor());  // Not affected by add
+}
+
+TEST("requireThatSoftDoomFacorIsComputedCorrectlyForDownAdjustment") {
+    MatchingStats stats;
+    EXPECT_EQUAL(0ul, stats.softDoomed());
+    EXPECT_EQUAL(0.5, stats.softDoomFactor());
+    stats.softDoomed(1);
+    stats.updatesoftDoomFactor(1.0, 0.5, 2.0);
+    EXPECT_EQUAL(1ul, stats.softDoomed());
+    EXPECT_EQUAL(0.47, stats.softDoomFactor());
+    stats.updatesoftDoomFactor(1.0, 0.5, 2.0);
+    EXPECT_EQUAL(1ul, stats.softDoomed());
+    EXPECT_EQUAL(0.44, stats.softDoomFactor());
+    stats.updatesoftDoomFactor(0.0009, 0.5, 2.0);   // hard limits less than 1ms should be ignored
+    EXPECT_EQUAL(1ul, stats.softDoomed());
+    EXPECT_EQUAL(0.44, stats.softDoomFactor());
+    stats.updatesoftDoomFactor(1.0, 0.0009, 2.0);   // soft limits less than 1ms should be ignored
+    EXPECT_EQUAL(1ul, stats.softDoomed());
+    EXPECT_EQUAL(0.44, stats.softDoomFactor());
+    stats.updatesoftDoomFactor(1.0, 0.5, 10.0);      // Prevent changes above 10%
+    EXPECT_EQUAL(1ul, stats.softDoomed());
+    EXPECT_EQUAL(0.396, stats.softDoomFactor());
+}
+
+TEST("requireThatSoftDoomFacorIsComputedCorrectlyForUpAdjustment") {
+    MatchingStats stats;
+    EXPECT_EQUAL(0ul, stats.softDoomed());
+    EXPECT_EQUAL(0.5, stats.softDoomFactor());
+    stats.softDoomed(1);
+    stats.updatesoftDoomFactor(1.0, 0.9, 0.1);
+    EXPECT_EQUAL(1ul, stats.softDoomed());
+    EXPECT_EQUAL(0.508, stats.softDoomFactor());
+    stats.updatesoftDoomFactor(1.0, 0.9, 0.1);
+    EXPECT_EQUAL(1ul, stats.softDoomed());
+    EXPECT_EQUAL(0.516, stats.softDoomFactor());
+    stats.updatesoftDoomFactor(0.0009, 0.9, 0.1);   // hard limits less than 1ms should be ignored
+    EXPECT_EQUAL(1ul, stats.softDoomed());
+    EXPECT_EQUAL(0.516, stats.softDoomFactor());
+    stats.updatesoftDoomFactor(1.0, 0.9, 0.1);   // soft limits less than 1ms should be ignored
+    EXPECT_EQUAL(1ul, stats.softDoomed());
+    EXPECT_EQUAL(0.524, stats.softDoomFactor());
+    stats.softDoomFactor(0.1);
+    stats.updatesoftDoomFactor(1.0, 0.9, 0.001);      // Prevent changes above 5%
+    EXPECT_EQUAL(1ul, stats.softDoomed());
+    EXPECT_EQUAL(0.105, stats.softDoomFactor());
+}
+
 TEST_MAIN() {
     TEST_RUN_ALL();
 }
diff --git a/searchcore/src/vespa/searchcore/proton/matching/matcher.cpp b/searchcore/src/vespa/searchcore/proton/matching/matcher.cpp
index 8de03411ea3..add4351f90b 100644
--- a/searchcore/src/vespa/searchcore/proton/matching/matcher.cpp
+++ b/searchcore/src/vespa/searchcore/proton/matching/matcher.cpp
@@ -56,9 +56,10 @@ FeatureSet::SP
 findFeatureSet(const DocsumRequest &req, MatchToolsFactory &mtf, bool summaryFeatures)
 {
     std::vector<uint32_t> docs;
-    for (size_t i = 0; i < req.hits.size(); ++i) {
-        if (req.hits[i].docid != search::endDocId) {
-            docs.push_back(req.hits[i].docid);
+    docs.reserve(req.hits.size());
+    for (const auto & hit : req.hits) {
+        if (hit.docid != search::endDocId) {
+            docs.push_back(hit.docid);
         }
     }
     std::sort(docs.begin(), docs.end());
@@ -102,7 +103,7 @@ Matcher::getFeatureSet(const DocsumRequest & req, ISearchContext & searchCtx, IA
         bool searchSessionCached = cache_props.lookup("query").found();
         if (searchSessionCached) {
             SearchSession::SP session(sessionMgr.pickSearch(sessionId));
-            if (session.get()) {
+            if (session) {
                 MatchToolsFactory &mtf = session->getMatchToolsFactory();
                 FeatureSet::SP result = findFeatureSet(req, mtf, summaryFeatures);
                 session->releaseEnumGuards();
@@ -117,7 +118,7 @@ Matcher::getFeatureSet(const DocsumRequest & req, ISearchContext & searchCtx, IA
     if (!mtf->valid()) {
         LOG(warning, "getFeatureSet(%s): query execution failed (invalid query). Returning empty feature set",
                      (summaryFeatures ? "summary features" : "rank features"));
-        return FeatureSet::SP(new FeatureSet());
+        return std::make_shared<FeatureSet>();
     }
     return findFeatureSet(req, *mtf, summaryFeatures);
 }
@@ -224,14 +225,14 @@ Matcher::match(const SearchRequest &request, vespalib::ThreadBundle &threadBundl
             shouldCacheSearchSession = cache_props.lookup("query").found();
             if (shouldCacheGroupingSession) {
                 GroupingSession::UP session(sessionMgr.pickGrouping(sessionId));
-                if (session.get()) {
+                if (session) {
                     return handleGroupingSession(sessionMgr, groupingContext, std::move(session));
                 }
             }
         }
         const Properties *feature_overrides = &request.propertiesMap.featureOverrides();
         if (shouldCacheSearchSession) {
-            owned_objects.feature_overrides.reset(new Properties(*feature_overrides));
+            owned_objects.feature_overrides = std::make_unique<Properties>(*feature_overrides);
             feature_overrides = owned_objects.feature_overrides.get();
         }
         MatchToolsFactory::UP mtf = create_match_tools_factory(request, searchContext, attrContext,
diff --git a/searchcore/src/vespa/searchcore/proton/matching/matching_stats.cpp b/searchcore/src/vespa/searchcore/proton/matching/matching_stats.cpp
index 0bc1807d714..4fb0e1d72e2 100644
--- a/searchcore/src/vespa/searchcore/proton/matching/matching_stats.cpp
+++ b/searchcore/src/vespa/searchcore/proton/matching/matching_stats.cpp
@@ -1,6 +1,7 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 
 #include "matching_stats.h"
+#include <cmath>
 
 namespace proton::matching {
 
@@ -13,6 +14,9 @@ MatchingStats::Partition &get_writable_partition(std::vector<MatchingStats::Part
     return state[id];
 }
 
+constexpr double MIN_TIMEOUT_SEC = 0.001;
+constexpr double MAX_CHANGE_FACTOR = 5;
+
 } // namespace proton::matching::<unnamed>
 
 MatchingStats::MatchingStats()
@@ -62,6 +66,7 @@ MatchingStats::add(const MatchingStats &rhs)
     _docsReRanked += rhs._docsReRanked;
     _softDoomed += rhs.softDoomed();
 
+
     _queryCollateralTime.add(rhs._queryCollateralTime);
     _queryLatency.add(rhs._queryLatency);
     _matchTime.add(rhs._matchTime);
@@ -75,10 +80,19 @@ MatchingStats::add(const MatchingStats &rhs)
 
 MatchingStats &
 MatchingStats::updatesoftDoomFactor(double hardLimit, double softLimit, double duration) {
-    if (duration < softLimit) {
-        _softDoomFactor += 0.01*(softLimit - duration)/hardLimit;
-    } else {
-        _softDoomFactor += 0.02*(softLimit - duration)/hardLimit;
+    // The safety capping here should normally not be necessary as all input numbers
+    // will normally be within reasonable values.
+    // It is merely a safety measure to avoid overflow on bad input as can happen with time senstive stuff
+    // in any soft real time system.
+    if ((hardLimit >= MIN_TIMEOUT_SEC) && (softLimit >= MIN_TIMEOUT_SEC)) {
+        double diff = (softLimit - duration)/hardLimit;
+        if (duration < softLimit) {
+            diff = std::min(diff, _softDoomFactor*MAX_CHANGE_FACTOR);
+            _softDoomFactor += 0.01*diff;
+        } else {
+            diff = std::max(diff, -_softDoomFactor*MAX_CHANGE_FACTOR);
+            _softDoomFactor += 0.02*diff;
+        }
     }
     return *this;
 }
diff --git a/searchcore/src/vespa/searchcore/proton/server/matchview.cpp b/searchcore/src/vespa/searchcore/proton/server/matchview.cpp
index 3162f9a1c45..9af648917c4 100644
--- a/searchcore/src/vespa/searchcore/proton/server/matchview.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/matchview.cpp
@@ -44,13 +44,13 @@ MatchView::MatchView(const Matchers::SP &matchers,
       _docIdLimit(docIdLimit)
 { }
 
-MatchView::~MatchView() { }
+MatchView::~MatchView() = default;
 
 Matcher::SP
 MatchView::getMatcher(const vespalib::string & rankProfile) const
 {
     Matcher::SP retval = _matchers->lookup(rankProfile);
-    if (retval.get() == NULL) {
+    if ( ! retval) {
         throw std::runtime_error(make_string("Failed locating Matcher for rank profile '%s'", rankProfile.c_str()));
     }
     LOG(debug, "Rankprofile = %s has termwise_limit=%f", rankProfile.c_str(), retval->get_termwise_limit());
@@ -60,8 +60,8 @@ MatchView::getMatcher(const vespalib::string & rankProfile) const
 
 MatchContext::UP MatchView::createContext() const {
     IAttributeContext::UP attrCtx = _attrMgr->createContext();
-    ISearchContext::UP searchCtx(new SearchContext(_indexSearchable, _docIdLimit.get()));
-    return MatchContext::UP(new MatchContext(std::move(attrCtx), std::move(searchCtx)));
+    auto searchCtx = std::make_unique<SearchContext>(_indexSearchable, _docIdLimit.get());
+    return std::make_unique<MatchContext>(std::move(attrCtx), std::move(searchCtx));
 }