Enable ChangeNotifier clients to dispatch event of object creation in constructor. #133060
Conversation
|
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 on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on 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. |
github-actions
bot
added
framework
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| _creationDispatched = true; | ||
| } | ||
|
|
||
| mayBeDispatchObjectCreation(); |
There was a problem hiding this comment.
Is the dart tree shaker smart enough to shake out this entire method call when kFlutterMemoryAllocationsEnabled is disabled (i.e. in release mode)?
There was a problem hiding this comment.
Same question for all other call sides.
There was a problem hiding this comment.
I created app that extends ChangeNotifier, and analyzed its content when kFlutterMemoryAllocationsEnabled is false, in two cases:
- When method maybeDispatchObjectCreation checks kFlutterMemoryAllocationsEnabled in condition (this PR)
- When there is no check in the method
First option did not contain MemoryAllocations, while second one contained.
So, yes, tree shaker is smart enough.
There was a problem hiding this comment.
But does it also eliminate the extra method call of calling mayBeDispatchObjectCreation or do you still pay for that unnecessarily in release mode?
There was a problem hiding this comment.
For some reasons I do not see this method in both cases, even when I invoke it from the app. Can it be optimized to become part of constructor that invokes it?
Anyway, this PR does not change anything from perspective of this code. It just moves existing code from one place to another.
How about merging this PR to unblock investigation for existing leaks, and then researching what is happening with the method?
There was a problem hiding this comment.
Per discussion, updated invocations to be under the flag kFlutterMemoryAllocationsEnabled
polina-c
added
the
autosubmit
…ation in constructor. (flutter/flutter#133060)
…ation in constructor. (flutter/flutter#133060)
flutter/flutter@bd836cc...deeb811 2023-08-24 zanderso@users.noreply.github.com Reverts "Roll Flutter Engine from aa98a9d2e86f to 3b92bb6eda19 (4 revisions) (#133224)" (flutter/flutter#133255) 2023-08-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from aa98a9d2e86f to 3b92bb6eda19 (4 revisions) (flutter/flutter#133224) 2023-08-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from 67e8b825cc91 to aa98a9d2e86f (4 revisions) (flutter/flutter#133220) 2023-08-24 engine-flutter-autoroll@skia.org Roll Flutter Engine from 27d75f6221d2 to 67e8b825cc91 (1 revision) (flutter/flutter#133214) 2023-08-23 katelovett@google.com Remove deprecated PlatformViewsService.synchronizeToNativeViewHierarchy (flutter/flutter#133175) 2023-08-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 58dc868c26cb to 27d75f6221d2 (14 revisions) (flutter/flutter#133211) 2023-08-23 katelovett@google.com Remove deprecated MaterialButtonWithIconMixin (flutter/flutter#133173) 2023-08-23 polinach@google.com Enable ChangeNotifier clients to dispatch event of object creation in constructor. (flutter/flutter#133060) 2023-08-23 goderbauer@google.com Enable literal_only_boolean_expressions (flutter/flutter#133186) 2023-08-23 47866232+chunhtai@users.noreply.github.com Add android analyzer commands for applinks (flutter/flutter#131009) 2023-08-23 reidbaker@google.com Bump memory usage in gradle for platform views (flutter/flutter#133155) 2023-08-23 christopherfujino@gmail.com [flutter_tools] Fix legacy version file not being ensured (flutter/flutter#133097) 2023-08-23 goderbauer@google.com Remove deprecated *TestValues from TestWindow (flutter/flutter#131098) 2023-08-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from ac352ba95f43 to 58dc868c26cb (2 revisions) (flutter/flutter#133165) 2023-08-23 mdebbar@google.com [web] Remove unnecessary lint ignore (flutter/flutter#133164) 2023-08-23 jacksongardner@google.com Add `--experimental-wasm-type-reflection` and support newer emscripten builds. (flutter/flutter#133084) 2023-08-23 engine-flutter-autoroll@skia.org Roll Flutter Engine from 72a06427bd37 to ac352ba95f43 (2 revisions) (flutter/flutter#133158) 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 rmistry@google.com,stuartmorgan@google.com,ychris@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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
| /// Make sure to invoke it with condition `if (kFlutterMemoryAllocationsEnabled) ...` | ||
| /// so that the method is tree-shaken away when the flag is false. | ||
| @protected | ||
| void maybeDispatchObjectCreation() { |
There was a problem hiding this comment.
Adding a new public method to ChangeNotifier is a breaking change for all users that implement ChangeNotifier
class ChangeNotifier2 implements ChangeNotifier {
// ...
}
Last year, there was a similar breaking change, which was converted to a static method instead to make it non-breaking.
Can we avoid the breaking change here somehow?
There was a problem hiding this comment.
@passsy You're right, we missed this in our review. It would be great if you (or whoever else suffers under this) could donate some tests to https://github.com/flutter/tests so next time somebody attempts to add a method here tests are failing to remind us that we shouldn't add new methods here.
@polina-c Can you please look into how we can refactor this without causing pain to people that implement ChangeNotifier?
polina-c
added
the
a: leak tracking
…ation in constructor. (flutter/flutter#133060)
It is needed to debug notDisposed leaks, where creation call-stack is used to locate owner of the leaking instance.
This PR makes one of the clients invoking the method. Others will start doing this in following PRs.
Peer PR in leak_tracker: dart-lang/leak_tracker#130