Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[web] Improve dom canvas speed. Fix density calculation regression in recomputeTransformClip#22328

Merged
ferhatb merged 3 commits intoflutter:masterfrom
ferhatb:perf1
Nov 5, 2020
Merged

[web] Improve dom canvas speed. Fix density calculation regression in recomputeTransformClip#22328
ferhatb merged 3 commits intoflutter:masterfrom
ferhatb:perf1

Conversation

@ferhatb
Copy link
Contributor

@ferhatb ferhatb commented Nov 5, 2020

Description

  • Dom canvas now uses flt-picture instead of creating extra dom-canvas node.
  • computeDensity now uses isIdentityOrTranslation() to fix regression in update_many_child_layers benchmark.
═════════════════════════╡ ••• Final A/B results ••• ╞══════════════════════════
Score	Average A (noise)	Average B (noise)	Speed-up
bench_update_many_child_layers.html.drawFrameDuration.average	8641.60 (1.38%)	2418.36 (2.71%)	3.57x	
bench_update_many_child_layers.html.drawFrameDuration.outlierRatio	1.80 (5.63%)	1.10 (1.48%)	1.64x
bench_update_many_child_layers.html.totalUiFrame.average	28969.70 (0.66%)	9189.70 (1.25%)	3.15x
  • Micro optimizations around global final domRenderer that does extra lazyinit lookup.
  • Micro optimizations around style setting:
Before:
    apply$0: function() {
      var t1 = this.rootElement.style,
        t2 = "translate(" + H.S(this.dx) + "px, " + H.S(this.dy) + "px)";
      t1.toString;
      C.CssStyleDeclaration_methods._setPropertyHelper$3(t1, C.CssStyleDeclaration_methods._browserPropertyName$1(t1, "transform"), t2, "");
    },
After:
 apply$0: function() {
      var t2,
        t1 = this.rootElement;
      t1.toString;
      t2 = "translate(" + H.S(this.dx) + "px, " + H.S(this.dy) + "px)";
      t1.style.transform = t2;
    },

Related Issues

flutter/flutter#69910
flutter/flutter#59404

Tests

Covered by web_benchmarks.dart.

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].

@ferhatb ferhatb requested a review from yjbanov November 5, 2020 19:43
@google-cla google-cla bot added the cla: yes label Nov 5, 2020
Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

bool isIdentityOrTranslation() =>
_m4storage[15] == 1.0 &&
_m4storage[0] == 1.0 // col 1
&&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look right. Maybe move && before // col 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

_m4storage[2] == 0.0 &&
_m4storage[3] == 0.0 &&
_m4storage[4] == 0.0 // col 2
&&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

_m4storage[6] == 0.0 &&
_m4storage[7] == 0.0 &&
_m4storage[8] == 0.0 // col 3
&&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@ferhatb ferhatb merged commit 39f9855 into flutter:master Nov 5, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 6, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 6, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 6, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 6, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 6, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 6, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 6, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 6, 2020
chaselatta pushed a commit to chaselatta/engine that referenced this pull request Nov 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants