Skip to content

Conversation

@gspencergoog
Copy link
Contributor

@gspencergoog gspencergoog commented Oct 3, 2019

Reverts #41220 because of a performance regression in microbenchmarks_stock_build_iteration (#41919). Will investigate and re-land next week.

Fixes #41919

@fluttergithubbot fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Oct 3, 2019
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

RSLGTM

@gspencergoog
Copy link
Contributor Author

gspencergoog commented Oct 4, 2019

I'm also reverting the shortcuts for keyboard traversal (#40186), since they won't function properly unless the InkWell has a Focus node, which is what the PR with the performance regression had in it.

@flutter flutter deleted a comment from codecov bot Oct 4, 2019
@codecov
Copy link

codecov bot commented Oct 4, 2019

Codecov Report

Merging #41945 into master will decrease coverage by 0.61%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #41945      +/-   ##
==========================================
- Coverage   59.31%   58.69%   -0.62%     
==========================================
  Files         194      193       -1     
  Lines       18934    18882      -52     
==========================================
- Hits        11230    11083     -147     
- Misses       7704     7799      +95
Flag Coverage Δ
#flutter_tool 58.69% <ø> (-0.62%) ⬇️
Impacted Files Coverage Δ
packages/flutter_tools/lib/src/android/gradle.dart 46.5% <0%> (-29.31%) ⬇️
packages/flutter_tools/lib/src/base/os.dart 25.39% <0%> (-3.18%) ⬇️
packages/flutter_tools/lib/src/build_info.dart 72.43% <0%> (-1.63%) ⬇️
packages/flutter_tools/lib/src/version.dart 91.9% <0%> (-1.43%) ⬇️
...ges/flutter_tools/lib/src/build_runner/web_fs.dart 28.87% <0%> (-0.84%) ⬇️
packages/flutter_tools/lib/src/cache.dart 45.75% <0%> (-0.67%) ⬇️
...utter_tools/lib/src/build_runner/build_script.dart
packages/flutter_tools/lib/src/base/utils.dart 95.5% <0%> (+2%) ⬆️
packages/flutter_tools/lib/src/dart/pub.dart 74.76% <0%> (+2.8%) ⬆️
packages/flutter_tools/lib/src/compile.dart 78.59% <0%> (+12.98%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8c7530...8b54a03. Read the comment docs.

@jonahwilliams jonahwilliams merged commit 3e2dc8c into flutter:master Oct 4, 2019
gspencergoog added a commit that referenced this pull request Oct 10, 2019
This attempts to reland #40186 and #41220, that were reverted in #41945.

The main modifications from the original PRs are that I predefine the shortcuts and actions maps instead of defining them inline in the build function, and I use a new mapEquals to do a deep comparison so that we don't rebuild modified things if the contents of the map haven't changed.

I also eliminated an operator== and hashCode that were defined on the Actions widget, since widgets shouldn't have those. (it's too bad though: I get an 85% speedup if we leave this in! Too bad it prevents rebuilding of the children...)

Fixes #40101
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
This attempts to reland flutter#40186 and flutter#41220, that were reverted in flutter#41945.

The main modifications from the original PRs are that I predefine the shortcuts and actions maps instead of defining them inline in the build function, and I use a new mapEquals to do a deep comparison so that we don't rebuild modified things if the contents of the map haven't changed.

I also eliminated an operator== and hashCode that were defined on the Actions widget, since widgets shouldn't have those. (it's too bad though: I get an 85% speedup if we leave this in! Too bad it prevents rebuilding of the children...)

Fixes flutter#40101
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

microbenchmarks_stock_build_iteration regression

5 participants