Skip to content

Commit

Permalink
try switching to rvalues
Browse files Browse the repository at this point in the history
  • Loading branch information
hendrikmuhs committed Apr 19, 2024
1 parent a6c2168 commit 8670fff
Show file tree
Hide file tree
Showing 9 changed files with 100 additions and 61 deletions.
12 changes: 8 additions & 4 deletions keyvi/bin/keyvi_c/c_api.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,18 @@ struct keyvi_dictionary {
};

struct keyvi_match {
explicit keyvi_match(const Match& obj) : obj_(obj) {}
explicit keyvi_match(Match&& obj) : obj_(std::move(obj)) {}

Match obj_;
};

struct keyvi_match_iterator {
explicit keyvi_match_iterator(const MatchIterator::MatchIteratorPair& obj) : current_(obj.begin()), end_(obj.end()) {}
explicit keyvi_match_iterator(const MatchIterator::MatchIteratorPair& obj)
//p_(std::move(obj)) {}
{}
//current_(std::get<0>(obj)), end_(std::move(std::get<1>(obj))) {}

//MatchIterator::MatchIteratorPair p_;
MatchIterator current_;
const MatchIterator end_;
};
Expand Down Expand Up @@ -187,8 +191,8 @@ bool keyvi_match_iterator_empty(const keyvi_match_iterator* iterator) {
return iterator->current_ == iterator->end_;
}

keyvi_match* keyvi_match_iterator_dereference(const keyvi_match_iterator* iterator) {
return new keyvi_match(*iterator->current_);
keyvi_match* keyvi_match_iterator_dereference(keyvi_match_iterator* iterator) {
return new keyvi_match(Match()); //*iterator->current_);
}

void keyvi_match_iterator_increment(keyvi_match_iterator* iterator) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ class MultiWordCompletion final {
}
};

return MatchIterator::MakeIteratorPair(tfunc, first_match);
return MatchIterator::MakeIteratorPair(tfunc, std::move(first_match));
}

return MatchIterator::EmptyIteratorPair();
Expand Down
4 changes: 2 additions & 2 deletions keyvi/include/keyvi/dictionary/completion/prefix_completion.h
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ class PrefixCompletion final {
}
};

return MatchIterator::MakeIteratorPair(tfunc, first_match);
return MatchIterator::MakeIteratorPair(tfunc, std::move(first_match));
}

return MatchIterator::EmptyIteratorPair();
Expand Down Expand Up @@ -219,7 +219,7 @@ class PrefixCompletion final {
}
};

return MatchIterator::MakeIteratorPair(tfunc, first_match);
return MatchIterator::MakeIteratorPair(tfunc, std::move(first_match));
}

private:
Expand Down
53 changes: 25 additions & 28 deletions keyvi/include/keyvi/dictionary/dictionary.h
Original file line number Diff line number Diff line change
Expand Up @@ -142,21 +142,23 @@ class Dictionary final {
}

Match m;
bool has_run = false;
//bool has_run = false;

// right now this is returning just 1 match, but it could be more if it is a multi-value dictionary
m = Match(0, text_length, key, 0, fsa_, fsa_->GetStateValue(state));

auto func = [m, has_run]() mutable {
/*
auto func = [m = std::move(m), has_run]() mutable {
if (!has_run) {
has_run = true;
return m;
return std::move(m);
}
return Match();
};
};*/

return MatchIterator::MakeIteratorPair(func);
return MatchIterator::MakeIteratorPair([](){return Match();}, std::move(m));

//return MatchIterator::MakeIteratorPair(func);
}

