Optimize my code =)

[bearophile]

Robin:

> http://dpaste.dzfl.pl/3df8694eb47d

I think it's better to improve your code iteratively. First 
starting with small simple things:


>    this(size_t rows, size_t cols) nothrow pure {

=>

     this(in size_t rows, in size_t cols) nothrow pure {


>    this(ref const(Dimension) that) nothrow pure {

=>

     this(const ref Dimension that) nothrow pure {


>    ref Dimension opAssign(ref Dimension rhs) nothrow pure {

Is this method necessary?


>    bool opEquals(ref const(Dimension) other) nothrow const {

Not necessary.


>    size_t min() @property nothrow const {
>        if (this.rows <= this.cols) return this.rows;
>        else                        return this.cols;
>    }

=> (I have changed its name to reduce my confusion 
std.algorithm.min):

     @property size_t minSide() const pure nothrow {
         return (rows <= cols) ? rows : cols;
     }


>    size_t size() @property nothrow const {

=>

     @property size_t size() @property pure nothrow {


>    size_t offset(size_t row, size_t col) nothrow const {

=>

size_t offset(in size_t row, in size_t col) const pure nothrow {



>    this(in size_t rows, in size_t cols, bool initialize = true) 
> nothrow pure {
>        this.dim = Dimension(rows, cols);
>        this.data = 
> minimallyInitializedArray!(typeof(this.data))(rows * cols);
>        if (initialize) this.data[] = to!T(0);
>    }

=>

     this(in size_t rows, in size_t cols, in bool initialize = 
true) nothrow pure {
     ...
     if (initialize)
         this.data[] = 0;


>    /**
>     *
>     */

Sometimes I prefer to use just one line of code to reduce 
vertical wasted space:

/// Move constructor. From rvalue.


>    this(ref const(Matrix) that) {

=>

     this(const ref Matrix that) pure {


>        this.data = that.data.dup;

Currently array.dup is not nothrow. This will eventually be fixed.


>    ref Matrix transpose() const {
>        return Matrix(this).transposeAssign();

=>

     ref Matrix transpose() const pure nothrow {
         return Matrix(this).transposeAssign;


>    ref Matrix transposeAssign() nothrow {
>        size_t r, c;
>        for (r = 0; r < this.dim.rows; ++r) {
>            for (c = 0; c < r; ++c) {
>                std.algorithm.swap(
>                    this.data[this.dim.offset(r, c)],
>                    this.data[this.dim.offset(c, r)]);
>            }
>        }
>        return this;
>    }

Use immutable foreach loops.
And even when you use for loops always define the loop variable 
inside the for: "for (size_t r = 0; ...".
Also in general I suggest you to write a little less code.

A first try (not tested), but probably this can be done with much 
less multiplications:

     ref Matrix transposeAssign() pure nothrow {
         foreach (immutable r; 0 .. dim.rows) {
             foreach (immutable c; 0 .. r) {
                 swap(data[dim.offset(r, c)], data[dim.offset(c, 
r)]);
             }
         }
         return this;
     }


>    Matrix opMul(ref const(Matrix) other)

Never use old-style operator overloading in new D code. Use only 
the new operator overloading style.


>        auto s = Matrix(other).transposeAssign();

=>

>        auto s = Matrix(other).transposeAssign;


>        size_t r1, r2, c;

Ditto.


>        foreach (ref T element; this.data) {
>            element *= scalar;
>        }

Try to use:

         data[] *= scalar;


>        for (auto i = 0; i < this.dim.size; ++i) {
>            this.data[i] += other.data[i];
>        }

Try to use:

         data[] += other.data[];


>        for (auto i = 0; i < this.dim.size; ++i) {
>            this.data[i] -= other.data[i];
>        }

Try to use:

         data[] -= other.data[];


>    bool opEquals(ref const(Matrix) other) nothrow const {
>        if (this.dim != other.dim) {
>            return false;
>        }
>        for (auto i = 0; i < this.dim.size; ++i) {
>            if (this.data[i] != other.data[i]) return false;
>        }
>        return true;
>    }

Try (in general try to write less code):

     return dim == other.dim && data == other.data;



>        auto stringBuilder = std.array.appender!string;

=>

         Appender!string result;


>        stringBuilder.put("Matrix: "
>            ~ to!string(this.dim.rows) ~ " x " ~ 
> to!string(this.dim.cols)

=>

         result ~= text("Matrix: ", dim.rows, " x ", dim.cols


>        auto m = Matrix(rows, cols);
>        for (auto i = 0; i < m.dim.min; ++i) {
>            m[i, i] = 1;
>        }

See the comment by John Colvin.


>        auto generator = Random(unpredictableSeed);

What if I want to fill the matrix deterministically, with a given 
seed? In general it's better to not add a random matrix method to 
your matrix struct. Better to add an external free function that 
uses UFCS and is more flexible and simpler to reason about.



>    writeln("\tTime required: " ~ to!string(secs) ~ " secs, " ~ 
> to!string(msecs) ~ " msecs");

Use .text to strinify, but here you don't need to stringify at 
all:

=>

     writeln("\tTime required: ", secs, " secs, ", msecs, " 
msecs");


>    sw.reset();

=>
     sw.reset;


> void multiplicationTest() {

In D there is also unittest {}. A benchmark is not exactly an 
unit test, so do as you prefer.

Bye,
bearophile