From 10d5ec875ddac3509d5f8f6487cdbcb03a0de6af Mon Sep 17 00:00:00 2001 From: Romain Rastel Date: Thu, 15 Apr 2021 01:49:03 +0200 Subject: [PATCH] Improve the performances of ChangeNotifier (#71947) --- .../lib/src/foundation/change_notifier.dart | 144 ++++++++++-- .../test/foundation/change_notifier_test.dart | 218 +++++++++++++++--- 2 files changed, 307 insertions(+), 55 deletions(-) diff --git a/packages/flutter/lib/src/foundation/change_notifier.dart b/packages/flutter/lib/src/foundation/change_notifier.dart index ce7b7166f36..4dd56c4f5b5 100644 --- a/packages/flutter/lib/src/foundation/change_notifier.dart +++ b/packages/flutter/lib/src/foundation/change_notifier.dart @@ -2,8 +2,6 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -import 'dart:collection'; - import 'package:meta/meta.dart'; import 'assertions.dart'; @@ -94,11 +92,6 @@ abstract class ValueListenable extends Listenable { T get value; } -class _ListenerEntry extends LinkedListEntry<_ListenerEntry> { - _ListenerEntry(this.listener); - final VoidCallback listener; -} - /// A class that can be extended or mixed in that provides a change notification /// API using [VoidCallback] for notifications. /// @@ -109,11 +102,15 @@ class _ListenerEntry extends LinkedListEntry<_ListenerEntry> { /// /// * [ValueNotifier], which is a [ChangeNotifier] that wraps a single value. class ChangeNotifier implements Listenable { - LinkedList<_ListenerEntry>? _listeners = LinkedList<_ListenerEntry>(); + int _count = 0; + List _listeners = List.filled(0, null); + int _notificationCallStackDepth = 0; + int _reentrantlyRemovedListeners = 0; + bool _debugDisposed = false; bool _debugAssertNotDisposed() { assert(() { - if (_listeners == null) { + if (_debugDisposed) { throw FlutterError( 'A $runtimeType was used after being disposed.\n' 'Once you have called dispose() on a $runtimeType, it can no longer be used.', @@ -142,7 +139,7 @@ class ChangeNotifier implements Listenable { @protected bool get hasListeners { assert(_debugAssertNotDisposed()); - return _listeners!.isNotEmpty; + return _count > 0; } /// Register a closure to be called when the object changes. @@ -174,7 +171,48 @@ class ChangeNotifier implements Listenable { @override void addListener(VoidCallback listener) { assert(_debugAssertNotDisposed()); - _listeners!.add(_ListenerEntry(listener)); + if (_count == _listeners.length) { + if (_count == 0) { + _listeners = List.filled(1, null); + } else { + final List newListeners = + List.filled(_listeners.length * 2, null); + for (int i = 0; i < _count; i++) { + newListeners[i] = _listeners[i]; + } + _listeners = newListeners; + } + } + _listeners[_count++] = listener; + } + + void _removeAt(int index) { + // The list holding the listeners is not growable for performances reasons. + // We still want to shrink this list if a lot of listeners have been added + // and then removed outside a notifyListeners iteration. + // We do this only when the real number of listeners is half the length + // of our list. + _count -= 1; + if (_count * 2 <= _listeners.length) { + final List newListeners = List.filled(_count, null); + + // Listeners before the index are at the same place. + for (int i = 0; i < index; i++) + newListeners[i] = _listeners[i]; + + // Listeners after the index move towards the start of the list. + for (int i = index; i < _count; i++) + newListeners[i] = _listeners[i + 1]; + + _listeners = newListeners; + } else { + // When there are more listeners than half the length of the list, we only + // shift our listeners, so that we avoid to reallocate memory for the + // whole list. + for (int i = index; i < _count; i++) + _listeners[i] = _listeners[i + 1]; + _listeners[_count] = null; + } } /// Remove a previously registered closure from the list of closures that are @@ -193,10 +231,22 @@ class ChangeNotifier implements Listenable { @override void removeListener(VoidCallback listener) { assert(_debugAssertNotDisposed()); - for (final _ListenerEntry entry in _listeners!) { - if (entry.listener == listener) { - entry.unlink(); - return; + for (int i = 0; i < _count; i++) { + final VoidCallback? _listener = _listeners[i]; + if (_listener == listener) { + if (_notificationCallStackDepth > 0) { + // We don't resize the list during notifyListeners iterations + // but we set to null, the listeners we want to remove. We will + // effectively resize the list at the end of all notifyListeners + // iterations. + _listeners[i] = null; + _reentrantlyRemovedListeners++; + } else { + // When we are outside the notifyListeners iterations we can + // effectively shrink the list. + _removeAt(i); + } + break; } } } @@ -210,7 +260,10 @@ class ChangeNotifier implements Listenable { @mustCallSuper void dispose() { assert(_debugAssertNotDisposed()); - _listeners = null; + assert(() { + _debugDisposed = true; + return true; + }()); } /// Call all the registered listeners. @@ -232,15 +285,26 @@ class ChangeNotifier implements Listenable { @visibleForTesting void notifyListeners() { assert(_debugAssertNotDisposed()); - if (_listeners!.isEmpty) + if (_count == 0) return; - final List<_ListenerEntry> localListeners = List<_ListenerEntry>.from(_listeners!); + // To make sure that listeners removed during this iteration are not called, + // we set them to null, but we don't shrink the list right away. + // By doing this, we can continue to iterate on our list until it reaches + // the last listener added before the call to this method. - for (final _ListenerEntry entry in localListeners) { + // To allow potential listeners to recursively call notifyListener, we track + // the number of times this method is called in _notificationCallStackDepth. + // Once every recursive iteration is finished (i.e. when _notificationCallStackDepth == 0), + // we can safely shrink our list so that it will only contain not null + // listeners. + + _notificationCallStackDepth++; + + final int end = _count; + for (int i = 0; i < end; i++) { try { - if (entry.list != null) - entry.listener(); + _listeners[i]?.call(); } catch (exception, stack) { FlutterError.reportError(FlutterErrorDetails( exception: exception, @@ -257,6 +321,44 @@ class ChangeNotifier implements Listenable { )); } } + + _notificationCallStackDepth--; + + if (_notificationCallStackDepth == 0 && _reentrantlyRemovedListeners > 0) { + // We really remove the listeners when all notifications are done. + final int newLength = _count - _reentrantlyRemovedListeners; + if (newLength * 2 <= _listeners.length) { + // As in _removeAt, we only shrink the list when the real number of + // listeners is half the length of our list. + final List newListeners = List.filled(newLength, null); + + int newIndex = 0; + for (int i = 0; i < _count; i++) { + final VoidCallback? listener = _listeners[i]; + if (listener != null) { + newListeners[newIndex++] = listener; + } + } + + _listeners = newListeners; + } else { + // Otherwise we put all the null references at the end. + for (int i = 0; i < newLength; i += 1) { + if (_listeners[i] == null) { + // We swap this item with the next not null item. + int swapIndex = i + 1; + while(_listeners[swapIndex] == null){ + swapIndex += 1; + } + _listeners[i] = _listeners[swapIndex]; + _listeners[swapIndex] = null; + } + } + } + + _reentrantlyRemovedListeners = 0; + _count = newLength; + } } } diff --git a/packages/flutter/test/foundation/change_notifier_test.dart b/packages/flutter/test/foundation/change_notifier_test.dart index eb19e4f785c..05b406c486b 100644 --- a/packages/flutter/test/foundation/change_notifier_test.dart +++ b/packages/flutter/test/foundation/change_notifier_test.dart @@ -20,7 +20,9 @@ class HasListenersTester extends ValueNotifier { class A { bool result = false; - void test() { result = true; } + void test() { + result = true; + } } class B extends A with ChangeNotifier { @@ -31,12 +33,36 @@ class B extends A with ChangeNotifier { } } +class Counter with ChangeNotifier { + int get value => _value; + int _value = 0; + set value(int value) { + if (_value != value) { + _value = value; + notifyListeners(); + } + } + + void notify() { + notifyListeners(); + } +} + void main() { testWidgets('ChangeNotifier', (WidgetTester tester) async { final List log = []; - void listener() { log.add('listener'); } - void listener1() { log.add('listener1'); } - void listener2() { log.add('listener2'); } + void listener() { + log.add('listener'); + } + + void listener1() { + log.add('listener1'); + } + + void listener2() { + log.add('listener2'); + } + void badListener() { log.add('badListener'); throw ArgumentError(); @@ -111,9 +137,18 @@ void main() { final TestNotifier test = TestNotifier(); final List log = []; - void listener1() { log.add('listener1'); } - void listener3() { log.add('listener3'); } - void listener4() { log.add('listener4'); } + void listener1() { + log.add('listener1'); + } + + void listener3() { + log.add('listener3'); + } + + void listener4() { + log.add('listener4'); + } + void listener2() { log.add('listener2'); test.removeListener(listener1); @@ -137,12 +172,19 @@ void main() { log.clear(); }); - test('During notifyListeners, a listener was added and removed immediately', () { + test('During notifyListeners, a listener was added and removed immediately', + () { final TestNotifier source = TestNotifier(); final List log = []; - void listener3() { log.add('listener3'); } - void listener2() { log.add('listener2'); } + void listener3() { + log.add('listener3'); + } + + void listener2() { + log.add('listener2'); + } + void listener1() { log.add('listener1'); source.addListener(listener2); @@ -167,7 +209,10 @@ void main() { log.add('selfRemovingListener'); source.removeListener(selfRemovingListener); } - void listener1() { log.add('listener1'); } + + void listener1() { + log.add('listener1'); + } source.addListener(listener1); source.addListener(selfRemovingListener); @@ -178,7 +223,9 @@ void main() { expect(log, ['listener1', 'selfRemovingListener', 'listener1']); }); - test('If the first listener removes itself, notifyListeners still notify all listeners', () { + test( + 'If the first listener removes itself, notifyListeners still notify all listeners', + () { final TestNotifier source = TestNotifier(); final List log = []; @@ -186,6 +233,7 @@ void main() { log.add('selfRemovingListener'); source.removeListener(selfRemovingListener); } + void listener1() { log.add('listener1'); } @@ -205,8 +253,13 @@ void main() { final List log = []; final Listenable merged = Listenable.merge([source1, source2]); - void listener1() { log.add('listener1'); } - void listener2() { log.add('listener2'); } + void listener1() { + log.add('listener1'); + } + + void listener2() { + log.add('listener2'); + } merged.addListener(listener1); source1.notify(); @@ -236,8 +289,11 @@ void main() { final TestNotifier source2 = TestNotifier(); final List log = []; - final Listenable merged = Listenable.merge([null, source1, null, source2, null]); - void listener() { log.add('listener'); } + final Listenable merged = + Listenable.merge([null, source1, null, source2, null]); + void listener() { + log.add('listener'); + } merged.addListener(listener); source1.notify(); @@ -252,7 +308,9 @@ void main() { final List log = []; final Listenable merged = Listenable.merge([source1, source2]); - void listener() { log.add('listener'); } + void listener() { + log.add('listener'); + } merged.addListener(listener); source1.notify(); @@ -269,22 +327,32 @@ void main() { test('Cannot use a disposed ChangeNotifier', () { final TestNotifier source = TestNotifier(); source.dispose(); - expect(() { source.addListener(() { }); }, throwsFlutterError); - expect(() { source.removeListener(() { }); }, throwsFlutterError); - expect(() { source.dispose(); }, throwsFlutterError); - expect(() { source.notify(); }, throwsFlutterError); + expect(() { + source.addListener(() {}); + }, throwsFlutterError); + expect(() { + source.removeListener(() {}); + }, throwsFlutterError); + expect(() { + source.dispose(); + }, throwsFlutterError); + expect(() { + source.notify(); + }, throwsFlutterError); }); test('Value notifier', () { final ValueNotifier notifier = ValueNotifier(2.0); final List log = []; - void listener() { log.add(notifier.value); } + void listener() { + log.add(notifier.value); + } notifier.addListener(listener); notifier.value = 3.0; - expect(log, equals([ 3.0 ])); + expect(log, equals([3.0])); log.clear(); notifier.value = 3.0; @@ -325,9 +393,10 @@ void main() { final TestNotifier source1 = TestNotifier(); final TestNotifier source2 = TestNotifier(); - void fakeListener() { } + void fakeListener() {} - final Listenable listenableUnderTest = Listenable.merge([source1, source2]); + final Listenable listenableUnderTest = + Listenable.merge([source1, source2]); expect(source1.isListenedTo, isFalse); expect(source2.isListenedTo, isFalse); listenableUnderTest.addListener(fakeListener); @@ -339,12 +408,11 @@ void main() { expect(source2.isListenedTo, isFalse); }); - test('hasListeners', () { final HasListenersTester notifier = HasListenersTester(true); expect(notifier.testHasListeners, isFalse); - void test1() { } - void test2() { } + void test1() {} + void test2() {} notifier.addListener(test1); expect(notifier.testHasListeners, isTrue); notifier.addListener(test1); @@ -388,12 +456,94 @@ void main() { } expect(error, isNotNull); expect(error!, isFlutterError); - expect(error.toStringDeep(), equalsIgnoringHashCodes( - 'FlutterError\n' - ' A TestNotifier was used after being disposed.\n' - ' Once you have called dispose() on a TestNotifier, it can no\n' - ' longer be used.\n' - )); + expect( + error.toStringDeep(), + equalsIgnoringHashCodes('FlutterError\n' + ' A TestNotifier was used after being disposed.\n' + ' Once you have called dispose() on a TestNotifier, it can no\n' + ' longer be used.\n')); }); + test('notifyListener can be called recursively', () { + final Counter counter = Counter(); + final List log = []; + + void listener1() { + log.add('listener1'); + if (counter.value < 0) { + counter.value = 0; + } + } + + counter.addListener(listener1); + counter.notify(); + expect(log, ['listener1']); + log.clear(); + + counter.value = 3; + expect(log, ['listener1']); + log.clear(); + + counter.value = -2; + expect(log, ['listener1', 'listener1']); + log.clear(); + }); + + test('Remove Listeners while notifying on a list which will not resize', () { + final TestNotifier test = TestNotifier(); + final List log = []; + final List listeners = []; + + void autoRemove() { + // We remove 4 listeners. + // We will end up with (13-4 = 9) listeners. + test.removeListener(listeners[1]); + test.removeListener(listeners[3]); + test.removeListener(listeners[4]); + test.removeListener(autoRemove); + } + + test.addListener(autoRemove); + + // We add 12 more listeners. + for (int i = 0; i < 12; i++) { + void listener() { + log.add('listener$i'); + } + + listeners.add(listener); + test.addListener(listener); + } + + final List remainingListenerIndexes = [ + 0, + 2, + 5, + 6, + 7, + 8, + 9, + 10, + 11 + ]; + final List expectedLog = + remainingListenerIndexes.map((int i) => 'listener$i').toList(); + + test.notify(); + expect(log, expectedLog); + + log.clear(); + // We expect to have the same result after the removal of previous listeners. + test.notify(); + expect(log, expectedLog); + + // We remove all other listeners. + for (int i = 0; i < remainingListenerIndexes.length; i++) { + test.removeListener(listeners[remainingListenerIndexes[i]]); + } + + log.clear(); + test.notify(); + expect(log, []); + }); }