Optimization suggestion 2

[stevemessick]

[user feedback]

Dart2js: arg.storage is a final field with a Float32Array. Why is it accessed multiple times? Should it be sufficient to call arg.get$storage() once?

 t1 = new Float32Array(9);
        t2 = this.storage;
        t3 = t2[0];
        t4 = arg.get$storage();
        if (0 >= t4.length)
          return H.ioore(t4, 0);
        t4 = t4[0];
        t5 = t2[3];
        t6 = arg.get$storage();
        if (1 >= t6.length)
          return H.ioore(t6, 1);
        t6 = t6[1];
        t7 = t2[6];
        t8 = arg.get$storage();
.......
////////////////////////////////////////////////////////////////////////////////////
Editor: 1.3.0.dev_03_02 (2014-03-10)
OS: Windows 8 - amd64 (6.2)
JVM: 1.7.0_45

projects: 2

open dart files: 0

auto-run pub: true
localhost resolves to: 127.0.0.1
mem max/total/free: 1778 / 853 / 528 MB
thread count: 29
index: 778380 relationships in 128899 keys in 1268 sources

SDK installed: true
Dartium installed: true

[floitschG]

cc @herhut-ggl.
cc @rakudrama.
Removed the owner.
Added Optimization label.

[rakudrama]

I suspect that the type of 'arg' is unknown.
There are several classes with a 'storage' getter (Vector2, Matrix3 etc) which all return a Float32List.
We could detect that all the get$storage methods are getters for final fields and thus always return the same value.

There will always be cases where the compiler can't quite figure out what the programmer knows is true. The programmer can help the compiler generate better code by manually doing common subexpression elimination.

For example, instead of
  ... arg.storage[0]
  ... arg.storage[1]

Write
  var arg_storage = arg.storage;
  ... arg_storage[0]
  ... arg_storage[1]

It is also worth while making 'storage' private. That way some other library that also has a storage field will not confuse the type analysis from inferring 'get$storage returns Float32List' to inferring 'get$storage returns union(Float32List, SomeOtherClass)' or worse.

[DartBot]

This comment was originally written by @Fox32

---

Thanks for your tips!

I took your suggestions and put them into vector_math: google/vector_math.dart#63 (comment)

1. Only call the getter once
2. Make storage private with a public getter and always use the private variable instead
3. Added type checks to operators
4. Rename the private field _storage to a unique name for every class (Matrix4._storage44, Matrix3._storage33)

All these changes reduced the type and size checks of the storage a lot. At many places I could reach the code I would write myself.

Any other suggestions I can take into account? Maybe a guide or howto for dartlang.com would be nice, so others could benefit from this tips, too.

[rakudrama]

It might be worth backing out the "3. Added type checks to operators".
The is-check is more expensive that calling "other.get$_storage44()", but since only one class in that library has a _storage44 field, dart2js knows "other" must be a Matrix4 after get$_storage44 succeeds (unless there are classes implementing noSuchMethod)

Essentially, don't forget to profile the results too.

[DartBot]

This comment was originally written by @Fox32

---

You are right the type checks in the operators where not necessary anymore. Thanks!

[rakudrama]

I had another idea that reduces garbage.

We already renamed the 'storage', and used temps to reduce the number of accesses:

class Vector2 implements Vector {
  final Float32List _storage2 = new Float32List(2);

  ...

  /// Set the values by copying them from [other].
  Vector2 setFrom(Vector2 other) {
     var other_storage2 = other._storage2;
    _storage2[1] = other_storage2[1];
    _storage2[0] = other_storage2[0];
    return this;
  }

The problem with this code is that 'setFrom' is not always inlined.
If it is not inlined, 'other' must be an allocated Vector2.
We can improve the chances of inlining by stratifying into Vector2 and Float32List levels,
where the Float32Level is encoded in private static methods that require no allocated Vector2.

  Vector2 setFrom(Vector2 other) {
    _setFrom(_storage2, other._storage2);
    // ^^ first argument is this._storage2, but don't write that as it currently looks bigger for inlining.
    return this;
  }

  static void _setFrom(Float32List first, Float32List second) {
    // ^^ types not necessary since they are inferred.
    first[0] = second[0];
    first[1] = second[1];
  }

Now if only setFrom is inlined, it might be that the compiler can see where other._storage is allocated and pass it directly to Vector2._setFrom and throw away Vector2 wrapper.

This is more important methods that stand no chance of being inlined, like normalize (the two return statements make it harder to inline).

  double get length {
    double sum;
    sum = (storage[0] * storage[0]);
    sum += (storage[1] * storage[1]);
    return Math.sqrt(sum);
  }

  Vector2 normalize() {
    double l = length;
     if (l == 0.0) {
      return this;
    }
    l = 1.0 / l;
    storage[0] *= l;
    storage[1] *= l;
    return this;
  }

-->

  double get length {
    return _length(_storage2);
  }

  static double _length(/Float32List/ storage) {
    double sum;
    sum = (storage[0] * storage[0]);
    sum += (storage[1] * storage[1]);
    return Math.sqrt(sum);
  }

  Vector2 normalize() {
    _normalize(_storage2);
    return this;
  }

  static _normalize(/Float32List/ storage) {
    double l = _length(storage);
     if (l == 0.0) {
      return;
    }
    l = 1.0 / l;
    storage[0] *= l;
    storage[1] *= l;
  }

I think getting pull requests accepted will be easier if each request does something that is clearly independent and easy to review, e.g. (a) rename Vector2.storage to Vector2._storage2 and nothing else (b) stratify Vector2 like I just described.
Don't mix this with changing the API - that is more controversial.

[DartBot]

This comment was originally written by @Fox32

---

Interesting suggestions, maybe I have to look into it again...