[web] Optimize Matrix4.identity

[ferhatb]

Description

Changes Matrix.identity from factory constructor calling zero().setIdentity to named ctor only setting 4 members on matrix.

Compiled JS Code Before:

Call site:
t2 = new E.Matrix4(new Float64Array(16));
t2.setIdentity$0();

Impl:
setIdentity$0: function() {
      var t1 = this.__engine$_m4storage;
      t1[0] = 1;
      t1[1] = 0;
      t1[2] = 0;
      t1[3] = 0;
      t1[4] = 0;
      t1[5] = 1;
      t1[6] = 0;
      t1[7] = 0;
      t1[8] = 0;
      t1[9] = 0;
      t1[10] = 1;
      t1[11] = 0;
      t1[12] = 0;
      t1[13] = 0;
      t1[14] = 0;
      t1[15] = 1;
    },

Code After :

Call site:
t2 = H.Matrix4$identity()

Impl:
Matrix4$identity: function() {
  var t1 = new Float32Array(16);
  t1[15] = 1;
  t1[0] = 1;
  t1[5] = 1;
  t1[10] = 1;
}

Related Issues

First PR for flutter/flutter#70888.

Tests

Covered by benchmarks.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

• I read the [contributor guide] and followed the process outlined there for submitting PRs.
• I signed the [CLA].
• I read and followed the [C++, Objective-C, Java style guides] for the engine.
• I read the [tree hygiene] wiki page, which explains my responsibilities.
• I updated/added relevant documentation.
• All existing and new tests are passing.
• I am willing to follow-up on review comments in a timely manner.

Reviewer Checklist

• I have submitted any presubmit flakes in this PR using the [engine presubmit flakes form] before re-triggering the failure.

Breaking Change

Did any tests fail when you ran them? Please read [handling breaking changes].

• No, no existing tests failed, so this is not a breaking change.
• Yes, this is a breaking change. If not, delete the remainder of this section.
  • I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
  • I got input from the developer relations team, specifically from: Replace with the names of who gave advice
  • I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[ferhatb]

/cc @dnfield , not sure what the native code looks like for this... Next i'm going to change named default params to use kIdentityMatrix with assert(kIdentityMatrix.isIdentity()) to make sure nobody mutates.

[dnfield]

@mraleph is probably better for the native code side than I am, or at least would know someone who can look at this.

[dnfield]

We should probably look to apply this change upstream in package:vector_math right? That way the framework will benefit as well (or is the framework using this copy?)

[ferhatb]

Due to dart:ui not being able import any package, yes its a copy. You are correct, we should upstream.

…

On Thu, Nov 19, 2020 at 4:54 PM Dan Field ***@***.***> wrote: We should probably look to apply this change upstream in package:vector_math right? That way the framework will benefit as well (or is the framework using this copy?) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#22622 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGTTKRUKPD23UUG6WEYGZLSQW46FANCNFSM4T4CQCKQ> .

[mraleph]

VM does not do much, we just emit allocation and setIdentity call. We can mark setIdentity with @pragma('vm:prefer-inline') and a small tweak into our store/load-forwarding pass (we don't seem to forward initial 0 values for typed data - unlike for normal arrays where we forward initial null value so stores putting null into newly allocated array disappear) to achieve the same type of code automatically. Is this on a hot path in some benchmark? /cc @alexmarkov

[nturgut]

thanks for the optimization! Shall we also add a comment about the compiler optimization we are doing(why are we accessing the 15th index before all the others)?

[ferhatb]

Done.

[nturgut]

LGTM.

[yjbanov]