I am working on a legacy application with a class named Point3D as shown below...

template <class T>
class Point3D final
{
    T values[3];
public:
    Point3D();
    Point3D(T x, T y, T z);
    explicit Point3D(const T value);
    Point3D(const Point3D& point);
    explicit Point3D(const Point2D<T>& point);
    ~Point3D();
};

This class is getting used in many places as vector<Point3D<double>>. The size of the vector is > 10^5. Also, the legacy code passes this vector as a value and returns by value. This is making the application very slow. Also in many places, we have similar codes as shown below...

for(auto i: n){ // This loop runs for 10^4 times
    Point3D<double> vpos;

    vpos[X_COORDINATE] = /*Some calculation*/;
    vpos[Y_COORDINATE] = /*Some calculation*/;
    vpos[Z_COORDINATE] = /*Some calculation*/;

    Positions.push_back(vpos);
}

To improve the performance, I plan to modify the Point3D class to use move semantics as shown below...

template <class T> 
class Point3D final {
    T* values;
public:
    Point3D() {
        values = new T[3];
    }
    Point3D(T x, T y, T z) {
        values = new T[3];
    }
    explicit Point3D(const T value) {
        values = new T[3];
        
    }
    Point3D(const Point3D& point) {
        values = new T[3];
    }
    ~Point3D() {
        delete[] values;
    }

    T  operator [] (qint64 coordinate) const { return values[coordinate]; }
    T& operator [] (qint64 coordinate) { return values[coordinate]; }

    Point3D& operator = (const Point3D& point) {
        ...
        return *this;
    }

    Point3D(Point3D&& other) noexcept : values(other.values)
    {
        other.values = nullptr;
    }

    Point3D& operator=(Point3D&& other) noexcept
    {
        using std::swap;
        swap(*this, other);
        return *this;
    }
};

I am new to move semantics, please let me know any other ways to improve the performance. Thanks

CodePudding user response：

Also, the legacy code passes this vector as a value and returns by value. This is making the application very slow.

Your suggested modifications will make this worse in general. The cost of copying or moving your original Point3D is very small if T is a simple type like double. It will just copy three double values. If T is a complex type, then as the other answer suggests, removing the declared destructor and copy constructor will cause move semantics to be implemented correctly automatically. (This is also known as the "rule-of-zero", letting all special member functions be implemented implicitly.)

If your problem are many copies of a vector of the type, move semantics on the type will however not help you at all. Copying the vector will still require copying all elements. Moving will not be sufficient.

You need to adjust your code at the use site of these vectors. For example, if you are passing a vector like this by-value:

void f(vector<Point3D<double>>);

void g() {
    vector<Point3D<double>> v;
    // fill v
    f(v);
}

Then this will make an unnecessary copy of the vector. It is unnecessary because v's data is not required anymore after the call to f in g. So it can be moved instead of copied into the function:

void g() {
    vector<Point3D<double>> v;
    // fill v
    f(std::move(v));
}

This will move the vector, not the Point3D<double>'s. Moving a vector is a constant time operation, no matter whether or not move semantics are implemented for the element type. It won't copy or move any element.

If however v's state is used in g after the call, then you can't use move semantics directly, because move semantics will cause the state to be lost. In that case you need to rethink the design of the function and consider e.g. passing by const-reference instead.

(Note however that you should not use std::move if you return the vector. E.g. return std::move(v); should be return v; instead. Move is implicit on return statements that just return a local variable directly by name and doing the implicit std::move call will inhibit copy elision which in certain situations will eliminate copy/moves completely.)

CodePudding user response：

The move constructor and assignment of std::vector don't care about the contained type, so you don't need to change your class at all. What you need to do is fix your use of std::vector.

You don't need to use dynamic allocations here, and you can gain move semantics by removing code. However there is nothing to gain from moving doubles, you would only benefit using Point3D<std::string> or similar.

template <class T>
class Point3D final
{
    T values[3];
public:
    Point3D();
    Point3D(T x, T y, T z);
    explicit Point3D(const T value);
    explicit Point3D(const Point2D<T>& point);
};

Because you had specified a copy constructor, no move constructor was defaulted for your type. The defaults would do the correct thing, so you don't need to specify the special member functions.

CodePudding user response：

Firstly in the example you showed you can simply replace the push_back by an emplace_back as follows:

for(auto i : n)
{ // This loop runs for 10^4 times
    auto x_coord = /*Some calculation*/;
    auto y_coord = /*Some calculation*/;
    auto z_coord = /*Some calculation*/;

    Positions.emplace_back(x_coord,y_coord,z_coord);
}

This constructs the Point3D instance in place so no move or copy has to be done.

I would suggest you to use std::array instead of a c style array for more modern c . Also then you can use the default move and copy constructor and do not have to explicitly move or copy every element in the array. The Point3D class could then look as follows:

#include <array>

template <typename T> 
class Point3D  
{ 
     std::array<T,3> values;
public:
    Point3D()= default;
    Point3D(const T& x, const T& y, const T& z):
    values{x,y,z}
    {}

    // Rest of Class ...
    
};