P3M cleanup of ParticleRanges

[jhossbach]

Continuation of #4721.

Description of changes:

• I continued on moving ParticleRanges out of function signatures and parsing combined_ranges instead.
• For charge_assign I had to move the definition to the .hpp file to avoid specifying the type of these boost ranges.
• I also removed the unused kernel method in p3m.hpp that probably was forgotten during some refactoring

Moving the construction of the new boost ranges would require to change the signature of long_range_kernel, which would require to touch long_range_forces, which would require bigger refactoring.
Through discussions with Rudolf the idea of an P3MImpl class to abstract implementation from the espresso implementation, as well as a base class that contains these ranges came up. I am not sure if this should be part of the PR, what do you think @jngrad?

[jngrad]

Moving the construction of the new boost ranges would require to change the signature of long_range_kernel, which would require to touch long_range_forces, which would require bigger refactoring.

The charge_assign() calls are being moved from the long-range kernel visitors in coulomb.cpp to p3m.cpp in #4820. Once @RudolfWeeber and I are done with the propagator refactoring, you will be able to move the boost zip code back into its original place (which you temporarily commented out).

[jngrad]

The PR was rebased on top of the python branch, which brought a lot of improvements regarding separation of concerns with call to long-range energy/force/pressure kernels.

As I understand it, we still haven't made P3M fully independent of ESPResSo implementation details. The next step would be to remove all ParticleRange from the signature of CoulombP3M class methods. For example, we would have to change the following signature:

void charge_assign(ParticleRange const &particles);

to a signature that takes a boost zip iterator of the particle positions and electrical charges. We cannot make this method a templated method that takes any type of zip iterator, because this would force us to move the method definition to the header file, and thus expose a lot of the internal logic. Alternatively, we could provide the full type of the boost zip iterator in the function signature, like so:

void charge_assign(
 decltype(boost::combine(
  std::declval<std::invoke_result_t<decltype(ParticlePropertyRange::q_range),ParticleRange const &>>(),
  std::declval<std::invoke_result_t<decltype(ParticlePropertyRange::pos_range),ParticleRange const &>>())
 ) const &p_q_pos_range);

Not only is this a mouthful, but it also contains the ParticleRange type, which is what we are trying to avoid. Here is the pretty printed demangled symbol for that class method:

void CoulombP3M::charge_assign(boost::range::combined_range<
  boost::tuples::tuple<
    boost::iterators::transform_iterator<ParticlePropertyRange::charge_range(const ParticleRange&)::<lambda(Particle&)>, ParticleIterator<Cell* const*>, boost::use_default, boost::use_default>,
    boost::iterators::transform_iterator<ParticlePropertyRange::pos_range(const ParticleRange&)::<lambda(Particle&)>, ParticleIterator<Cell* const*>, boost::use_default, boost::use_default>,
    boost::tuples::null_type, boost::tuples::null_type, boost::tuples::null_type, boost::tuples::null_type,
    boost::tuples::null_type, boost::tuples::null_type, boost::tuples::null_type, boost::tuples::null_type
  >
> const &)

I'm not completely sure if it's safe to use decltype on an expression containing a temporary lambda. Here a transform iterator that stores a lambda would be created in one translation unit and sent by reference to another translation unit, where it would be executed. We could rewrite those as structs, if need be.

Since we want to hide the ParticleRange type, we need to use some form of type erasure. We are working with ranges instead of single values, so we need a type-erased iterator.

As a quick refresher, here is what type erasure looks like:

struct TypeErasedRange {
  template <typename Range>
  TypeErasedRange(Range &range) : m_data{new Impl<Range>(range)} {}
  struct Base {};

private:
  template <typename Range> struct Impl : Base {
    Impl(Range &range) : m_begin(range.begin()), m_end(range.end()) {}

  private:
    typename Range::iterator m_begin;
    typename Range::iterator m_end;
  };
  std::unique_ptr<Base> m_data;
};
int main() {
  std::vector<int> vec = {1, 2, 3};
  TypeErasedRange erased_vec{vec};
}

Here erased_vec stores the begin and end iterators of the vector of integers, but has the type TypeErasedRange. Right now this class can't do anything with the data. To access the data, the Base class needs to be populated with virtual methods that are overridden in the Impl<Range> class. We need:

• a way to increment the beginning iterator (to move forward in the sequence),
• a way to dereference it (to access the data, and possibly modify it on-the-fly to e.g. fold particle positions),
• a way to compare the current iterator with the past-the-end iterator (to know when to stop the range-based for loop).

