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.
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
Proposal for improving the performances of ChangeNotifier #71900
Comments
For adding listeners we saw something different in our benchmark:
I don't really know why there is so much difference between our benchmark and the official one for now, but I can look into it. Yes, good point about the removing of listeners. If an app creates a lot of listeners it could look like a memory leak. |
True, we didn't think of that. I hope this does not cost of much of our gains. void removeAt(int index){
_length--;
if(_length *2 < _listeners!.length){
final newListeners = List.filled(_length, null);
newListeners.replaceRange(0, index -1, _listeners! );
newListeners.replaceRange(index, _length -1, _listeners!.skip(index) );
_listeners = newListeners;
}else{
for (int i = index; i < _length; i++) {
_listeners![i] = _listeners![i + 1];
}
}
} (did not actually check if that works, probably there are off-by-one errors, just an idea). It might also be faster just to use two for loops instead of replaceRange. |
I wonder if you'd get some better performance out of making I think at some point you would need to shrink the list if it grew too big, but I'd expect that not to happen too much in a real usage - these are really designed to have a relatively small number of listeners, though listeners may be added and removed over time, I would not expect the actual list size to grow much beyond say 16, which shouldn't normally be a problem. I'd suggest adding the benchmark to our microbenchmarks suite as a starting point. But this seems like a reasonable approach to me overall. |
It'd also be interesting to see if it has any impact on, e.g., the transitions perf benchmarks for gallery/new gallery. |
@letsar @knaeckeKami we already use the removeAt() function if not during a notify. So I don't see the problem with removeListener but maybe I'm just overlooking something. what could be that if our List grew before and then a lot of listeneres where removed the array could still be too big, so we could add a check in removeAt and allocate a smaller List again. @dnfield If we want to use a ´bool´ flag we can't store the handler at the same place so we would need an aditional node object, which was how my first implementation lookked like. @mraleph then recommended using only the list without addional objects which indeed was faster, so I don't expect that null safe with objects is any faster. |
@escamoteur the removeAt actually never shrinks the list, it only shift things so that all null are at the end of the list. But yes, we have to make sure that in the case a lot of listeners had been added, we shrink the list when it makes sense. For the bool flag, I think there is a misunderstanding. It would be only for making the list not nullable. I did something like that in a previous version but I can compare the two approaches. |
I think @dnfield meant that instead of List<VoidCallback?>? _listeners = List<VoidCallback?>.filled(0, null); we use List<VoidCallback?> _listeners = List<VoidCallback?>.filled(0, null);
bool _isDisposed = false; and change void dispose() {
assert(_debugAssertNotDisposed());
_listeners.clear();
_isDisposed = true;
} and also change |
@knaeckeKami I thought we had that already in? |
At a moment, yes, but I thought it could be an unnecessary object. I have to bench it. |
Yeah, I would definitely recommend avoiding making Regarding grow-shrink strategy: this is a problem which does not have a solution which works for all usage patterns, different grow/shrink strategies have different performance trade offs. I'd recommend to try collecting size histograms from real applications to see how |
I ran the benchmarks with this new _removeAt method, and the performances are still ok. void _removeAt(int index) {
_length--;
if (_length * 2 <= _listeners!.length) {
final List<VoidCallback?> newListeners =
List<VoidCallback?>.filled(_length, null);
for (int i = 0; i < index; i++) {
newListeners[i] = _listeners![i];
}
for (int i = index; i < _length; i++) {
newListeners[i] = _listeners![i + 1];
}
_listeners = newListeners;
} else {
for (int i = index; i < _length; i++) _listeners![i] = _listeners![i + 1];
}
} Concerning the @dnfield where can I find the benchmarks for Gallery you mentioned? |
If there are benchmarks that you would like to make sure we don't regress when refactoring this code in the future, please make sure to contribute those also (ideally in a PR before landing the improvement, so that we can verify that the PR does improve matters in the first place). There's already some benchmarks around this as you have noticed. |
@letsar - something like this should do it cd dev/devicelab
dart bin/run.dart -t flutter_gallery__transition_perf You can also try |
(you'll need an attached android device for those) |
@dnfield I found the tests, but I'm unable to launch them. Unhandled exception:
JSON-RPC error -32000: Server error
package:json_rpc_2/src/client.dart 123:62 Client.sendRequest
package:json_rpc_2/src/peer.dart 98:15 Peer.sendRequest
package:vm_service_client/src/scope.dart 64:23 Scope.sendRequestRaw
package:vm_service_client/src/isolate.dart 361:19 VMIsolateRef.invokeExtension
package:flutter_devicelab/framework/runner.dart 83:63 runTask |
If there are any other logs that may be helpful. Is your device unlocked? |
The device is unlocked, and I'm able to run flutter apps on it. Here the full trace:$ dart bin/run.dart -t new_gallery__transition_perf ════════════╡ ••• Running task "new_gallery__transition_perf" ••• ╞═════════════ Executing: /Users/xxxxxx/Dev/flutter_fork/flutter/bin/cache/dart-sdk/bin/dart --disable-dart-dev --enable-vm-service=0 --no-pause-isolates-on-exit bin/tasks/new_gallery__transition_perf.dart in /Users/xxxxxx/Dev/flutter_fork/flutter/dev/devicelab with environment {} |
Hmm. That seems strange. There's a patch coming to remove vm_client_service which may help this, maybe once that lands we can try again. |
I was able to run in on
Old Value notifier: "average_frame_build_time_millis": 1.174159528907923,
"90th_percentile_frame_build_time_millis": 1.698,
"99th_percentile_frame_build_time_millis": 9.582,
"worst_frame_build_time_millis": 22.488,
"missed_frame_build_budget_count": 3,
"average_frame_rasterizer_time_millis": 5.107235042735051,
"90th_percentile_frame_rasterizer_time_millis": 5.786,
"99th_percentile_frame_rasterizer_time_millis": 23.925,
"worst_frame_rasterizer_time_millis": 76.047,
"missed_frame_rasterizer_budget_count": 14,
"frame_count": 934,
"frame_rasterizer_count": 936, New value notifier: "average_frame_build_time_millis": 1.14836712913554,
"90th_percentile_frame_build_time_millis": 1.6,
"99th_percentile_frame_build_time_millis": 9.42,
"worst_frame_build_time_millis": 19.687,
"missed_frame_build_budget_count": 2,
"average_frame_rasterizer_time_millis": 5.00333049040511,
"90th_percentile_frame_rasterizer_time_millis": 5.702,
"99th_percentile_frame_rasterizer_time_millis": 22.119,
"worst_frame_rasterizer_time_millis": 77.254,
"missed_frame_rasterizer_budget_count": 14,
"frame_count": 937,
"frame_rasterizer_count": 938,
Old value notifier: "average_frame_build_time_millis": 1.5316753926701585,
"90th_percentile_frame_build_time_millis": 1.982,
"99th_percentile_frame_build_time_millis": 12.621,
"worst_frame_build_time_millis": 51.118,
"missed_frame_build_budget_count": 11,
"average_frame_rasterizer_time_millis": 6.200340091563104,
"90th_percentile_frame_rasterizer_time_millis": 6.624,
"99th_percentile_frame_rasterizer_time_millis": 27.202,
"worst_frame_rasterizer_time_millis": 321.044,
"missed_frame_rasterizer_budget_count": 23,
"frame_count": 1528,
"frame_rasterizer_count": 1529, New value notifier: "average_frame_build_time_millis": 1.5151169170476826,
"90th_percentile_frame_build_time_millis": 1.954,
"99th_percentile_frame_build_time_millis": 11.552,
"worst_frame_build_time_millis": 56.567,
"missed_frame_build_budget_count": 10,
"average_frame_rasterizer_time_millis": 6.144109660574416,
"90th_percentile_frame_rasterizer_time_millis": 6.665,
"99th_percentile_frame_rasterizer_time_millis": 32.115,
"worst_frame_rasterizer_time_millis": 157.345,
"missed_frame_rasterizer_budget_count": 25,
"frame_count": 1531,
"frame_rasterizer_count": 1532, Seems a bit faster I don't know if it's significant though. I ran it only once. |
Nice. If you want you might try running it a few more times to check for noise, but this does seem like it improves some numbers there signfiicantly percentage wise, although there are a few in there that are slightly worse (I'd wonder if that's just noise though). At any rate, this gives some more confidence that the improvements are meaningful for apps and not just something that's helping a narrow microbenchmark :) |
I ran both benchmarks 5 times with both implementations respectively. Here are the results: I'm neither a microbenchmark guru nor a statistician, and there seems to be quite some noise, especially in the worst_time and 99 percentile measurements - we would probably need more measurements or filter some of the outliers (see flutter_gallery__transition_perf - new value notifier - missed_frame_rasterizer_budget_count - Run 2). |
@knaeckeKami If we can see this in the application test so strongly, we should instrument the notifier and get numbers of add, remove, notifiy (with number of listeners), add/remove while notify, number of array reallocations. |
Yeah, would be interesting to see. Also maybe interesting for the grow/shrink strategies. |
In |
I'll create an instrumentated version then we see more. But I guess the code for a look won't be so different than having an addtional if around. |
I created this instrumented version here https://gist.github.com/escamoteur/6fd70cc9a4895e133a4b6dede6e4235f Possibly this path for the logfile has to be changed: It creates a ton of data on everey function that is called on a changenotifier. I try to create sort of UID for every change notifier so we could also observer how one behaves over a livetime. @knaeckeKami could you run this one with the performacetest of the GaleryApp? Or is there any other app that we could run with this? I hope someone of you is able afterwards to draw some conclusions on the _listener size and resizing strategy from the created data. |
Look interesting. I'll look into it tomorrow or the day after tomorrow. |
I believe any differences in the benchmark performances are due to AOT optimizations. |
@colinpoirier Sorry but I'm not able to follow what you want to say. All versions get AOT optimization. Which is the growable version? What is the OP benchmark? |
I now ran the instrumented version @escamoteur . |
Now we only need someone who can make the most sense of this data :-) |
Appreciate the conversation on this thread. I'm tentatively assigning this |
@kf6gpe there is actuall already a PR that is widely aceepted in the pipeline :-) |
Sweet! Thanks, @escamoteur . Back to P3, since we're working on it! :) |
Apologies, @escamoteur hopefully this helps clear things up. My previous comment proposed a theory with supporting data in an attempt to explain the difference in addListener performance between the screenshot of the Flutter benchmark in the original post and the table of the addlistener benchmark posted in another comment. And that lead to my question in the comment about which benchmark is more representative of in-app performance/optimization. I think I was able to answer that question. I look forward to the PR landing and hopefully in the future this implementation making it's way to Animations and other Listenables. With this being the accepted implementation, should a member of the Flutter team address this older and similar proposal? #61619 |
Closing this one as that PR is submitted. I posted some graphs on the PR showing the impact that this had on the microbenchmarks: #71947 (comment) |
This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of |
Hi👋
We (@escamoteur, @knaeckeKami, and me, but also with the help of @mraleph and @Kavantix ) found a new implementation for
ChangeNotifier
which would improve its performances, especially whennotifyListeners
is called.We created multiple benchmarks to see how our implementation behaves in multiple conditions.
These benchmarks are available here: https://github.com/knaeckeKami/changenotifier_benchmark
We measured the performances in terms of CPU time, but also the Memory Footprint, for 3 different implementations:
ChangeNotifier
implementation before this PR: Use a LinkedList to improve the performances of ChangeNotifier #62330.CPU Time
Our Benchmark
You will find below the results of our main benchmark:
The code of this benchmark is the following, and you can find it here.
This benchmark has been run on my OnePlus 8 Pro (with Android 11) with the following command:
As you can see, the Proposed implementation seems to be much faster than the previous ones.
Flutter microbenchmark
We also ran the flutter microbenchmark, for
ChangeNotifier
, available here in which we made some modifications to be able to test our 3 implementations, with 100,000 iterations for each one.We normalized the results to see how much other implementations are slower than the fastest (the implementation with a score of 1).
Here are our results:

