New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Working Pimpl-Version of Index #386
Conversation
The Index class is rather expensive to compile and needs Access to many datatypes that aren't needed elsewhere. This commit restructures the Index class to use the Pimpl (Pointer to implementation) idiom, s.t. we can change the Index implementation without retriggering the expensive compilation of all different Operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have left a comment in all the large
(aka copied and automatically renamed) files,
to make it easier for @hannahbast to find the places worth looking at.
#include <stxxl/vector> | ||
#include <vector> | ||
#include "../engine/ResultTable.h" | ||
#include "../engine/IdTable.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this file,
The signatures of all public methods of the original index class should be unchanged,
but only the declarations should be present here (The Pimpl-Idiom).
@@ -2,17 +2,19 @@ | |||
// Chair of Algorithms and Data Structures. | |||
// Author: Björn Buchhold (buchhold@informatik.uni-freiburg.de) | |||
|
|||
#include "./IndexImpl.h" | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This File was simply renamed (from Index to IndexImpl). Nothing else should be changed (I don't understand the whole text business anyway)
// Chair of Algorithms and Data Structures. | ||
// Author: Björn Buchhold (buchhold@informatik.uni-freiburg.de) | ||
|
||
#include "./IndexImpl.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the Original Index.cpp
file renamed to IndexImpl.cpp
Unfortunately github did not recognize this rename.
I have marked some places, where actual change happened.
template <typename F, typename... Args> | ||
auto IndexImpl::applyForPermutation(Permutation p, F f, Args&&... args) { | ||
auto fLifted = [&](const auto& permutationImpl) { | ||
return f(permutationImpl, std::forward<Args>(args)...); | ||
}; | ||
|
||
switch (p) { | ||
case Permutation::POS: | ||
return fLifted(POS()); | ||
case Permutation::PSO: | ||
return fLifted(PSO()); | ||
case Permutation::SOP: | ||
return fLifted(SOP()); | ||
case Permutation::SPO: | ||
return fLifted(SPO()); | ||
case Permutation::OSP: | ||
return fLifted(OSP()); | ||
case Permutation::OPS: | ||
return fLifted(OPS()); | ||
default: | ||
AD_CHECK(false); | ||
} | ||
} | ||
|
||
template <typename F, typename... Args> | ||
auto IndexImpl::applyForPermutation(Permutation p, F f, Args&&... args) const { | ||
auto fLifted = [&](const auto& permutationImpl) { | ||
return f(permutationImpl, std::forward<Args>(args)...); | ||
}; | ||
|
||
switch (p) { | ||
case Permutation::POS: | ||
return fLifted(POS()); | ||
case Permutation::PSO: | ||
return fLifted(PSO()); | ||
case Permutation::SOP: | ||
return fLifted(SOP()); | ||
case Permutation::SPO: | ||
return fLifted(SPO()); | ||
case Permutation::OSP: | ||
return fLifted(OSP()); | ||
case Permutation::OPS: | ||
return fLifted(OPS()); | ||
default: | ||
AD_CHECK(false); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is new.
// ___________________________________________________________________________ | ||
template <PermutationConcept P> | ||
void IndexImpl::scanImpl(Id key, IdTable* result, const P& p, | ||
ad_utility::SharedConcurrentTimeoutTimer timer) const { | ||
if (p._meta.relationExists(key)) { | ||
const FullRelationMetaData& rmd = p._meta.getRmd(key)._rmdPairs; | ||
result->reserve(rmd.getNofElements() + 2); | ||
result->resize(rmd.getNofElements()); | ||
p._file.read(result->data(), rmd.getNofElements() * 2 * sizeof(Id), | ||
rmd._startFullIndex, std::move(timer)); | ||
} | ||
} | ||
|
||
// ___________________________________________________________________________ | ||
void IndexImpl::scan(Id key, IdTable* result, const Permutation& p, | ||
ad_utility::SharedConcurrentTimeoutTimer timer) const { | ||
auto f = [&](const auto& permutationImpl) { | ||
return scanImpl(key, result, permutationImpl, timer); | ||
}; | ||
return applyForPermutation(p, f); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scanImpl
is the previous scan
method,
And the new scan
method just does the Transition from the simple Permutation
enum,
to the different PermutationImpl
types
(similar for the two other overloads below)
// Copyright 2015, University of Freiburg, | ||
// Chair of Algorithms and Data Structures. | ||
// Author: Björn Buchhold (buchhold@informatik.uni-freiburg.de) | ||
#pragma once |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the old Index.h
, if you want to you can vimdiff
the two files somewhere locally,
Unfortunately this is not shown here.
Again I have marked the places where small actual changes happened.
// Converts the Permutation p to the corresponding PermutationImpl permImpl | ||
// (Permutation::POS -> POS()) and then calls f(permImpl, args...) | ||
template <typename F, typename... Args> | ||
auto applyForPermutation(Permutation p, F f, Args&&... args); | ||
|
||
// Const overload of applyForPermutation | ||
template <typename F, typename... Args> | ||
auto applyForPermutation(Permutation p, F f, Args&&... args) const; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are new.
void scan(Id key, IdTable* result, const Permutation& p, | ||
ad_utility::SharedConcurrentTimeoutTimer timer = nullptr) const; | ||
|
||
private: | ||
// _____________________________________________________________________ | ||
template <PermutationConcept P> | ||
void scanImpl(Id key, IdTable* result, const P& p, | ||
ad_utility::SharedConcurrentTimeoutTimer timer = nullptr) const; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern is new.
The scan
is the external interface (using a simple enum for the Permutation
,
and the scanImpl
does the actual work and gets a PermutationConcept
aka PermutationImpl::PermutationImpl<SomeTypes>
.
See the .cpp file for the details.
( there are two more scan
overloads below with the same pattern.
The Index class is rather expensive to compile and needs Access to many datatypes that aren't needed elsewhere.
This commit restructures the Index class to use the Pimpl (Pointer to implementation) idiom, s.t. we can change
the Index implementation without retriggering the expensive compilation of all different Operations.