diff options
author | Henning Baldersheim <balder@yahoo-inc.com> | 2018-07-18 15:45:11 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2018-07-18 15:45:11 +0200 |
commit | fc0bdb721c6452de8ed6a715ba0d4c93b5e60396 (patch) | |
tree | bdffc26ce4b0a03683454391c575f47d377c3166 /searchcore | |
parent | 27b628f599ca9e3f0432ab849ea83bb6f70ed03b (diff) | |
parent | 428829739f3d009a072045a462a8cea471f56eb5 (diff) |
Merge pull request #6413 from vespa-engine/balder/prevent-large-adjustments
Balder/prevent large adjustments
Diffstat (limited to 'searchcore')
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)); } |