Sort Element.dependencies during toString #49318
Conversation
fluttergithubbot
added
the
framework
| final List<InheritedElement> sortedDependencies = _dependencies | ||
| .toList() | ||
| ..sort((InheritedElement a, InheritedElement b) => | ||
| a.widget.runtimeType.toString().compareTo(b.widget.runtimeType.toString())); |
There was a problem hiding this comment.
There is a helper method that @dnfield wrote (I think?) to avoid accessing widget.runtimeType.toString directly, which is slow.
There was a problem hiding this comment.
it's in foundation - but I don't think it would help here, because you're actually trying to sort the string results here...
In theory this is only called from debug code.
I strongly suspect that if the tests care about order, this isn't helping anything. And if they don't care about order, we should probably just make them tolerate out of order items rather than putting this in the framework though.
Unless we just stop using a HashSet where we say we're using a Set :)
There was a problem hiding this comment.
This is the test that cares about order:
The stringified
Stepper widget displays its dependencies.According to the test I added, it should help.
dnfield
left a comment
dnfield
left a comment
There was a problem hiding this comment.
Why are we using a HashSet instead of the default LinkedHashSet, which would preserve order?
If we are using a HashSet, why are we advertising it as a Set, which by the spec says its ordered?
|
A lot of time was spent optimizing Inherited elements. HashSets/Maps are notably faster than the default options. |
|
@jonahwilliams - do we have a benchmark tracking that? |
|
Almost every benchmark will be inadvertently affected by InheritedElement lookup |
|
For this specific instance, we only call I'm not convinced that this specific one would be detrimental to performance to change to a regular |
|
@dnfield If we only call contains in assert as you said, should it be even better a |
|
A list would allow duplicates. |
|
Still, changing |
|
This would change an O(1) set of algorithms into O(N log N). The solution is to make the tests that depend on the order more resilient. |
6 participants
Description
Since
Element.dependenciesis aHashSet, its order is not guaranteed, which can cause problem if someone relies on its exact string value. This is actually observed in #48453, wherestepper_test.dartcompares the exact value of an error that includes the string ofStepper, and the order isn't even consistent between desktop and web.This PR fixes this issue by sorting
dependenciesby the widget name. I assume this should be sufficient in most cases, since the most common way of adding a dependency isdependOnInheritedWidgetOfExactType, in which case there should be no conflicted widget types.Related Issues
Tests
I added the following 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
Did any tests fail when you ran them? Please read Handling breaking changes.