Pass _caretPrototype to prevent cache miss by GaryQian · Pull Request #46720 · flutter/flutter

GaryQian · 2019-12-10T21:27:09Z

Description

Passing Rect.zero would cache the zero caretProto, which would cause future calls to perform redundant computation.

Pass the proper _caretPrototype to ensure the cached version is not discarded.

Related Issues

Fixes #24522

Tests

Added perf test.

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 Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

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: Replace with a link to your migration guide

Before:
"average_frame_build_time_millis": 3.5870540540540534,
"90th_percentile_frame_build_time_millis": 6.709,
"99th_percentile_frame_build_time_millis": 24.555,
"worst_frame_build_time_millis": 24.555,
"missed_frame_build_budget_count": 1,
"average_frame_rasterizer_time_millis": 6.6930000000000005,
"90th_percentile_frame_rasterizer_time_millis": 6.919,
"99th_percentile_frame_rasterizer_time_millis": 70.695,
"worst_frame_rasterizer_time_millis": 70.695,
"missed_frame_rasterizer_budget_count": 2,
"frame_count": 37,

After:
"average_frame_build_time_millis": 3.3834864864864858,
"90th_percentile_frame_build_time_millis": 4.756,
"99th_percentile_frame_build_time_millis": 24.9,
"worst_frame_build_time_millis": 24.9,
"missed_frame_build_budget_count": 1,
"average_frame_rasterizer_time_millis": 6.965729729729728,
"90th_percentile_frame_rasterizer_time_millis": 8.253,
"99th_percentile_frame_rasterizer_time_millis": 74.73,
"worst_frame_rasterizer_time_millis": 74.73,
"missed_frame_rasterizer_budget_count": 2,
"frame_count": 37,

fluttergithubbot · 2019-12-10T21:27:13Z

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.

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

GaryQian · 2019-12-12T23:47:09Z

This change does not really result in any testable changes, as passing Rect.zero vs _caretPrototype results in the same return values. The only difference is the internally cached values hitting/missing. I don't think this is worth adding a specific benchmark for.

GaryQian · 2019-12-12T23:48:33Z

