fix KeySet.hashCode; enable multiple web tests

[yjbanov]

Description

Fix and enable several tests that were previously skipped/broken on the Web.

@gspencergoog Please have a look at the change in shortcuts.dart. I think I fixed a hashCode bug, but want to make sure I'm not creating a bug :)

[skia-gold]

Gold has detected one or more untriaged digests on patchset 1.
Please triage them at https://flutter-gold.skia.org/search?issue=52861.

[gspencergoog]

No, as far as I know, this isn't necessary. HashSet is stable and not based on insertion order (LinkedHashSet, on the other hand is based on insertion order). Do you have an example that shows that it's not stable?

I switched to using a HashSet in #38936 to fix the bug I think you're trying to fix.

From the docs for HashSet:

"The iteration order of the set is not specified and depends on the hashcodes of the provided elements. However, the order is stable: multiple iterations over the same set produce the same order, as long as the set is not modified."

[yjbanov]

The issue is that the insertion order is not what affects the computation of the hash code. What affects it is the order of elements we pass to hashList. While LinkedHashSet guarantees insertion order, HashSet does not guarantee any order. However, when you iterate over the elements of a HashSet it will return the elements in some order. So when we pass it to hashList the elements are iterated in some unpredictable order.

[gspencergoog]

But a consistent order, based on the hash. Isn't that all we need? I mean, your sorted version is just that: consistent and based on the hash.

[yjbanov]

Hmm, perhaps the difference is that on the VM HashSet iteration order depends only on the hashcodes of its elements while on the Web they do not. In general, AFAIK hash is not the only thing that determines the iteration order in a hash table. Two elements with different hashes can still end up in the same bucket, at which point there's no way to guarantee which of the two elements is extracted first. Let me poke at it for bit.

[yjbanov]

Confirmed. Here's the difference between Web and VM:

Web:
set2: a (hash: 97) b (hash: 98) c (hash: 99) d (hash: 100)
set3: d (hash: 100) c (hash: 99) b (hash: 98) a (hash: 97)

VM:
set2: a (hash: 97) b (hash: 98) c (hash: 99) d (hash: 100)
set3: a (hash: 97) b (hash: 98) c (hash: 99) d (hash: 100)

This was printed using:

print(set2.keys.map((k) => '${k.keyLabel} (hash: ${k.hashCode})').join(' '));
print(set3.keys.map((k) => '${k.keyLabel} (hash: ${k.hashCode})').join(' '));

[yjbanov]

Ah, you mean the first access to hashCode. So, the numbers are as follows:

• 1-key sets improved by 16%
• 2-key sets improved by 0.84%
• 3-key sets regressed by 21%
• 4-key sets regressed by 20%

This is first access only. If the hash code is used a second time anywhere at all, it's offset immediately by the cache.

How many 3- and 4-key sets do we have in Flutter?

[gspencergoog]

None, and I wouldn't expect many.

[gspencergoog]

OK, let's go ahead with your changes. Seems like a win, generally.

[yjbanov]

Thanks! I'll keep an eye on the benchmarks for regressions.

[gspencergoog]

One thing that occurred to me: since you're now sorting things by hash, is it any faster to use the default Set implementation instead of HashSet? It might be able to be constructed faster.

[fluttergithubbot]

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

Changes to golden files are considered breaking changes, so consult Handling Breaking Changes to proceed. While there are exceptions to this rule, if this patch modifies an existing golden file, it is probably not an exception. Only new golden file tests, or downstream changes like those from skia updates are considered non-breaking.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #52861 at sha 0d0cd82fd2e2c8f8cdd4fa1562471475f52400d6

[gspencergoog]