Improve perf on system fonts #94826
Improve perf on system fonts #94826
Conversation
flutter-dashboard
bot
added
the
framework
| void removeListener(VoidCallback listener) { | ||
| _systemFontsCallbacks.remove(listener); | ||
| } | ||
| class _SystemFontsNotifier extends ChangeNotifier { |
There was a problem hiding this comment.
the changeNotifier uses list and the text renderobject add/remove itself in attach and detach, it may depends on how you test it there may be performance regression as well. Is it possible to override the hashcode method of the closure?
There was a problem hiding this comment.
Hmm. Attach and detach might not be the right place for this - now that RO has dispose, perhaps we should move it to creation and disposal?
There was a problem hiding this comment.
Can you explain more? the detached RO doesn't need to be notified about font change
There was a problem hiding this comment.
Ahh ok. I'm running a benchmark like this: https://gist.github.com/dnfield/51b8f5ee3a0439b47d2841fc688d95c6, where this change is showing significant improvement.
I'll try this change out in a larger "real application" benchmark as well though.
There was a problem hiding this comment.
In flutter_gallery__transitions_perf on both a Android Go and a Pixel 4 device, this all seems to be lost in noise. It improve the microbenchmark but not the real application benchmark.
Given that, I'd be fine with just waiting on a fix for hashing closures in the upstream Dart SDK.
This is on a hot path when building lots of text widgets. At least in part because of dart-lang/sdk#47873, it's significantly slower than change notifier. Running a benchmark that builds lots of text widgets in a tight loop shows improvement from this change.
This also had behavior that's slightly different from "regular" change notifiers previously, because it didn't use identity hash coding on closures, and so would allow multiple additions without calling back multiple times.