Create class MemoryAllocations. by polina-c · Pull Request #110230 · flutter/flutter

polina-c · 2022-08-25T01:27:27Z

Design: go/dart-leaktracker-productization
Peer PR in engine: flutter/engine#35274

Verified memory_allocations.dart is not included into release build if the flag --dart-define=flutter.memory_allocations=true is not passed:

dnfield

I'm not quite clear on how this will be used or why it belongs in the framework at this point.

packages/flutter/lib/src/foundation/memory_allocations.dart

goderbauer

It's kinda difficult reviewing this skeleton without docs filled in and seeing how this would be used by other parts of the framework.

packages/flutter/lib/src/foundation/memory_allocations.dart

packages/flutter/lib/src/foundation/change_notifier.dart

goderbauer · 2022-09-01T17:42:07Z

packages/flutter/lib/src/foundation/change_notifier.dart

+        object: this,
+      ));
+    }
+    _creationDispatched = true;


Why is the inherited implementation from ChangeNotifier not enough? Also, it is odd that this is reaching out to a private in ChangeNotifier. Does that mean, I couldn't implement my own ValueNotifier-like class based on ChangeNotifier outside of the framework?

Yes, the implementation is not super-elegant. However, as there is no constructor in ChangeNotifier (because it is used as mixin), I did not find a better way.

You can implement ValueNotifier-like class based on ChangeNotifier though. It will dispatch the events if only a listener is added.

goderbauer · 2022-09-01T17:43:42Z