/**
Expand Down Expand Up @@ -208,7 +210,7 @@ class Dictionary final {
}
}
};
return MatchIterator::MakeIteratorPair(tfunc, first_match);
return MatchIterator::MakeIteratorPair(tfunc, std::move(first_match));
}

/**
Expand Down Expand Up @@ -239,24 +241,14 @@ class Dictionary final {
}

Match m;
bool has_run = false;

if (last_final_state) {
// right now this is returning just 1 match, but it could do more
m = Match(offset, last_final_state_position, text.substr(offset, last_final_state_position - offset), 0, fsa_,
fsa_->GetStateValue(last_final_state));
}

auto func = [m, has_run]() mutable {
if (!has_run) {
has_run = true;
return m;
}

return Match();
};

return MatchIterator::MakeIteratorPair(func);
return MatchIterator::MakeIteratorPair([](){return Match();}, std::move(m));
}

/**
Expand All @@ -271,7 +263,7 @@ class Dictionary final {

TRACE("LookupText, 1st lookup for: %s", text.c_str());

iterators.push(Lookup(text).begin());
iterators.push(std::get<0>(Lookup(text)));
size_t position = 1;

while (position < text_length) {
Expand All @@ -282,21 +274,24 @@ class Dictionary final {

++position;
TRACE("LookupText, starting lookup for: %s", text.c_str() + position);
iterators.push(Lookup(text, position).begin());
iterators.push(std::get<0>(Lookup(text, position)));
}

MatchIterator current_it = iterators.front();
/*
MatchIterator& current_it = iterators.front();
iterators.pop();
auto func = [iterators, current_it]() mutable {
while (iterators.size() && (*current_it).IsEmpty()) {
current_it = iterators.front();
//auto func = [iterators, current_it]() mutable {
auto func = [iterators = std::move(iterators), current_it = std::move(current_it)]() mutable {
while (iterators.size() && (*iterators.front()).IsEmpty()) {
iterators.pop();
}
return *current_it++;
};

*/
auto func = []()
{return Match();};
return MatchIterator::MakeIteratorPair(func);
}

Expand All @@ -315,8 +310,10 @@ class Dictionary final {
auto data = std::make_shared<matching::NearMatching<>>(
matching::NearMatching<>::FromSingleFsa(fsa_, key, minimum_prefix_length, greedy));

auto func = [data]() { return data->NextMatch(); };
return MatchIterator::MakeIteratorPair(func, data->FirstMatch());
auto func = [data]() { return std::move(data->NextMatch()); };

Check warning on line 313 in keyvi/include/keyvi/dictionary/dictionary.h

View workflow job for this annotation

GitHub Actions / Release on macos-latest

moving a temporary object prevents copy elision [-Wpessimizing-move]

Check warning on line 313 in keyvi/include/keyvi/dictionary/dictionary.h

View workflow job for this annotation

GitHub Actions / Debug on macos-latest

moving a temporary object prevents copy elision [-Wpessimizing-move]
//Match m(std::move(data->FirstMatch()));

return MatchIterator::MakeIteratorPair(func, std::move(data->FirstMatch()));

Check warning on line 316 in keyvi/include/keyvi/dictionary/dictionary.h

View workflow job for this annotation

GitHub Actions / Release on macos-latest

moving a temporary object prevents copy elision [-Wpessimizing-move]

Check warning on line 316 in keyvi/include/keyvi/dictionary/dictionary.h

View workflow job for this annotation

GitHub Actions / Debug on macos-latest

moving a temporary object prevents copy elision [-Wpessimizing-move]
}

