Skip to content

PERF: Use FixedArray for table within BSplineInterpolationWeightFunction #2721

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.

Sign up for GitHub

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

Merged
hjmjohnson merged 1 commit into from
Sep 9, 2021
Merged

PERF: Use FixedArray for table within BSplineInterpolationWeightFunction #2721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions Modules/Core/Common/include/itkBSplineInterpolationWeightFunction.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "itkContinuousIndex.h"
#include "itkArray.h"
#include "itkArray2D.h"
#include "itkIndexRange.h"
#include "itkMath.h"

namespace itk
Expand Down Expand Up @@ -112,15 +113,21 @@ class ITK_TEMPLATE_EXPORT BSplineInterpolationWeightFunction
#endif

protected:
BSplineInterpolationWeightFunction();
BSplineInterpolationWeightFunction() = default;
~BSplineInterpolationWeightFunction() override = default;

private:
/** Lookup table type. */
using TableType = Array2D<unsigned int>;
using TableType = FixedArray<IndexType, NumberOfWeights>;

/** Table mapping linear offset to indices. */
TableType m_OffsetToIndexTable;
const TableType m_OffsetToIndexTable{ [] {
TableType table;
// Note: Copied the constexpr value `SupportSize` to a temporary, `SizeType{ SupportSize }`, to prevent a GCC
// (Ubuntu 7.5.0-3ubuntu1~18.04) link error, "undefined reference to `SupportSize`".
std::copy_n(ZeroBasedIndexRange<SpaceDimension>(SizeType{ SupportSize }).cbegin(), NumberOfWeights, table.begin());
Copy link
Contributor

@Leengit Leengit Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it isn't making sense to defined SupportSize outside the class declaration so that references and pointers to it are enabled -- perhaps because we don't want to allocate that memory just to support the reference -- similar can be accomplished via a local variable to this lambda.

const auto supportSize {Self::SupportSize};

As to const vs. constexpr here, I think the former might work out better -- the latter might be interpreted as a hint to the compiler that we don't want the memory allocated.

Copy link
Contributor Author

@N-Dekker N-Dekker Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Leengit Indeed, it appears that the link error could also be avoided by a local const auto supportSize within the lambda. But personally I don't really like to have an extra local variable. For now I think the syntax SizeType{ SupportSize } is just fine.

In general, it might become cumbersome to spell out the type name of the constexpr data member every time it is passed by reference. Instead maybe a helper function could be useful:

auto CopyToNonConstexprValue(T value) { return value; }

The value of the constexpr SupportSize could then be passed to a function like ProcessSize(const SizeType&), as follows:

ProcessSize( CopyToNonConstexprValue(SupportSize) );

Do you like that? 😺

Copy link
Contributor

@Leengit Leengit Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fear that it will give the same errors that we are already / still seeing in cdash. But if it doesn't then I like it.

Copy link
Contributor Author

@N-Dekker N-Dekker Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Leengit Try it out at https://godbolt.org/z/YGTh9q1Po 😃 As follows (using GCC 10.3):

struct SizeType {};

struct Wrapper
{
  constexpr static SizeType ConstexprValue{};
};

void ProcessSize(const SizeType&) {}

template <typename T>
auto CopyToNonConstexprValue(T value) { return value; }

int main()
{
    ProcessSize(CopyToNonConstexprValue(Wrapper::ConstexprValue)); // OK
    ProcessSize(Wrapper::ConstexprValue); // <== error: undefined reference
}

Copy link
Contributor Author

@N-Dekker N-Dekker Sep 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Leengit Related section from https://github.com/cplusplus/draft/releases/download/n4892/n4892.pdf (C++ Working Draft, 2021-06-18):

D.7 Redeclaration of static constexpr data members [depr.static.constexpr]

1 For compatibility with prior revisions of C++, a constexpr static data member may be redundantly redeclared
outside the class with no initializer. This usage is deprecated.
[Example 1:

struct A {
  static constexpr int n = 5; // definition (declaration in C++2014)
};
constexpr int A::n; // redundant declaration (definition in C++2014)

— end example]

Copy link
Contributor

@Leengit Leengit Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good idea. Is this for C++23? Do they have an alternative way of making references and pointers work for class static constexpr members, or are they saying that the class declaration needs to be sufficient?

return table;
}() };
};
} // end namespace itk

Expand Down
21 changes: 0 additions & 21 deletions Modules/Core/Common/include/itkBSplineInterpolationWeightFunction.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,30 +23,9 @@
#include "itkImage.h"
#include "itkMatrix.h"
#include "itkMath.h"
#include "itkIndexRange.h"

namespace itk
{
/** Constructor */
template <typename TCoordRep, unsigned int VSpaceDimension, unsigned int VSplineOrder>
BSplineInterpolationWeightFunction<TCoordRep, VSpaceDimension, VSplineOrder>::BSplineInterpolationWeightFunction()
{
// Initialize offset to index lookup table
m_OffsetToIndexTable.set_size(Self::NumberOfWeights, SpaceDimension);

constexpr auto supportSize = Self::SupportSize;
unsigned int counter = 0;

for (const IndexType index : ZeroBasedIndexRange<VSpaceDimension>(supportSize))
{
for (unsigned int j = 0; j < SpaceDimension; ++j)
{
m_OffsetToIndexTable[counter][j] = index[j];
}
++counter;
}
}

/** Compute weights for interpolation at continuous index position */
template <typename TCoordRep, unsigned int VSpaceDimension, unsigned int VSplineOrder>
typename BSplineInterpolationWeightFunction<TCoordRep, VSpaceDimension, VSplineOrder>::WeightsType
Expand Down