Get rid of _NullElement. #146741

polina-c · 2024-04-14T20:36:21Z

flutter-dashboard · 2024-04-16T01:26:21Z

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 "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

polina-c · 2024-04-16T01:27:44Z

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 "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

Test covered in flutter_test_config.dart

goderbauer

LGTM

goderbauer · 2024-04-16T17:08:04Z

You'll need to do what the bot says to get a test exception, though.

Hixie · 2024-04-16T20:24:51Z

Have we checked the performance implications of this change? The original code was intended to avoid overhead in list generation and the new code has a lot more function calls.

polina-c · 2024-04-16T20:52:04Z

Have we checked the performance implications of this change? The original code was intended to avoid overhead in list generation and the new code has a lot more function calls.

From my perspective the new code has better performance.
The old code did: (1) list generation with pitting stub to every cell and (2) updating each value in the list with one more full scan, so looping two times total.
New code is looping through items just one time without doing more for each item.

And I expect flutter performance banchmarks to catch the issue if i missed something.

If it is not convincing enough, what will be convincing for you?

Hixie · 2024-04-17T00:27:30Z

The only way to tell is to benchmark it.

polina-c · 2024-04-17T18:49:47Z

Measured timing. On mac performance is improved 4 times if number of tabs is 30.

Code:

class Element {}

final stub = Element();
const numberOfTabs = 30;
const repeat = 1000000;

Element generateItem() => Element();

List<Element> oldCode() {
  final result = List<Element>.filled(numberOfTabs, stub);

  for (int i = 0; i < result.length; i += 1) {
    final Element newChild = generateItem();
    result[i] = newChild;
  }
  return result;
}

List<Element> newCode() {
  return List<Element>.generate(numberOfTabs, (int i) => generateItem(),
      growable: false);
}

void main() {
  late List<Element> list;
  
  Stopwatch oldTime = Stopwatch()..start();
  for (int i = 0; i < repeat; i ++) {
    list = oldCode();
  }
  oldTime.stop();
  print('old: ${oldTime.elapsedMilliseconds} ms');
  
  Stopwatch newTime = Stopwatch()..start();
  for (int i = 0; i < repeat; i ++) {
    list = newCode();
  }
  newTime.stop();
  print('new: ${newTime.elapsedMilliseconds} ms');
  
  
  print('length: ${list.length}');
}

Output:

old: 471 ms
new: 127 ms
length: 30

Hixie · 2024-04-17T19:04:36Z

nice!

Hixie

@main

