Skip to content

Commit

Permalink
Merge pull request #30612 from vespa-engine/havardpe/stop-using-non-h…
Browse files Browse the repository at this point in the history 
…eap-strict-OR

stop using non-heap strict OR
  • Loading branch information
@geirst
geirst authored Mar 13, 2024
2 parents 1a13f6f + 3b4a94c commit 2bcf020
Show file tree
Hide file tree
Showing 5 changed files with 56 additions and 33 deletions.
12 changes: 7 additions & 5 deletions searchlib/src/tests/queryeval/blueprint/intermediate_blueprints_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ using vespalib::slime::SlimeInserter;
using vespalib::make_string_short::fmt;
using Path = std::vector<std::variant<size_t,vespalib::stringref>>;

vespalib::string strict_equiv_name = "search::queryeval::EquivImpl<true, search::queryeval::StrictHeapOrSearch<search::queryeval::NoUnpack, vespalib::LeftArrayHeap, unsigned char> >";

struct InvalidSelector : ISourceSelector {
InvalidSelector() : ISourceSelector(Source()) {}
void setSource(uint32_t, Source) override { abort(); }
Expand Down Expand Up @@ -1141,7 +1143,7 @@ TEST("require that children does not optimize when parents refuse them to") {
MatchData::UP md = MatchData::makeTestInstance(100, 10);
top_up->fetchPostings(ExecuteInfo::FALSE);
SearchIterator::UP search = top_up->createSearch(*md, true);
EXPECT_EQUAL("search::queryeval::EquivImpl<true>", search->getClassName());
EXPECT_EQUAL(strict_equiv_name, search->getClassName());
{
const auto & e = dynamic_cast<const MultiSearch &>(*search);
EXPECT_EQUAL("search::BitVectorIteratorStrictT<false>", e.getChildren()[0]->getClassName());
Expand All @@ -1151,7 +1153,7 @@ TEST("require that children does not optimize when parents refuse them to") {

md->resolveTermField(12)->tagAsNotNeeded();
search = top_up->createSearch(*md, true);
EXPECT_EQUAL("search::queryeval::EquivImpl<true>", search->getClassName());
EXPECT_EQUAL(strict_equiv_name, search->getClassName());
{
const auto & e = dynamic_cast<const MultiSearch &>(*search);
EXPECT_EQUAL("search::BitVectorIteratorStrictT<false>", e.getChildren()[0]->getClassName());
Expand Down Expand Up @@ -1179,7 +1181,7 @@ TEST("require_that_unpack_optimization_is_not_overruled_by_equiv") {
MatchData::UP md = MatchData::makeTestInstance(100, 10);
top_up->fetchPostings(ExecuteInfo::FALSE);
SearchIterator::UP search = top_up->createSearch(*md, true);
EXPECT_EQUAL("search::queryeval::EquivImpl<true>", search->getClassName());
EXPECT_EQUAL(strict_equiv_name, search->getClassName());
{
const auto & e = dynamic_cast<const MultiSearch &>(*search);
EXPECT_EQUAL("search::queryeval::StrictHeapOrSearch<search::queryeval::(anonymous namespace)::FullUnpack, vespalib::LeftArrayHeap, unsigned char>",
Expand All @@ -1188,7 +1190,7 @@ TEST("require_that_unpack_optimization_is_not_overruled_by_equiv") {

md->resolveTermField(2)->tagAsNotNeeded();
search = top_up->createSearch(*md, true);
EXPECT_EQUAL("search::queryeval::EquivImpl<true>", search->getClassName());
EXPECT_EQUAL(strict_equiv_name, search->getClassName());
{
const auto & e = dynamic_cast<const MultiSearch &>(*search);
EXPECT_EQUAL("search::queryeval::StrictHeapOrSearch<search::queryeval::(anonymous namespace)::SelectiveUnpack, vespalib::LeftArrayHeap, unsigned char>",
Expand All @@ -1198,7 +1200,7 @@ TEST("require_that_unpack_optimization_is_not_overruled_by_equiv") {
md->resolveTermField(1)->tagAsNotNeeded();
md->resolveTermField(3)->tagAsNotNeeded();
search = top_up->createSearch(*md, true);
EXPECT_EQUAL("search::queryeval::EquivImpl<true>", search->getClassName());
EXPECT_EQUAL(strict_equiv_name, search->getClassName());
{
const auto & e = dynamic_cast<const MultiSearch &>(*search);
EXPECT_EQUAL("search::queryeval::StrictHeapOrSearch<search::queryeval::NoUnpack, vespalib::LeftArrayHeap, unsigned char>",
Expand Down
18 changes: 13 additions & 5 deletions searchlib/src/vespa/searchlib/attribute/attribute_blueprint_factory.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ using search::queryeval::PredicateBlueprint;
using search::queryeval::SearchIterator;
using search::queryeval::Searchable;
using search::queryeval::SimpleLeafBlueprint;
using search::queryeval::StrictHeapOrSearch;
using search::queryeval::WeightedSetTermBlueprint;
using search::queryeval::flow::btree_cost;
using search::queryeval::flow::btree_strict_cost;
Expand Down Expand Up @@ -235,11 +236,11 @@ AttributeFieldBlueprint::visitMembers(vespalib::ObjectVisitor &visitor) const

//-----------------------------------------------------------------------------

template <bool is_strict>
struct LocationPreFilterIterator : public OrLikeSearch<is_strict, NoUnpack>
template <bool is_strict, typename Parent>
struct LocationPreFilterIterator : public Parent
{
explicit LocationPreFilterIterator(OrSearch::Children children)
: OrLikeSearch<is_strict, NoUnpack>(std::move(children), NoUnpack()) {}
: Parent(std::move(children), NoUnpack()) {}
void doUnpack(uint32_t) override {}
};

Expand Down Expand Up @@ -312,9 +313,16 @@ class LocationPreFilterBlueprint : public ComplexLeafBlueprint
children.push_back(search->createIterator(tfmda[0], strict));
}
if (strict) {
return std::make_unique<LocationPreFilterIterator<true>>(std::move(children));
if (children.size() < 0x70) {
using Parent = StrictHeapOrSearch<NoUnpack, vespalib::LeftArrayHeap, uint8_t>;
return std::make_unique<LocationPreFilterIterator<true, Parent>>(std::move(children));
} else {
using Parent = StrictHeapOrSearch<NoUnpack, vespalib::LeftHeap, uint32_t>;
return std::make_unique<LocationPreFilterIterator<true, Parent>>(std::move(children));
}
} else {
return std::make_unique<LocationPreFilterIterator<false>>(std::move(children));
using Parent = OrLikeSearch<false, NoUnpack>;
return std::make_unique<LocationPreFilterIterator<false, Parent>>(std::move(children));
}
}
SearchIteratorUP createFilterSearch(bool strict, FilterConstraint constraint) const override {
Expand Down
2 changes: 2 additions & 0 deletions searchlib/src/vespa/searchlib/queryeval/children_iterators.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ class ChildrenIterators {
: _data(std::move(data)) {}
ChildrenIterators(ChildrenIterators && other) = default;

size_t size() const noexcept { return _data.size(); }

// convenience constructors for unit tests:
template <typename... Args>
ChildrenIterators(SearchIterator::UP a, Args&&... args) {
Expand Down
38 changes: 23 additions & 15 deletions searchlib/src/vespa/searchlib/queryeval/equivsearch.cpp
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
// Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.

#include "equivsearch.h"
#include <vespa/vespalib/util/left_right_heap.h>

namespace search::queryeval {

template <bool strict>
class EquivImpl : public OrLikeSearch<strict, NoUnpack>
template <bool strict, typename Parent>
class EquivImpl : public Parent
{
private:
fef::MatchData::UP _inputMatchData;
Expand All @@ -27,22 +28,22 @@ class EquivImpl : public OrLikeSearch<strict, NoUnpack>
const fef::TermFieldMatchDataArray &outputs);
};

template<bool strict>
EquivImpl<strict>::EquivImpl(MultiSearch::Children children,
fef::MatchData::UP inputMatchData,
const fef::TermMatchDataMerger::Inputs &inputs,
const fef::TermFieldMatchDataArray &outputs)
template<bool strict, typename Parent>
EquivImpl<strict, Parent>::EquivImpl(MultiSearch::Children children,
fef::MatchData::UP inputMatchData,
const fef::TermMatchDataMerger::Inputs &inputs,
const fef::TermFieldMatchDataArray &outputs)

: OrLikeSearch<strict, NoUnpack>(std::move(children), NoUnpack()),
_inputMatchData(std::move(inputMatchData)),
_merger(inputs, outputs),
_valid(outputs.valid())
: Parent(std::move(children), NoUnpack()),
_inputMatchData(std::move(inputMatchData)),
_merger(inputs, outputs),
_valid(outputs.valid())
{
}

template<bool strict>
template<bool strict, typename Parent>
void
EquivImpl<strict>::doUnpack(uint32_t docid)
EquivImpl<strict, Parent>::doUnpack(uint32_t docid)
{
if (_valid) {
MultiSearch::doUnpack(docid);
Expand All @@ -58,9 +59,16 @@ EquivSearch::create(Children children,
bool strict)
{
if (strict) {
return std::make_unique<EquivImpl<true>>(std::move(children), std::move(inputMatchData), inputs, outputs);
if (children.size() < 0x70) {
using Parent = StrictHeapOrSearch<NoUnpack, vespalib::LeftArrayHeap, uint8_t>;
return std::make_unique<EquivImpl<true, Parent>>(std::move(children), std::move(inputMatchData), inputs, outputs);
} else {
using Parent = StrictHeapOrSearch<NoUnpack, vespalib::LeftHeap, uint32_t>;
return std::make_unique<EquivImpl<true, Parent>>(std::move(children), std::move(inputMatchData), inputs, outputs);
}
} else {
return std::make_unique<EquivImpl<false>>(std::move(children), std::move(inputMatchData), inputs, outputs);
using Parent = OrLikeSearch<false, NoUnpack>;
return std::make_unique<EquivImpl<false, Parent>>(std::move(children), std::move(inputMatchData), inputs, outputs);
}
}

Expand Down
19 changes: 11 additions & 8 deletions searchlib/src/vespa/searchlib/queryeval/orlikesearch.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ class OrLikeSearch : public OrSearch
};

template <typename Unpack, typename HEAP, typename ref_t>
class StrictHeapOrSearch final : public OrSearch
class StrictHeapOrSearch : public OrSearch
{
private:
struct Less {
Expand All @@ -88,12 +88,12 @@ class StrictHeapOrSearch final : public OrSearch
_data[i] = i;
}
}
void onRemove(size_t index) override {
void onRemove(size_t index) final {
_unpacker.onRemove(index);
_child_docid.erase(_child_docid.begin() + index);
init_data();
}
void onInsert(size_t index) override {
void onInsert(size_t index) final {
_unpacker.onInsert(index);
_child_docid.insert(_child_docid.begin() + index, getChildren()[index]->getDocId());
init_data();
Expand All @@ -116,7 +116,8 @@ class StrictHeapOrSearch final : public OrSearch
HEAP::require_left_heap();
init_data();
}
void initRange(uint32_t begin, uint32_t end) override {
~StrictHeapOrSearch() override;
void initRange(uint32_t begin, uint32_t end) final {
OrSearch::initRange(begin, end);
for (size_t i = 0; i < getChildren().size(); ++i) {
_child_docid[i] = getChildren()[i]->getDocId();
Expand All @@ -125,24 +126,26 @@ class StrictHeapOrSearch final : public OrSearch
HEAP::push(data_begin(), data_pos(i), Less(_child_docid));
}
}
void doSeek(uint32_t docid) override {
void doSeek(uint32_t docid) final {
while (_child_docid[HEAP::front(data_begin(), data_end())] < docid) {
seek_child(HEAP::front(data_begin(), data_end()), docid);
HEAP::adjust(data_begin(), data_end(), Less(_child_docid));
}
setDocId(_child_docid[HEAP::front(data_begin(), data_end())]);
}
void doUnpack(uint32_t docid) override {
void doUnpack(uint32_t docid) override { // <- not final
_unpacker.each([&](ref_t child) {
if (__builtin_expect(_child_docid[child] == docid, false)) {
getChildren()[child]->doUnpack(docid);
}
}, getChildren().size());
}
bool needUnpack(size_t index) const override {
bool needUnpack(size_t index) const final {
return _unpacker.needUnpack(index);
}
Trinary is_strict() const override { return Trinary::True; }
Trinary is_strict() const final { return Trinary::True; }
};
template <typename Unpack, typename HEAP, typename ref_t>
StrictHeapOrSearch<Unpack, HEAP, ref_t>::~StrictHeapOrSearch() = default;

}

0 comments on commit 2bcf020

Please sign in to comment.