packages/flutter/lib/src/foundation/change_notifier.dart

  @override
  void addListener(VoidCallback listener) {
    assert(ChangeNotifier.debugAssertNotDisposed(this));
+    if (!_creationDispatched) {


One thought that occurred to me: In release builds that ship to customers you probably want this entire code tree-shaken out. That's currently no possible?

Thank you for bringing it. Introduced kFlutterMemoryAllocationsEnabled and verified tree-shaking happens (see screen shot in description)

goderbauer · 2022-09-01T17:45:22Z

packages/flutter/lib/src/foundation/change_notifier.dart

    assert(ChangeNotifier.debugAssertNotDisposed(this));
+    if (!_creationDispatched) {
+      if (MemoryAllocations.instance.hasListeners) {
+          MemoryAllocations.instance.dispatchObjectEvent(ObjectCreated(


I am not sure how these events will be consumed, but will it not be confusing if an object creation notification is received somewhat randomly long after the object has been created?

Yes, it is not perfect, but I do not see other way to inform about ChangeNotifier creation.

goderbauer · 2022-09-01T17:46:31Z

packages/flutter/lib/src/foundation/memory_allocations.dart

+
+/// List of the Flutter Framework and SDK libraries with instrumented
+/// classes.
+class FlutterLibraries {


Does this have to be public API?

packages/flutter/lib/src/foundation/memory_allocations.dart

goderbauer · 2022-09-01T17:57:21Z

packages/flutter/lib/src/foundation/memory_allocations.dart

+      try {
+        listeners[i]?.call(objectEvent);
+      } catch (exception, stack) {
+        final String type = objectEvent.object.runtimeType.toString();


There should be a no_runtimeType_toString lint on this line, no?

nope

And if i remove the type, I get lint about return type

goderbauer · 2022-09-01T18:00:10Z

packages/flutter/lib/src/foundation/memory_allocations.dart

+      return;
+    }
+
+    if (_activeDispatchLoops > 0) {


For my own understanding, why is the removal/notification logic so different from ChangeNotifier? I wish we could just reuse that logic instead of reinventing everything.

ChangeNotifier is different, because it is not-singletone and is optimized for often adding/removing listeners.
One of comments in ChangeNotifier is 'The list holding the listeners is not growable for performances reasons.'.
We are ok with growable list for MemoryAllocations, because it is singletone.

We still care about performance for event dispatching, as the number of events can be huge.

packages/flutter/lib/src/foundation/change_notifier.dart

goderbauer

LGTM

zanderso · 2022-09-13T16:50:04Z

SkiaPerf is reporting regressions in some microbenchmarks on this change:

https://flutter-flutter-perf.skia.org/e/?begin=1662761661&end=1663004766&keys=X1a50f85f8486121abd691e2222ee5d46&selected=commit%3D30988%26name%3D%252Carch%253Dintel%252Cbranch%253Dmaster%252Cconfig%253Ddefault%252Cdevice_type%253DMoto_G__4_%252Cdevice_version%253Dandroid-24%252Chost_type%253Dmac%252Csub_result%253Dnotify0%252Ctest%253Dmicrobenchmarks%252C&xbaroffset=30988

Does the flag to disable this feature need to be passed in more places?

polina-c · 2022-09-13T18:19:53Z

Following up.

polina-c · 2022-09-13T19:12:28Z

Regression is triggered by:
https://github.com/flutter/flutter/blob/master/dev/devicelab/lib/tasks/microbenchmarks.dart
Subresults: notify0, removeWhileNotify1

Guidance: https://github.com/flutter/flutter/tree/master/dev/devicelab

Command:

../../bin/cache/dart-sdk/bin/dart bin/test_runner.dart test -t microbenchmarks

https://github.com/polina-c/spikes/tree/master/memory_allocation_investigation

dnfield · 2022-09-14T22:30:51Z

From offline conversation:

Polina got initial results from an emulator, which may be why the differences didn't show up.
We should probably just turn this off by default in profile mode and enable it on benchmarks/tests/situations that need it explicitly.
The benchmarks this affected are change notifier related ones, which are likely to be somewhat impactful in real life apps using Provider, so this may affect internal benchmarks.

@polina-c is going to follow up with a patch to remove the || kProfileMode for now.

polina-c · 2022-09-14T22:52:07Z

fix: #111615

polina-c · 2022-09-15T15:15:29Z

Recovered: https://flutter-flutter-perf.skia.org/e/?begin=1662659034&end=1663253364&keys=X1a50f85f8486121abd691e2222ee5d46&requestType=0&selected=commit%3D30988%26name%3D%252Carch%253Dintel%252Cbranch%253Dmaster%252Cconfig%253Ddefault%252Cdevice_type%253DMoto_G__4_%252Cdevice_version%253Dandroid-24%252Chost_type%253Dmac%252Csub_result%253Dnotify0%252Ctest%253Dmicrobenchmarks%252C&xbaroffset=30988

(to get this link i opened the regression link and scrolled right by pressing 'd')

polina-c added 6 commits August 11, 2022 15:54

-

117760a

Update memory_allocations.dart

78b1829

Update memory_allocations.dart

6ecaed2

Merge branch 'master' of github.com:flutter/flutter into ma

0290ac4

Update memory_allocations.dart

204d597

Update memory_allocations.dart

b96d7bb

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Aug 25, 2022

polina-c added 2 commits August 24, 2022 18:28

Update memory_allocations_test.dart

ad17409

Update memory_allocations.dart

55fbc40

polina-c mentioned this pull request Aug 25, 2022

Instrument Image and Picture for leak tracking. flutter/engine#35274

Merged

4 tasks

dnfield reviewed Aug 25, 2022

View reviewed changes

packages/flutter/lib/src/foundation/memory_allocations.dart Outdated Show resolved Hide resolved

packages/flutter/lib/src/foundation/memory_allocations.dart Outdated Show resolved Hide resolved

polina-c changed the title ~~Create class MemoryAllocation.~~ Create class MemoryAllocations. Aug 25, 2022

goderbauer reviewed Aug 25, 2022

View reviewed changes

packages/flutter/lib/src/foundation/memory_allocations.dart Outdated Show resolved Hide resolved

polina-c added 3 commits August 26, 2022 08:34

Update memory_allocations.dart

65915a5

Update memory_allocations.dart

6279e33

Update memory_allocations.dart

60bf779

polina-c mentioned this pull request Aug 26, 2022

leak-tracker: Detect memory leaks by identifying not disposed or not GCed objects. flutter/devtools#3951

Closed

polina-c added 10 commits August 26, 2022 16:20

Merge branch 'master' of github.com:flutter/flutter into ma

e465294

-

a942893

-

80f0f97

-

2758510

Merge branch 'master' of github.com:flutter/flutter into ma

359f7e5

Merge branch 'master' of github.com:flutter/flutter into ma

73e60f5

-

5f02d60

Update memory_allocations.dart

841111f

-

81dd4c3

Update memory_allocations.dart

cb5952c

polina-c marked this pull request as ready for review September 1, 2022 03:51

goderbauer reviewed Sep 1, 2022

View reviewed changes

Update memory_allocations.dart

83d8c48

polina-c marked this pull request as ready for review September 9, 2022 18:48

Merge branch 'master' of github.com:flutter/flutter into ma

a0a7834

goderbauer approved these changes Sep 9, 2022

View reviewed changes

polina-c added 2 commits September 9, 2022 15:44

Merge branch 'flutter:master' into ma

b25d7a9

Merge branch 'flutter:master' into ma

1a9b79a

polina-c merged commit a3c3dc8 into flutter:master Sep 10, 2022

polina-c deleted the ma branch September 10, 2022 00:23

This was referenced Sep 10, 2022

Roll Flutter from ad946f8478bf to 4930444f4afe (31 revisions) flutter/packages#2588

Merged

Roll Flutter from 4cd734ae3a1c to 4930444f4afe (28 revisions) flutter/plugins#6386

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 10, 2022

a3c3dc8 Create class MemoryAllocations. (flutter/flutter#110230)

9a2f3cb

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 10, 2022

a3c3dc8 Create class MemoryAllocations. (flutter/flutter#110230)

25aa0f8

engine-flutter-autoroll mentioned this pull request Sep 10, 2022

Roll Flutter from 4cd734ae3a1c to 4930444f4afe (28 revisions) flutter/plugins#6387

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 10, 2022

a3c3dc8 Create class MemoryAllocations. (flutter/flutter#110230)

5437937

engine-flutter-autoroll mentioned this pull request Sep 11, 2022

Roll Flutter from 4cd734ae3a1c to 4930444f4afe (28 revisions) flutter/plugins#6388

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 11, 2022

a3c3dc8 Create class MemoryAllocations. (flutter/flutter#110230)

e0e3e3b

engine-flutter-autoroll mentioned this pull request Sep 12, 2022

Roll Flutter from 4cd734ae3a1c to 7ad47233a9a3 (29 revisions) flutter/plugins#6410

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 12, 2022

a3c3dc8 Create class MemoryAllocations. (flutter/flutter#110230)

4a307da

engine-flutter-autoroll mentioned this pull request Sep 12, 2022

Roll Flutter from 4cd734ae3a1c to 6b56654890b0 (30 revisions) flutter/plugins#6412

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 12, 2022

a3c3dc8 Create class MemoryAllocations. (flutter/flutter#110230)

27fff26

engine-flutter-autoroll mentioned this pull request Sep 12, 2022

Roll Flutter from 4cd734ae3a1c to 81fd386a7fdb (32 revisions) flutter/plugins#6413

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 12, 2022

a3c3dc8 Create class MemoryAllocations. (flutter/flutter#110230)

dd4c769

engine-flutter-autoroll mentioned this pull request Sep 13, 2022

Roll Flutter from 4cd734ae3a1c to bbc42632a084 (57 revisions) flutter/plugins#6419

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 13, 2022

a3c3dc8 Create class MemoryAllocations. (flutter/flutter#110230)

fa40aa8

polina-c mentioned this pull request Sep 14, 2022

Fix performance regression. #111615

Merged

Conversation

polina-c commented Aug 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polina-c Sep 1, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

polina-c Sep 1, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

zanderso commented Sep 13, 2022

Uh oh!

polina-c commented Sep 13, 2022

Uh oh!

polina-c commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfield commented Sep 14, 2022

Uh oh!

polina-c commented Sep 14, 2022

Uh oh!

polina-c commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

polina-c commented Aug 25, 2022 •

edited

Loading

polina-c Sep 1, 2022 •

edited

Loading

polina-c Sep 1, 2022 •

edited

Loading

polina-c commented Sep 13, 2022 •

edited

Loading

polina-c commented Sep 15, 2022 •

edited

Loading