Roll Flutter from 3882afb to fb110b9 (56 revisions) flutter/flutter@3882afb...fb110b9 2024-04-18 [email protected] Fix memory leaks in `MaterialBanner` (flutter/flutter#146963) 2024-04-18 [email protected] Fix memory leak in data table (flutter/flutter#146892) 2024-04-18 [email protected] Dispose the curved animation in transition test (flutter/flutter#146961) 2024-04-18 [email protected] Roll Flutter Engine from 5687011d5e94 to 2c3e9c8bfce6 (1 revision) (flutter/flutter#146982) 2024-04-18 [email protected] Roll Flutter Engine from 132d364ef373 to 5687011d5e94 (2 revisions) (flutter/flutter#146972) 2024-04-18 [email protected] Roll Flutter Engine from 20638b75c8c4 to 132d364ef373 (1 revision) (flutter/flutter#146969) 2024-04-18 [email protected] [macOS] Migrate @NSApplicationMain attribute to @main (flutter/flutter#146848) 2024-04-18 [email protected] Roll Flutter Engine from 07f7532ecd29 to 20638b75c8c4 (2 revisions) (flutter/flutter#146962) 2024-04-18 [email protected] Roll Flutter Engine from 725ebd708246 to 07f7532ecd29 (1 revision) (flutter/flutter#146960) 2024-04-18 [email protected] Roll Flutter Engine from 624730fb8840 to 725ebd708246 (4 revisions) (flutter/flutter#146959) 2024-04-17 [email protected] Roll Flutter Engine from 9edcb26f5949 to 624730fb8840 (2 revisions) (flutter/flutter#146955) 2024-04-17 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.0 to 3.25.1 (flutter/flutter#146953) 2024-04-17 49699333+dependabot[bot]@users.noreply.github.com Bump peter-evans/create-pull-request from 6.0.3 to 6.0.4 (flutter/flutter#146954) 2024-04-17 [email protected] Roll Flutter Engine from cc73c5c124c4 to 9edcb26f5949 (4 revisions) (flutter/flutter#146946) 2024-04-17 [email protected] Bump meta to 1.14.0 (flutter/flutter#146925) 2024-04-17 [email protected] Make goldenFileComparator a field instead of a trivial property (flutter/flutter#146800) 2024-04-17 [email protected] Assert that the goldenFileComparator is a LocalFileComparator (flutter/flutter#146802) 2024-04-17 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Try to be more consistent about deleting test apps in devicelab logic (#146931)" (flutter/flutter#146947) 2024-04-17 [email protected] Create web tests suite & runner_utils (flutter/flutter#146592) 2024-04-17 [email protected] Roll Flutter Engine from 376c86e3bbb6 to cc73c5c124c4 (3 revisions) (flutter/flutter#146940) 2024-04-17 [email protected] Try to be more consistent about deleting test apps in devicelab logic (flutter/flutter#146931) 2024-04-17 [email protected] Roll Flutter Engine from bc6382e35365 to 376c86e3bbb6 (2 revisions) (flutter/flutter#146937) 2024-04-17 [email protected] Remove now-redundant tests for isForEnvironment (flutter/flutter#146804) 2024-04-17 [email protected] Roll pub packages (flutter/flutter#146929) 2024-04-17 [email protected] Get rid of _NullElement. (flutter/flutter#146741) 2024-04-17 [email protected] Roll Flutter Engine from 818191d11404 to bc6382e35365 (2 revisions) (flutter/flutter#146930) 2024-04-17 [email protected] Fix memory leak in paginated tables (flutter/flutter#146755) 2024-04-17 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Try to be more consistent about deleting test apps in devicelab logic. (#146856)" (flutter/flutter#146927) 2024-04-17 [email protected] Roll Flutter Engine from 25b09e883dd8 to 818191d11404 (3 revisions) (flutter/flutter#146898) 2024-04-17 [email protected] Try to be more consistent about deleting test apps in devicelab logic. (flutter/flutter#146856) 2024-04-17 [email protected] Unpin frontend_server_client and roll packages (flutter/flutter#146650) 2024-04-17 [email protected] Roll Packages from 90c876d to d39830e (4 revisions) (flutter/flutter#146895) 2024-04-17 [email protected] Roll Flutter Engine from 0e9e2f419225 to 25b09e883dd8 (1 revision) (flutter/flutter#146889) 2024-04-17 [email protected] Reland: Update link branches to `main` (flutter/flutter#146882) 2024-04-17 [email protected] Roll Flutter Engine from 4b949d7d6065 to 0e9e2f419225 (4 revisions) (flutter/flutter#146884) 2024-04-17 [email protected] Revert "Update link branches to `main`" (flutter/flutter#146880) 2024-04-17 [email protected] Fix `Tab` indicator image configuration doesn't inherit device pixel ratio (flutter/flutter#146812) 2024-04-17 [email protected] Roll Flutter Engine from 1ea39b29ce8c to 4b949d7d6065 (1 revision) (flutter/flutter#146865) 2024-04-17 [email protected] Update link branches to `main` (flutter/flutter#146558) 2024-04-17 [email protected] Roll Flutter Engine from 45c86593a09a to 1ea39b29ce8c (1 revision) (flutter/flutter#146863) 2024-04-17 [email protected] Rewrap some lines that are about to get much longer. (flutter/flutter#146803) 2024-04-17 [email protected] Roll Flutter Engine from 0cdbc8a6e463 to 45c86593a09a (1 revision) (flutter/flutter#146862) 2024-04-17 [email protected] Roll Flutter Engine from 31b67985de27 to 0cdbc8a6e463 (1 revision) (flutter/flutter#146861) 2024-04-17 [email protected] Roll Flutter Engine from e7d8c62d291a to 31b67985de27 (1 revision) (flutter/flutter#146859) 2024-04-17 [email protected] Roll Flutter Engine from 4d69c0c95483 to e7d8c62d291a (11 revisions) (flutter/flutter#146857) 2024-04-16 [email protected] Switch to relevant `Remote` constructors (flutter/flutter#146773) ...

Fixes flutter#145602

@main

Roll Flutter from 3882afb to fb110b9 (56 revisions) flutter/flutter@3882afb...fb110b9 2024-04-18 [email protected] Fix memory leaks in `MaterialBanner` (flutter/flutter#146963) 2024-04-18 [email protected] Fix memory leak in data table (flutter/flutter#146892) 2024-04-18 [email protected] Dispose the curved animation in transition test (flutter/flutter#146961) 2024-04-18 [email protected] Roll Flutter Engine from 5687011d5e94 to 2c3e9c8bfce6 (1 revision) (flutter/flutter#146982) 2024-04-18 [email protected] Roll Flutter Engine from 132d364ef373 to 5687011d5e94 (2 revisions) (flutter/flutter#146972) 2024-04-18 [email protected] Roll Flutter Engine from 20638b75c8c4 to 132d364ef373 (1 revision) (flutter/flutter#146969) 2024-04-18 [email protected] [macOS] Migrate @NSApplicationMain attribute to @main (flutter/flutter#146848) 2024-04-18 [email protected] Roll Flutter Engine from 07f7532ecd29 to 20638b75c8c4 (2 revisions) (flutter/flutter#146962) 2024-04-18 [email protected] Roll Flutter Engine from 725ebd708246 to 07f7532ecd29 (1 revision) (flutter/flutter#146960) 2024-04-18 [email protected] Roll Flutter Engine from 624730fb8840 to 725ebd708246 (4 revisions) (flutter/flutter#146959) 2024-04-17 [email protected] Roll Flutter Engine from 9edcb26f5949 to 624730fb8840 (2 revisions) (flutter/flutter#146955) 2024-04-17 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.0 to 3.25.1 (flutter/flutter#146953) 2024-04-17 49699333+dependabot[bot]@users.noreply.github.com Bump peter-evans/create-pull-request from 6.0.3 to 6.0.4 (flutter/flutter#146954) 2024-04-17 [email protected] Roll Flutter Engine from cc73c5c124c4 to 9edcb26f5949 (4 revisions) (flutter/flutter#146946) 2024-04-17 [email protected] Bump meta to 1.14.0 (flutter/flutter#146925) 2024-04-17 [email protected] Make goldenFileComparator a field instead of a trivial property (flutter/flutter#146800) 2024-04-17 [email protected] Assert that the goldenFileComparator is a LocalFileComparator (flutter/flutter#146802) 2024-04-17 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Try to be more consistent about deleting test apps in devicelab logic (#146931)" (flutter/flutter#146947) 2024-04-17 [email protected] Create web tests suite & runner_utils (flutter/flutter#146592) 2024-04-17 [email protected] Roll Flutter Engine from 376c86e3bbb6 to cc73c5c124c4 (3 revisions) (flutter/flutter#146940) 2024-04-17 [email protected] Try to be more consistent about deleting test apps in devicelab logic (flutter/flutter#146931) 2024-04-17 [email protected] Roll Flutter Engine from bc6382e35365 to 376c86e3bbb6 (2 revisions) (flutter/flutter#146937) 2024-04-17 [email protected] Remove now-redundant tests for isForEnvironment (flutter/flutter#146804) 2024-04-17 [email protected] Roll pub packages (flutter/flutter#146929) 2024-04-17 [email protected] Get rid of _NullElement. (flutter/flutter#146741) 2024-04-17 [email protected] Roll Flutter Engine from 818191d11404 to bc6382e35365 (2 revisions) (flutter/flutter#146930) 2024-04-17 [email protected] Fix memory leak in paginated tables (flutter/flutter#146755) 2024-04-17 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Try to be more consistent about deleting test apps in devicelab logic. (#146856)" (flutter/flutter#146927) 2024-04-17 [email protected] Roll Flutter Engine from 25b09e883dd8 to 818191d11404 (3 revisions) (flutter/flutter#146898) 2024-04-17 [email protected] Try to be more consistent about deleting test apps in devicelab logic. (flutter/flutter#146856) 2024-04-17 [email protected] Unpin frontend_server_client and roll packages (flutter/flutter#146650) 2024-04-17 [email protected] Roll Packages from 90c876d to d39830e (4 revisions) (flutter/flutter#146895) 2024-04-17 [email protected] Roll Flutter Engine from 0e9e2f419225 to 25b09e883dd8 (1 revision) (flutter/flutter#146889) 2024-04-17 [email protected] Reland: Update link branches to `main` (flutter/flutter#146882) 2024-04-17 [email protected] Roll Flutter Engine from 4b949d7d6065 to 0e9e2f419225 (4 revisions) (flutter/flutter#146884) 2024-04-17 [email protected] Revert "Update link branches to `main`" (flutter/flutter#146880) 2024-04-17 [email protected] Fix `Tab` indicator image configuration doesn't inherit device pixel ratio (flutter/flutter#146812) 2024-04-17 [email protected] Roll Flutter Engine from 1ea39b29ce8c to 4b949d7d6065 (1 revision) (flutter/flutter#146865) 2024-04-17 [email protected] Update link branches to `main` (flutter/flutter#146558) 2024-04-17 [email protected] Roll Flutter Engine from 45c86593a09a to 1ea39b29ce8c (1 revision) (flutter/flutter#146863) 2024-04-17 [email protected] Rewrap some lines that are about to get much longer. (flutter/flutter#146803) 2024-04-17 [email protected] Roll Flutter Engine from 0cdbc8a6e463 to 45c86593a09a (1 revision) (flutter/flutter#146862) 2024-04-17 [email protected] Roll Flutter Engine from 31b67985de27 to 0cdbc8a6e463 (1 revision) (flutter/flutter#146861) 2024-04-17 [email protected] Roll Flutter Engine from e7d8c62d291a to 31b67985de27 (1 revision) (flutter/flutter#146859) 2024-04-17 [email protected] Roll Flutter Engine from 4d69c0c95483 to e7d8c62d291a (11 revisions) (flutter/flutter#146857) 2024-04-16 [email protected] Switch to relevant `Remote` constructors (flutter/flutter#146773) ...

-

13303dc

github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. f: cupertino flutter/packages/flutter/cupertino repository labels Apr 14, 2024

Update flutter_test_config.dart

b35564b

flutter deleted a comment from flutter-dashboard bot Apr 14, 2024

polina-c added 2 commits April 14, 2024 13:41

Update text_selection_toolbar.dart

3023288

Update text_selection_toolbar.dart

a9722b5

polina-c requested a review from goderbauer April 14, 2024 20:47

polina-c added the a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker label Apr 15, 2024

polina-c changed the title ~~Get rid of null element.~~ Get rid of _NullElement. Apr 16, 2024

goderbauer approved these changes Apr 16, 2024

View reviewed changes

polina-c requested a review from Hixie April 16, 2024 20:52

Hixie approved these changes Apr 17, 2024

View reviewed changes

polina-c added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 17, 2024

auto-submit bot merged commit ae0a0df into flutter:master Apr 17, 2024

polina-c deleted the null-element branch April 17, 2024 19:11

engine-flutter-autoroll mentioned this pull request Apr 18, 2024

Roll Flutter from 3882afbfa997 to fb110b98da15 (56 revisions) flutter/packages#6565

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 18, 2024

ae0a0df Get rid of _NullElement. (flutter/flutter#146741)

178bc54

engine-flutter-autoroll mentioned this pull request Apr 18, 2024

Manual roll Flutter from 3882afbfa997 to c219bf73fce4 (61 revisions) flutter/packages#6566

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 18, 2024

ae0a0df Get rid of _NullElement. (flutter/flutter#146741)

5acad19

gilnobrega pushed a commit to gilnobrega/flutter that referenced this pull request Apr 22, 2024

Get rid of _NullElement. (flutter#146741)

e4a0d79

Fixes flutter#145602

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Get rid of _NullElement. #146741

Get rid of _NullElement. #146741

Uh oh!

polina-c commented Apr 14, 2024

Uh oh!

flutter-dashboard bot commented Apr 16, 2024

Uh oh!

polina-c commented Apr 16, 2024

Uh oh!

goderbauer left a comment

Uh oh!

goderbauer commented Apr 16, 2024

Uh oh!

Hixie commented Apr 16, 2024

Uh oh!

polina-c commented Apr 16, 2024 •

edited

Loading

Uh oh!

Hixie commented Apr 17, 2024

Uh oh!

polina-c commented Apr 17, 2024 •

edited

Loading

Uh oh!

Hixie commented Apr 17, 2024

Uh oh!

Hixie left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Get rid of _NullElement. #146741

Get rid of _NullElement. #146741

Uh oh!

Conversation

polina-c commented Apr 14, 2024

Uh oh!

flutter-dashboard bot commented Apr 16, 2024

Uh oh!

polina-c commented Apr 16, 2024

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

goderbauer commented Apr 16, 2024

Uh oh!

Hixie commented Apr 16, 2024

Uh oh!

polina-c commented Apr 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hixie commented Apr 17, 2024

Uh oh!

polina-c commented Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hixie commented Apr 17, 2024

Uh oh!

Hixie left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

polina-c commented Apr 16, 2024 •

edited

Loading

polina-c commented Apr 17, 2024 •

edited

Loading