Skip to content

Commit

Permalink
Address all comments from Johannes' review
Browse files Browse the repository at this point in the history
  • Loading branch information
Hannah Bast committed Nov 19, 2024
2 parents f4ccd87 + 92d906f commit b8e5803
Show file tree
Hide file tree
Showing 5 changed files with 82 additions and 59 deletions.
7 changes: 2 additions & 5 deletions src/engine/LocalVocab.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "absl/strings/str_cat.h"
#include "global/Id.h"
#include "global/ValueId.h"
#include "util/TransparentFunctors.h"

// _____________________________________________________________________________
LocalVocab LocalVocab::clone() const {
Expand All @@ -20,11 +21,7 @@ LocalVocab LocalVocab::clone() const {
// _____________________________________________________________________________
LocalVocab LocalVocab::merge(std::span<const LocalVocab*> vocabs) {
LocalVocab result;
result.mergeWith(vocabs |
std::views::transform(
[](const LocalVocab* localVocab) -> const LocalVocab& {
return *localVocab;
}));
result.mergeWith(vocabs | std::views::transform(ad_utility::dereference));
return result;
}

Expand Down
21 changes: 15 additions & 6 deletions src/engine/LocalVocab.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,17 @@
// after it has been computed once.
//
// A `LocalVocab` has a primary set of `LocalVocabEntry`s, which can grow
// dynamically, and a collection of other sets of `LocalVocabEntry`s, which are
// static. A `LocalVocabEntry` lives exactly as long as it is contained in at
// least one of the (primary or other) sets of a `LocalVocab`.
// dynamically, and a collection of other sets of `LocalVocabEntry`s, which
// cannot be modified by this class. A `LocalVocabEntry` lives exactly as long
// as it is contained in at least one of the (primary or other) sets of a
// `LocalVocab`.
class LocalVocab {
private:
// The primary set of `LocalVocabEntry`s, which can grow dynamically.
//
// NOTE: This is a `absl::node_hash_set` because we hand out pointers to
// the `LocalVocabEntry`s and it is hence essential that their addresses
// remain stable over their lifetime in the hash set.
using Set = absl::node_hash_set<LocalVocabEntry>;
std::shared_ptr<Set> primaryWordSet_ = std::make_shared<Set>();

Expand All @@ -54,9 +59,10 @@ class LocalVocab {
LocalVocab(const LocalVocab&) = delete;
LocalVocab& operator=(const LocalVocab&) = delete;

// Make a logical copy, where all sets of `LocalVocabEntry`s become static
// and the primary set becomes empty. This only copies shared pointers and
// takes time linear in the number of sets.
// Make a logical copy, where all sets of `LocalVocabEntry`s become "other"
// sets, that is, they cannot be modified by the copy. The primary set becomes
// empty. This only copies shared pointers and takes time linear in the number
// of sets.
LocalVocab clone() const;

// Moving a local vocabulary is not problematic (though the typical use case
Expand Down Expand Up @@ -92,6 +98,9 @@ class LocalVocab {
bool empty() const { return size() == 0; }

// Get the `LocalVocabEntry` corresponding to the given `LocalVocabIndex`.
//
// NOTE: This used to be a more complex function but is now a simple
// dereference. It could be thrown out in the future.
const LocalVocabEntry& getWord(LocalVocabIndex localVocabIndex) const;

// Add all sets (primary and other) of the given local vocabs as other sets
Expand Down
6 changes: 2 additions & 4 deletions src/engine/TransitivePathBase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ Result::Generator TransitivePathBase::fillTableWithHullImpl(
ad_utility::Timer timer{ad_utility::Timer::Stopped};
size_t outputRow = 0;
IdTableStatic<OUTPUT_WIDTH> table{getResultWidth(), allocator()};
std::vector<LocalVocab> storedLocalVocabs;
LocalVocab mergedVocab{};
for (auto& [node, linkedNodes, localVocab, idTable, inputRow] : hull) {
timer.cont();
// As an optimization nodes without any linked nodes should not get yielded
Expand All @@ -120,7 +120,7 @@ Result::Generator TransitivePathBase::fillTableWithHullImpl(
}

if (yieldOnce) {
storedLocalVocabs.emplace_back(std::move(localVocab));
mergedVocab.mergeWith(std::span{&localVocab, 1});
} else {
timer.stop();
runtimeInfo().addDetail("IdTable fill time", timer.msecs());
Expand All @@ -132,8 +132,6 @@ Result::Generator TransitivePathBase::fillTableWithHullImpl(
}
if (yieldOnce) {
timer.start();
LocalVocab mergedVocab{};
mergedVocab.mergeWith(storedLocalVocabs);
runtimeInfo().addDetail("IdTable fill time", timer.msecs());
co_yield {std::move(table).toDynamic(), std::move(mergedVocab)};
}
Expand Down
16 changes: 13 additions & 3 deletions src/engine/TransitivePathImpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ class TransitivePathImpl : public TransitivePathBase {
transitiveHull(edges, sub->getCopyOfLocalVocab(), std::move(nodes),
targetSide.isVariable()
? std::nullopt
: std::optional{std::get<Id>(targetSide.value_)});
: std::optional{std::get<Id>(targetSide.value_)},
yieldOnce);

auto result = fillTableWithHull(
std::move(hull), startSide.outputCol_, targetSide.outputCol_,
Expand Down Expand Up @@ -131,7 +132,8 @@ class TransitivePathImpl : public TransitivePathBase {
edges, sub->getCopyOfLocalVocab(), std::span{&tableInfo, 1},
targetSide.isVariable()
? std::nullopt
: std::optional{std::get<Id>(targetSide.value_)});
: std::optional{std::get<Id>(targetSide.value_)},
yieldOnce);

auto result = fillTableWithHull(std::move(hull), startSide.outputCol_,
targetSide.outputCol_, yieldOnce);
Expand Down Expand Up @@ -240,11 +242,15 @@ class TransitivePathImpl : public TransitivePathBase {
* `TableColumnWithVocab` that can be consumed to create a transitive hull.
* @param target Optional target Id. If supplied, only paths which end
* in this Id are added to the hull.
* @param yieldOnce This has to be set to the same value as the consuming
* code. When set to true, this will prevent yielding the same LocalVocab over
* and over again to make merging faster (because merging with an empty
* LocalVocab is a no-op).
* @return Map Maps each Id to its connected Ids in the transitive hull
*/
NodeGenerator transitiveHull(const T& edges, LocalVocab edgesVocab,
std::ranges::range auto startNodes,
std::optional<Id> target) const {
std::optional<Id> target, bool yieldOnce) const {
ad_utility::Timer timer{ad_utility::Timer::Stopped};
for (auto&& tableColumn : startNodes) {
timer.cont();
Expand All @@ -260,6 +266,10 @@ class TransitivePathImpl : public TransitivePathBase {
mergedVocab.clone(), tableColumn.table_,
currentRow};
timer.cont();
// Reset vocab to prevent merging the same vocab over and over again.
if (yieldOnce) {
mergedVocab = LocalVocab{};
}
}
currentRow++;
}
Expand Down
91 changes: 50 additions & 41 deletions src/util/TransparentFunctors.h
Original file line number Diff line number Diff line change
@@ -1,25 +1,23 @@
// Copyright 2022, University of Freiburg,
// Chair of Algorithms and Data Structures.
// Author:
// 2022 - Johannes Kalmbach <[email protected]>
// Copyright 2022 - 2024, University of Freiburg
// Chair of Algorithms and Data Structures
// Author: Johannes Kalmbach <[email protected]>

#ifndef QLEVER_TRANSPARENTFUNCTORS_H
#define QLEVER_TRANSPARENTFUNCTORS_H
#pragma once

#include <util/Forward.h>
#include <util/TypeTraits.h>

#include <utility>

/// Contains several function object types with templated operator() that wrap
/// overloaded functions from the standard library. This enables passing them as
/// function parameters.

/// Note that in theory all of them could be implemented shorter as captureless
/// lambda expressions. We have chosen not to do this because the STL also does
/// not choose this approach (see e.g. `std::less`, `std::plus`, etc.) and
/// because global inline lambdas in header files might in theory cause ODR (one
/// definition rule) problems, especially when using different compilers.
// Contains several function object types with templated operator() that wrap
// overloaded functions from the standard library. This enables passing them as
// function parameters.
//
// NOTE: in theory all of them could be implemented shorter as captureless
// lambda expressions. We have chosen not to do this because the STL also does
// not choose this approach (see e.g. `std::less`, `std::plus`, etc.) and
// because global inline lambdas in header files might in theory cause ODR (one
// definition rule) problems, especially when using different compilers.

namespace ad_utility {

Expand Down Expand Up @@ -79,37 +77,60 @@ struct ToBoolImpl {
}
};

// Implementation of `staticCast` (see below).
template <typename T>
struct StaticCastImpl {
constexpr decltype(auto) operator()(auto&& x) const {
return static_cast<T>(AD_FWD(x));
}
};

// Implementation of `dereference` (see below).
struct DereferenceImpl {
constexpr decltype(auto) operator()(auto&& x) const { return *AD_FWD(x); }
};

} // namespace detail

/// Return the first element via perfect forwarding of any type for which
/// `std::get<0>(x)` is valid. This holds e.g. for `std::pair`, `std::tuple`,
/// and `std::array`.
// Return the first element via perfect forwarding of any type for which
// `std::get<0>(x)` is valid. This holds e.g. for `std::pair`, `std::tuple`,
// and `std::array`.
static constexpr detail::FirstImpl first;

/// Return the second element via perfect forwarding of any type for which
/// `std::get<1>(x)` is valid. This holds e.g. for `std::pair`, `std::tuple`,
/// and `std::array`.
// Return the second element via perfect forwarding of any type for which
// `std::get<1>(x)` is valid. This holds e.g. for `std::pair`, `std::tuple`,
// and `std::array`.
static constexpr detail::SecondImpl second;

/// Transparent functor for `std::holds_alternative`
// Transparent functor for `std::holds_alternative`
template <typename T>
static constexpr detail::HoldsAlternativeImpl<T> holdsAlternative;

/// Transparent functor for `std::get`. Currently only works for `std::variant`
/// and not for `std::array` or `std::tuple`.
// Transparent functor for `std::get`. Currently only works for `std::variant`
// and not for `std::array` or `std::tuple`.
template <typename T>
static constexpr detail::GetImpl<T> get;

/// Transparent functor for `std::get_if`. As an extension to `std::get_if`,
/// `ad_utility::getIf` may also be called with a `variant` object or reference,
/// not only with a pointer.
// Transparent functor for `std::get_if`. As an extension to `std::get_if`,
// `ad_utility::getIf` may also be called with a `variant` object or reference,
// not only with a pointer.
template <typename T>
static constexpr detail::GetIfImpl<T> getIf;

// Transparent functor that converts any type to `bool` via
// `static_cast<bool>`.
static constexpr detail::ToBoolImpl toBool;

/// A functor that takes an arbitrary number of arguments by reference and does
/// nothing.
// Transparent functor that casts any type to `T` via `static_cast<T>`.
template <typename T>
static constexpr detail::StaticCastImpl<T> staticCast{};

// Transparent functor that dereferences a pointer or smart pointer.
static constexpr detail::DereferenceImpl dereference;

// Transparent functor that takes an arbitrary number of arguments by reference
// and does nothing. We also use the type `Noop`, hence it is defined here and
// not in the `detail` namespace above.
struct Noop {
void operator()(const auto&...) const {
// This function deliberately does nothing (static analysis expects a
Expand All @@ -118,16 +139,4 @@ struct Noop {
};
[[maybe_unused]] static constexpr Noop noop{};

template <typename T>
struct StaticCast {
constexpr decltype(auto) operator()(auto&& x) const {
return static_cast<T>(AD_FWD(x));
}
};

template <typename T>
static constexpr StaticCast<T> staticCast{};

} // namespace ad_utility

#endif // QLEVER_TRANSPARENTFUNCTORS_H

0 comments on commit b8e5803

Please sign in to comment.