With these 3 functions, we fulfill the minimal requirements of a LegacyForwardIterator. There is a lot of boilerplate code to completely implement the iterator, but this can be done automatically with a boost iterator facade.

A minimal working example can be found at the bottom of this message. With it, the boost combine symbol that originally contained a tuple of boost::iterators::transform_iterator<ParticlePropertyRange...> types contains instead a tuple of TypeErasedIterator<double> types, i.e. the underlying Particle type is fully erased. Yet this solution is not totally satisfactory, because each loop iteration needs to carry out an incrementation, a dereference and a comparison, all of which come with a virtual dispatch penalty. The assembly code generated by the MWE with -O3 -DNDEBUG looks also extremely large compared to what I would expect for a loop over a vector of integers... There is probably an issue with my code.

It is not immediately obvious to me, how to avoid that cost. We cannot increment the pointer by adding the size of the pointee (e.g., sizeof(Utils::Vector3d)), because we actually iterate through Particle objects, and we can't pass sizeof(Particle) as a template argument, because the Particle objects are not stored contiguously in memory. In fact, ParticleRange is itself an iterator facade that loops through cells. The solution below also doesn't allow transformation of the data on-the-fly, because it cannot return a lvalue pointer from a rvalue pointee. Maybe we could store the transformed value in a data member of the facade to make it a lvalue?

@RudolfWeeber before I invest more time into this, please let me know if you think this is the right approach. Maybe I overlooked a more suitable design pattern.

MWE:

#include <boost/iterator/iterator_facade.hpp>
#include <boost/range/combine.hpp>
#include <boost/range/iterator_range.hpp>
#include <memory>
#include <utility>
#include <vector>

template <typename ValueType>
class TypeErasedIterator
    : public boost::iterator_facade<TypeErasedIterator<ValueType>, ValueType,
                                    boost::forward_traversal_tag> {
public:
  template <typename Range>
  TypeErasedIterator(Range &range, int flag)
      : m_data{new Impl<Range>(range, flag)} {}

private:
  friend class boost::iterator_core_access;

  void increment() { m_data->increment(); };

  bool equal(TypeErasedIterator<ValueType> const &other) const {
    return pos() == other.pos();
  };

  auto &dereference() const { return *pos(); };

  ValueType *begin() const { return m_data->begin(); }
  ValueType *end() const { return m_data->end(); }
  ValueType *pos() const { return m_data->pos(); }

  struct Base {
    virtual ValueType *begin() const = 0;
    virtual ValueType *end() const = 0;
    virtual ValueType *pos() const = 0;
    virtual void increment() = 0;
    virtual ~Base() = default;
  };

private:
  template <typename Range> struct Impl : Base {
    Impl(Range &range, int flag)
        : m_begin(range.begin()),
          m_end(range.end()), m_pos{(flag == 1) ? range.begin() : range.end()} {
    }
    ~Impl() override = default;

  private:
    typename Range::iterator m_begin;
    typename Range::iterator m_end;
    typename Range::iterator m_pos;
    ValueType *pos() const override { return &*m_pos; };
    ValueType *begin() const override { return &*m_begin; };
    ValueType *end() const override { return &*m_end; };
    void increment() override { ++m_pos; };
  };
  std::shared_ptr<Base> m_data;
};

template <typename T>
class TypeErasedRange : public boost::iterator_range<TypeErasedIterator<T>> {
  using base_type = boost::iterator_range<TypeErasedIterator<T>>;

public:
  using base_type::base_type;
  typename base_type::size_type size() const {
    return std::distance(base_type::begin(), base_type::end());
  }
  template <typename Range>
  TypeErasedRange(Range &&range)
      : boost::iterator_range<TypeErasedIterator<T>>{
            TypeErasedIterator<T>(range, 1), TypeErasedIterator<T>(range, 2)} {}
};

template <typename T> void foor(T range);

#include <iostream>
int main() {
  std::vector<int> vec = {1, 2, 3};
  std::ignore = boost::combine(vec, TypeErasedRange<int>(vec));
  for (auto const val : TypeErasedRange<int>(vec)) {
    std::cout << val << "\n";
  }
}

[jngrad]

We could also pre-compute std::vector of double and Vector3d for inputs (charges, positions) abd Vector3d* for outputs (forces) and pass them to the P3M kernels. Since some kernels have optional inputs (only needed when a charge assignment is pending), we would have to extract the charge assignment logic to a new method and give the user a way to query the pending flag.