Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove opt out for CurvedAnimation. #147594

Merged
merged 22 commits into from
May 28, 2024
Merged

Remove opt out for CurvedAnimation. #147594

merged 22 commits into from
May 28, 2024
+46 −224

Conversation

polina-c
Copy link
Contributor

@polina-c polina-c commented Apr 30, 2024 •
edited

Fixes #145600

Prerequisite:

  • clean up ~400 leaking tests of CurvedAnimation.

@ValentinVignal , @Dimilkalathiya and @ksokolovskyi

I highly appreciate your help resolving leaks.
I noticed couple overlaps during last week. I appreciate the effort and do not want it to be less efficient than it could be. How about splitting work?

Suggested parts:

  1. Failures of Windows framework_tests_libraries_leak_tracking, from top of stdout
  2. Failures of Windows framework_tests_libraries_leak_tracking, from bottom of stdout
  3. Failures of Windows framework_tests_widgets_leak_tracking, from top of stdout
  4. Failures of Windows framework_tests_widgets_leak_tracking, from bottom of stdout

Overlaps still can happen, but with less probability. Feel free to pick a part.

@github-actions github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Apr 30, 2024
@polina-c polina-c changed the title - Remaining leaks. Apr 30, 2024
@polina-c polina-c changed the title Remaining leaks. Remaining leaks of CurvedAnimation. May 1, 2024
@polina-c polina-c mentioned this pull request May 1, 2024
Open
9 tasks
@ValentinVignal
Copy link
Contributor

ValentinVignal commented May 4, 2024 •
edited

Great idea, maybe we can split the work once #146999 is merged. For now, most of the leaks are from CupertinoPageTransition and CupertinoFullscreenDialogTransition and hopefully #146999 will fix them

@polina-c
Copy link
Contributor Author

polina-c commented May 5, 2024 •
edited

Great idea, maybe we can split the work once #146999 is merged. For now, most of the leaks are from CupertinoPageTransition and CupertinoFullscreenDialogTransition and hopefully #146999 will fix them

Cool!
I did not realize this PR still fixes some leaks. If it does, should it be reflected in name in addition to janks?
I see this PR is waiting for review. In general, feel free to ping me on GitHub or twitter (PolinaCC) if a PR is waiting for something more than one business day.

@polina-c
Copy link
Contributor Author

polina-c commented May 8, 2024 •
edited

Number of leaking test libraries:

  • Windows framework_tests_libraries_leak_tracking: 24
  • Windows framework_tests_widgets_leak_tracking: 3

It is way less than original ~400
Thank you for fixes!

@Dimilkalathiya
Copy link
Contributor

Dimilkalathiya commented May 20, 2024 •
edited