packages/flutter/lib/src/rendering/editable.dart

    final Offset startOffset = _textPainter.getOffsetForCaret(
      TextPosition(offset: _selection.start, affinity: _selection.affinity),
-      Rect.zero,
+      _caretPrototype,


This particular usage does not care what is passed in for the caretPrototype parameter. We pass in the _caretPrototype to stay consistent with the other calls of this and not mess up the cached values, avoiding redundant re-computation.

GaryQian · 2019-12-12T23:50:10Z

packages/flutter/lib/src/painting/text_painter.dart

  void markNeedsLayout() {
    _paragraph = null;
    _needsLayout = true;
+    _previousCaretPosition = null;


clear cached values when marking as dirty.

justinmc

Looks like a solid performance improvement, good catch. Can you test it?

liyuqian · 2019-12-16T22:56:39Z

@GaryQian : a performance test for this would be nice (see *perf* in https://github.com/flutter/flutter/blob/master/dev/devicelab/manifest.yaml, where xyz_perf_abc in that yaml corresponds to a xyz_perf_abc.dart in our repo). More details of how to write a performance test is available in this doc.

GaryQian · 2019-12-18T22:48:41Z

Added a new perf test for textfields,

justinmc

LGTM

Thanks for doing all of the perf test work!

liyuqian

LGTM. Thank you Gary for the performance improvement and the test that guards it! Nit request: can you please put in the commit description the performance number of your test before and after your fix? Just in case that someone regressed it in the future, they'll have some historical records to dig up.

liyuqian · 2020-02-20T01:03:04Z

@GaryQian : the original issue (#24522) complained about the GPU thread frame rasterization time, but I only see big UI thread frame build time improvement in your experiment. Do you know why GPU thread frame rasterization time doesn't have a big difference?

A continuation of flutter#46720

anuaroa · 2020-05-20T09:04:24Z

Hi,
The version of installed Flutter is 1.17.1 (Flutter (Channel stable, v1.17.1, on Mac OS X 10.15.4 19E266, locale ru-KZ))
It's been written here https://flutter.dev/docs/development/tools/sdk/release-notes/changelogs/changelog-1.17.0 about #46720 issue, but the problem exists yet.

Pass caretProto to prevent cache miss

8c086f2

GaryQian added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. c: performance Relates to speed or footprint issues (see "perf:" labels) labels Dec 10, 2019

GaryQian self-assigned this Dec 10, 2019

googlebot added the cla: yes label Dec 10, 2019

GaryQian mentioned this pull request Dec 10, 2019

Caret performance is poor, high GPU time per frame. #24522

Closed

Mark dirty when need layout

d820435

GaryQian mentioned this pull request Dec 11, 2019

TextField widget is using a high cpu on iOS compared to native one #33833

Closed

GaryQian requested review from HansMuller and justinmc December 11, 2019 23:04

GaryQian commented Dec 12, 2019

View reviewed changes

justinmc reviewed Dec 13, 2019

View reviewed changes

liyuqian added the perf: speed Performance issues related to (mostly rendering) speed label Dec 16, 2019

GaryQian added this to the February 2020 milestone Dec 18, 2019

Add caret performance test

55de41f

GaryQian requested review from justinmc and liyuqian December 18, 2019 22:47

Remove initial delay to obtain worst times

c0745b8

fluttergithubbot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Dec 18, 2019

GaryQian added 2 commits December 18, 2019 16:32

Fix analyzer

933afd0

newline at EOF

239a104

justinmc approved these changes Dec 19, 2019

View reviewed changes

liyuqian approved these changes Dec 19, 2019

View reviewed changes

GaryQian merged commit b011ea5 into flutter:master Dec 19, 2019

liyuqian added a commit to liyuqian/flutter that referenced this pull request Mar 2, 2020

Add textfield perf test to devicelab

86c4bab

A continuation of flutter#46720

liyuqian mentioned this pull request Mar 2, 2020

Add textfield perf test to devicelab #51788

Merged

liyuqian mentioned this pull request Mar 16, 2020

Text painter layout cost more time when text content is longer Even if there is no difference in display #52651

Closed

iapicca mentioned this pull request Jun 12, 2020

Blinking cursor should use less CPU #59327

Open

CareF mentioned this pull request Jul 24, 2020

macrobenchmark/textfield_perf crashes in debug mode #62172

Closed

This was referenced Jul 21, 2021

Devicelab task tests not running anywhere #86802

Closed

Remove obsolete textfield_perf test #86827

Merged

github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass _caretPrototype to prevent cache miss#46720

Pass _caretPrototype to prevent cache miss#46720
GaryQian merged 6 commits intoflutter:masterfrom
GaryQian:caretPerf

GaryQian commented Dec 10, 2019 •

edited

Loading

Uh oh!

fluttergithubbot commented Dec 10, 2019

Uh oh!

GaryQian commented Dec 12, 2019

Uh oh!

GaryQian Dec 12, 2019 •

edited

Loading

Uh oh!

GaryQian Dec 12, 2019

Uh oh!

justinmc left a comment

Uh oh!

liyuqian commented Dec 16, 2019

Uh oh!

GaryQian commented Dec 18, 2019

Uh oh!

justinmc left a comment

Uh oh!

liyuqian left a comment

Uh oh!

liyuqian commented Feb 20, 2020

Uh oh!

anuaroa commented May 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

GaryQian commented Dec 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

fluttergithubbot commented Dec 10, 2019

Uh oh!

GaryQian commented Dec 12, 2019

Uh oh!

GaryQian Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GaryQian Dec 12, 2019

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

liyuqian commented Dec 16, 2019

Uh oh!

GaryQian commented Dec 18, 2019

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

liyuqian left a comment

Choose a reason for hiding this comment

Uh oh!

liyuqian commented Feb 20, 2020

Uh oh!

anuaroa commented May 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

GaryQian commented Dec 10, 2019 •

edited

Loading

GaryQian Dec 12, 2019 •

edited

Loading