MatchIterator::MatchIteratorPair GetFuzzy(const std::string& query, const int32_t max_edit_distance,
Expand Down
44 changes: 36 additions & 8 deletions keyvi/include/keyvi/dictionary/match.h
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,41 @@ struct Match {

Match() : matched_item_(), raw_value_() {}

Match(Match&& other)
: start_(other.start_),
end_(other.end_),
matched_item_(std::move(other.matched_item_)),
raw_value_(std::move(other.raw_value_)),
score_(other.score_),
fsa_(other.fsa_),
state_(other.state_),
attributes_(std::move(other.attributes_)) {
other.start_ = 0;
other.end_ = 0;
other.score_ = 0;
other.state_ = 0;
}

Match& operator=(Match&& other) {
start_ = other.start_;
end_ = other.end_;
matched_item_ = std::move(other.matched_item_);
raw_value_ = std::move(other.raw_value_);
score_ = other.score_;
fsa_ = std::move(other.fsa_);
state_ = other.state_;
attributes_ = std::move(other.attributes_);

other.start_ = 0;
other.end_ = 0;
other.score_ = 0;
other.state_ = 0;
return *this;
}

// todo: consider disallowing copy and assignment
// Match& operator=(Match const&) = delete;
// Match(const Match& that) = delete;
//Match& operator=(Match const&) = delete;
//Match(const Match& that) = delete;

size_t GetEnd() const { return end_; }

Expand Down Expand Up @@ -184,9 +216,7 @@ struct Match {
*
* @param value
*/
void SetRawValue(const std::string& value) {
raw_value_ = value;
}
void SetRawValue(const std::string& value) { raw_value_ = value; }

private:
size_t start_ = 0;
Expand All @@ -204,9 +234,7 @@ struct Match {
template <class MatcherT, class DeletedT>
friend Match index::internal::FirstFilteredMatch(const MatcherT&, const DeletedT&);

fsa::automata_t& GetFsa() {
return fsa_;
}
fsa::automata_t& GetFsa() { return fsa_; }
};

} /* namespace dictionary */
Expand Down
32 changes: 21 additions & 11 deletions keyvi/include/keyvi/dictionary/match_iterator.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,11 @@
#ifndef KEYVI_DICTIONARY_MATCH_ITERATOR_H_
#define KEYVI_DICTIONARY_MATCH_ITERATOR_H_

#include <tuple>

#include <boost/iterator/iterator_facade.hpp>

#include "keyvi/dictionary/match.h"
#include "keyvi/dictionary/util/iterator_utils.h"

// #define ENABLE_TRACING
#include "keyvi/dictionary/util/trace.h"
Expand All @@ -53,25 +54,34 @@ namespace dictionary {
*/
class MatchIterator : public boost::iterator_facade<MatchIterator, Match const, boost::single_pass_traversal_tag> {
public:
typedef util::iterator_pair<MatchIterator> MatchIteratorPair;
using MatchIteratorPair = std::tuple<MatchIterator, MatchIterator>;

explicit MatchIterator(std::function<Match()> match_functor, const Match& first_match = Match(),
explicit MatchIterator(std::function<Match()> match_functor, Match&& first_match = Match(),
std::function<void(uint32_t)> set_min_weight = {})
: match_functor_(match_functor), set_min_weight_(set_min_weight) {
current_match_ = first_match;
: match_functor_(match_functor), current_match_(std::move(first_match)), set_min_weight_(set_min_weight) {
//current_match_ = first_match;
if (first_match.IsEmpty()) {
increment();
}
}

static MatchIteratorPair MakeIteratorPair(std::function<Match()> f, const Match& first_match = Match(),
std::function<void(uint32_t)> set_min_weight = {}) {
return MatchIteratorPair(MatchIterator(f, first_match, set_min_weight), MatchIterator());
MatchIterator(MatchIterator&& other)
: match_functor_(std::move(other.match_functor_)),
current_match_(std::move(other.current_match_)),
set_min_weight_(std::move(other.set_min_weight_)) {
other.current_match_ = Match();
}

static MatchIteratorPair EmptyIteratorPair() { return MatchIteratorPair(MatchIterator(), MatchIterator()); }

MatchIterator() : match_functor_(0), set_min_weight_({}) {}
//MatchIterator& operator=(MatchIterator const&) = delete;
//MatchIterator(const MatchIterator& that) = delete;

static MatchIteratorPair MakeIteratorPair(std::function<Match()> f, Match&& first_match = Match(),
std::function<void(uint32_t)> set_min_weight = {}) {
return std::make_tuple<>(MatchIterator(f, std::move(first_match), set_min_weight), MatchIterator());
}

static MatchIteratorPair EmptyIteratorPair() { return std::make_tuple<>(MatchIterator(), MatchIterator()); }

void SetMinWeight(uint32_t min_weight) {
// ignore if a min weight setter was not provided
Expand All @@ -89,7 +99,7 @@ class MatchIterator : public boost::iterator_facade<MatchIterator, Match const,
if (match_functor_) {
TRACE("Match Iterator: call increment()");

current_match_ = match_functor_();
current_match_ = std::move(match_functor_());

Check warning on line 102 in keyvi/include/keyvi/dictionary/match_iterator.h

View workflow job for this annotation

GitHub Actions / Release on macos-latest

moving a temporary object prevents copy elision [-Wpessimizing-move]

Check warning on line 102 in keyvi/include/keyvi/dictionary/match_iterator.h

View workflow job for this annotation

GitHub Actions / Debug on macos-latest

moving a temporary object prevents copy elision [-Wpessimizing-move]

// if we get an empty match, release the functor
if (current_match_.IsEmpty()) {
Expand Down
2 changes: 1 addition & 1 deletion keyvi/include/keyvi/dictionary/matching/fuzzy_matching.h
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ class FuzzyMatching final {
return fsa_start_state_pairs;
}

Match FirstMatch() const { return first_match_; }
Match FirstMatch() { return first_match_; }

Check failure on line 236 in keyvi/include/keyvi/dictionary/matching/fuzzy_matching.h

View workflow job for this annotation

GitHub Actions / Debug on ubuntu-22.04

use of deleted function ‘keyvi::dictionary::Match::Match(const keyvi::dictionary::Match&)’

Check failure on line 236 in keyvi/include/keyvi/dictionary/matching/fuzzy_matching.h

View workflow job for this annotation

GitHub Actions / Release on ubuntu-22.04

use of deleted function ‘keyvi::dictionary::Match::Match(const keyvi::dictionary::Match&)’

Check failure on line 236 in keyvi/include/keyvi/dictionary/matching/fuzzy_matching.h

View workflow job for this annotation

GitHub Actions / Release on macos-latest

call to implicitly-deleted copy constructor of 'keyvi::dictionary::Match'

Check failure on line 236 in keyvi/include/keyvi/dictionary/matching/fuzzy_matching.h

View workflow job for this annotation

GitHub Actions / Debug on macos-latest

call to implicitly-deleted copy constructor of 'keyvi::dictionary::Match'

Match NextMatch() {
for (; traverser_ptr_ && *traverser_ptr_; (*traverser_ptr_)++) {
Expand Down
4 changes: 2 additions & 2 deletions keyvi/include/keyvi/dictionary/matching/near_matching.h
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ class NearMatching final {
return fsa_start_state_payloads;
}

Match FirstMatch() const { return first_match_; }
Match FirstMatch() { return first_match_; }

Check failure on line 185 in keyvi/include/keyvi/dictionary/matching/near_matching.h

View workflow job for this annotation

GitHub Actions / Debug on ubuntu-22.04

use of deleted function ‘keyvi::dictionary::Match::Match(const keyvi::dictionary::Match&)’

Check failure on line 185 in keyvi/include/keyvi/dictionary/matching/near_matching.h

View workflow job for this annotation

GitHub Actions / Release on ubuntu-22.04

use of deleted function ‘keyvi::dictionary::Match::Match(const keyvi::dictionary::Match&)’

Check failure on line 185 in keyvi/include/keyvi/dictionary/matching/near_matching.h

View workflow job for this annotation

GitHub Actions / Release on macos-latest

call to implicitly-deleted copy constructor of 'keyvi::dictionary::Match'

Check failure on line 185 in keyvi/include/keyvi/dictionary/matching/near_matching.h

View workflow job for this annotation

GitHub Actions / Debug on macos-latest

call to implicitly-deleted copy constructor of 'keyvi::dictionary::Match'

Match NextMatch() {
TRACE("call next match %lu", matched_depth_);
Expand Down Expand Up @@ -217,7 +217,7 @@ class NearMatching final {
private:
std::unique_ptr<innerTraverserType> traverser_ptr_;
const std::string exact_prefix_;
const Match first_match_;
Match first_match_;
const bool greedy_ = false;
size_t matched_depth_ = 0;

Expand Down
8 changes: 4 additions & 4 deletions keyvi/include/keyvi/dictionary/util/iterator_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,14 @@ class iterator_pair {
iterator_pair(){}

iterator_pair(Iterator first, Iterator last)
: f_(first),
l_(last) {
: f_(std::move(first)),
l_(std::move(last)) {
}
Iterator begin() const {
return f_;
return std::move(f_);
}
Iterator end() const {
return l_;
return std::move(l_);
}

private:
Expand Down

0 comments on commit 8670fff

Please sign in to comment.