As you can see, the Proposed implementation is the fastest for almost all iterations in this benchmark as well.
Memory Footprint
We also wanted to compare the memory footprint of these different implementations.
We didn't find an automatic way to measure this so we took an approach based on Heap Snapshots with Dart DevTools.
We created a simple app which will instantiate 1000 notifiers with 1000 listeners for each one of them:
The protocol to measure the memory footprint can be reproduced by following these steps:
name
constant to the implementation for which you want to measure the memory footprint (Initial, Current or Proposed).flutter run --profile lib/main.dart
.We also measure the memory footprint of the same app with 0 notifiers to have a reference.
These are our results:
We can see that Initial and Proposed implementations have about the same memory footprint, but the Current implementation's footprint is higher.
By removing the total memory footprint of the 0 notifiers iteration, we can compute that the Proposed implementation consumes 1.55 times less than the Current one.
We also think that this proposed implementation allocates fewer temporary objects because it doesn't create a new list for each call to
notifyListener
.Proposed implementation
You will find below the proposed implementation which lays on a custom growable list:
This implementation passes all current tests.
The main drawback of this implementation can be the source code which is more complicated than the previous ones.
But the benchmark results show a lot of benefits and we think that it could be a good opportunity to change the current implementation to this one.
If you think that's a good idea, we can do a PR ourselves and also add some tests to
ChangeNotifier
.The text was updated successfully, but these errors were encountered: