Improve routers performance #42526
Improve routers performance #42526
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. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
93b1669 to
e1a6d29
Compare
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
| @@ -14,8 +14,8 @@ typedef PointerRoute = void Function(PointerEvent event); | |||
|
|
|||
| /// A routing table for [PointerEvent] events. | |||
| class PointerRouter { | |||
| final Map<int, LinkedHashSet<_RouteEntry>> _routeMap = <int, LinkedHashSet<_RouteEntry>>{}; | |||
| final LinkedHashSet<_RouteEntry> _globalRoutes = LinkedHashSet<_RouteEntry>(); | |||
| final Map<int, LinkedHashMap<PointerRoute, Matrix4>> _routeMap = <int, LinkedHashMap<PointerRoute, Matrix4>>{}; | |||
There was a problem hiding this comment.
Instead of LinkedHashMap, can you just use Map and when instantiating use the map literal { }? LinkedHashMap is the default implementation that you get there and it reads a lot nicer.
There was a problem hiding this comment.
(This applies everywhere in this code)
| PointerEvent event, | ||
| LinkedHashMap<PointerRoute, Matrix4> referenceRoutes, | ||
| LinkedHashMap<PointerRoute, Matrix4> copiedRoutes, | ||
| ) => copiedRoutes.forEach((PointerRoute route, Matrix4 transform) { |
There was a problem hiding this comment.
formatting nit: the ) should be aligned with the void.
Also: Flutter Style only allows the => for functions that fit in one line. This one doesn't, so please change this to use { } style.
There was a problem hiding this comment.
Fixed. I've also broken some lines for fitting them in 80 chars in this commit. Is this OK? Or is it better to revert these changes?
| PointerEvent event, | ||
| LinkedHashMap<PointerRoute, Matrix4> referenceRoutes, | ||
| LinkedHashMap<PointerRoute, Matrix4> copiedRoutes, | ||
| ) => copiedRoutes.forEach((PointerRoute route, Matrix4 transform) { |
There was a problem hiding this comment.
If you're concerned about performance in this code path: It may be faster to use a regular for loop over the keys then calling forEach. If you have a benchmark for this, you may want to try that.
There was a problem hiding this comment.
After reading this commit I thought that 'for-in' loop could be really faster, however, I decided to create benchmark tests for checking this supposition. And tests show that 'forEach' is faster than 'for-in'.
You can view these benchmark tests in the repository, two code blocks are compared there. These ones:
map.forEach((String key, int value) {
...
});
and
for (String key in map.keys) {
...
}
d5d3bf3 to
613d903
Compare
613d903 to
1e92273
Compare
Description
Use the structure
LinkedHashMapinstead ofLinkedHashSetfor routes to improve performance. This PR should change complexity for global event dispatching fromO((n*(n + 1) / 2)toO(n).It seems the subject issue caused by performance regression after these changes because the method
Iterable.anyis extremely slower thanSet.contains( O(n) vs O(1) )Related Issues
Tests
I didn't add the tests, because this PR doesn't change the logic and must just improve performance. Please let me know if I should add some performance tests.
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?