It seems we can we only have one issue to fix to close CurvedAnimation chapter (after #147823 is merged) 👀

as of now only 3 test files are leaking:-

  • material/dialog_test.dart
  • cupertino/dialog_test.dart
  • cupertino/route_test.dart
Screenshot 2024-05-20 at 10 40 40 PM

Origin of leak for all of them is CupertinoDialogRoute in which _buildCupertinoDialogTransitions is using CurvedAnimation without any means of disposal

@polina-c
Copy link
Contributor Author

It seems we can we only have one issue to fix to close CurvedAnimation chapter (after #147823 is merged) 👀

as of now only 3 test files are leaking:-

  • material/dialog_test.dart
  • cupertino/dialog_test.dart
  • cupertino/route_test.dart

Cool!

@polina-c
Copy link
Contributor Author

polina-c commented May 26, 2024 •
edited

Time to celebrate - no remaining leaks of CurvedAnimation!!!

Thank you @Dimilkalathiya , for landing last PR 🩵

I will cleanup CurvedAnimation TODOs

@polina-c polina-c closed this May 26, 2024
@polina-c polina-c reopened this May 26, 2024
@polina-c polina-c added the a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker label May 26, 2024
@polina-c polina-c changed the title Remaining leaks of CurvedAnimation. Remove opt out for CurvedAnimation. May 26, 2024
@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems f: material design flutter/packages/flutter/material repository. f: scrolling Viewports, list views, slivers, etc. f: cupertino flutter/packages/flutter/cupertino repository f: routes Navigator, Router, and related APIs. f: focus Focus traversal, gaining or losing focus labels May 26, 2024
@polina-c polina-c marked this pull request as ready for review May 27, 2024 02:18
@polina-c polina-c requested a review from goderbauer May 27, 2024 02:18
goderbauer
goderbauer approved these changes May 28, 2024
View reviewed changes
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

@polina-c polina-c merged commit 65e3007 into flutter:master May 28, 2024
71 checks passed
@polina-c polina-c deleted the count-leaks branch May 28, 2024 23:35
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 29, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 29, 2024
@engine-flutter-autoroll
65e300768 Remove opt out for CurvedAnimation. (flutter/flutter#147594)
efad5ef
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 29, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 29, 2024
@engine-flutter-autoroll
65e300768 Remove opt out for CurvedAnimation. (flutter/flutter#147594)
c9a276b
auto-submit bot pushed a commit to flutter/packages that referenced this pull request May 29, 2024
@engine-flutter-autoroll
Roll Flutter from a1a33e63b911 to c85fa6abf4ff (20 revisions) (#6829)
82a811f 
flutter/flutter@a1a33e6...c85fa6a

2024-05-29 polinach@google.com Clean leak in editable_text_test.dart. (flutter/flutter#149223)
2024-05-29 sokolovskyi.konstantin@gmail.com Add tests for animated_switcher.0.dart API example. (flutter/flutter#149180)
2024-05-29 engine-flutter-autoroll@skia.org Manual roll Flutter Engine from d0323905fc2f to b26e1b023cdb (16 revisions) (flutter/flutter#149220)
2024-05-29 hany212mohamed@gmail.com Change snack bar default hitTestBehavior to deferToChild when SnackBarThemeData.insetPadding is not null (flutter/flutter#148568)
2024-05-29 98614782+auto-submit[bot]@users.noreply.github.com Reverts "sliverGridDelegate mainAxisExtent add assert (#148470)" (flutter/flutter#149224)
2024-05-29 yinxulolol@gmail.com sliverGridDelegate mainAxisExtent add assert (flutter/flutter#148470)
2024-05-29 luis901101@gmail.com Fix `SearchAnchor` suggestions not refreshing after long API call (flutter/flutter#148767)
2024-05-28 737941+loic-sharma@users.noreply.github.com Add link to golden file test docs in the framework gardener guide (flutter/flutter#149207)
2024-05-28 polinach@google.com Remove opt out for CurvedAnimation. (flutter/flutter#147594)
2024-05-28 31859944+LongCatIsLooong@users.noreply.github.com Fix the RenderFlex.computeDryBaseline implementation to match computeDistanceToActualBaseline (flutter/flutter#149062)
2024-05-28 polinach@google.com Clean leaky test. (flutter/flutter#149199)
2024-05-28 34871572+gmackall@users.noreply.github.com Change `android_plugin_new_output_dir_test.dart` test description (flutter/flutter#149198)
2024-05-28 leroux_bruno@yahoo.fr fix M2 InputDecorator suffix icon doesn't turn red on error (flutter/flutter#149161)
2024-05-28 77919688+varunkamanibosc@users.noreply.github.com Add selectionOverlayBuilder in CupertinoDatePicker and CupertinoTimer� (flutter/flutter#143079)
2024-05-28 jmccandless@google.com Mouse onEnter and onExit now support hovering stylus (flutter/flutter#149006)
2024-05-28 31859944+LongCatIsLooong@users.noreply.github.com Remove `TextEditingController` private member access (flutter/flutter#149042)
2024-05-28 engine-flutter-autoroll@skia.org Roll Packages from b7bcb4b to a933c30 (1 revision) (flutter/flutter#149184)
2024-05-28 engine-flutter-autoroll@skia.org Roll Flutter Engine from b1751088c7e9 to d0323905fc2f (2 revisions) (flutter/flutter#149169)
2024-05-28 kevmoo@users.noreply.github.com [tool] Use kebabCase directly (flutter/flutter#149150)
2024-05-28 katelovett@google.com [wiki migration] Leftover wiki pages and links (flutter/flutter#148989)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC camillesimon@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
victorsanni pushed a commit to victorsanni/flutter that referenced this pull request May 31, 2024
@polina-c @victorsanni
Remove opt out for CurvedAnimation. (flutter#147594)
88abe3f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goderbauer goderbauer goderbauer approved these changes

Assignees
No one assigned
Labels
a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker a: text input Entering text in a text field or keyboard related problems f: cupertino flutter/packages/flutter/cupertino repository f: focus Focus traversal, gaining or losing focus f: material design flutter/packages/flutter/material repository. f: routes Navigator, Router, and related APIs. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CurvedAnimation is leaking.
4 participants
@polina-c @ValentinVignal @Dimilkalathiya @goderbauer