-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
Memoize Locale.hashCode #33140
Memoize Locale.hashCode #33140
Conversation
countryCode == '' ? null : countryCode, | ||
); | ||
// Memoize this value since languageCode and countryCode require lookups. | ||
static final Expando<int> _hashCode = Expando<int>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expandos aren't free - how much is the lookup in the expando costing vs calculating the hashCode outright?
One thing I wonder about is - locale doesn't change all that often (assumed below in the memoization for toString).
Could we be using a better data structure that would make it cheaper/faster to lookup the last used locale when we do lookups? I'm not sure where the lookup code is happening though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I just read your comment in the issue that seems to answer the first part of my question. The second part could be addressed independent from this patch)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expandos aren't free - how much is the lookup in the expando costing vs calculating the hashCode outright?
Right, napkin math we are exchanging (1 map lookup) for (2 map lookups, 3 calls to hashCode, 1 hashValue calls, 1 null comparison). I ran a microbenchmark I described in the description and this was 70% faster. The memory cost is just one int per Locale. Since Locale is const there shouldn't be many of them.
One thing I wonder about is - locale doesn't change all that often (assumed below in the memoization for toString).
I don't think it has to do with changing the Locale, looks like the Map of all the Locales is being generated no matter the selected Locale.
Could we be using a better data structure that would make it cheaper/faster to lookup the last used locale when we do lookups? I'm not sure where the lookup code is happening though.
I'm not sure that would be faster since the comparison of equality to see if you are talking to the last Locale may be more expensive than looking up the Expando which is presumably looking into a map based on an identity hash which is readily available. The equality check requires 2 map lookups (into the deprecated tables). Plus it wouldn't help when rebuilding a map of multiple Locales, which seems to be happening in Gallery's case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way to make it faster would be to check if they're identical, which for const instances that are the same should evaluate to true without doing the operator== check.
Locale
where deprecated countries would return all the same data, but the would not be considered==
I made this change because
Locale
is sometimes used as a key into a map, as it is in Flutter gallery. When it is used in a Map it'shashCode
is calculated, which is based on the country code and the language code, which requires lookups into these maps.The reason I identified this as a potential performance fix is that I setup Flutter to continuously build frames and navigated through the Material section of Flutter gallery and noticed that
![Screen Shot 2022-05-03 at 10 47 11 AM](https://user-images.githubusercontent.com/30870216/166851999-b39ee1f9-b303-4e96-aa6f-b37a4b5839c5.png)
Locale.hashCode
shows up pretty high. I also saw other Flutter users in Google using Locale as a key.In local benchmarks on iOS this the following benchmark was 70% faster:
Note:
customer: money
.Pre-launch Checklist
writing and running engine tests.
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.