Bug 222828 - Update ApplicationCacheStorage::originsWithCache() to return a HashSet<SecurityOriginData> instead of a Vector<Ref<SecurityOrigin>>
Summary: Update ApplicationCacheStorage::originsWithCache() to return a HashSet<Securi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-03-05 14:13 PST by Chris Dumez
Modified: 2021-03-05 17:24 PST (History)
6 users (show)

See Also:


Attachments
Patch (18.49 KB, patch)
2021-03-05 14:22 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (20.14 KB, patch)
2021-03-05 15:38 PST, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-03-05 14:13:32 PST
Update ApplicationCacheStorage::originsWithCache() to return a HashSet<SecurityOriginData> instead of a Vector<Ref<SecurityOrigin>>.
Comment 1 Chris Dumez 2021-03-05 14:22:56 PST
Created attachment 422419 [details]
Patch
Comment 2 Chris Dumez 2021-03-05 15:38:26 PST
Created attachment 422443 [details]
Patch
Comment 3 Chris Dumez 2021-03-05 17:05:33 PST
Comment on attachment 422443 [details]
Patch

Patch is ready for review.
Comment 4 Darin Adler 2021-03-05 17:09:41 PST
Comment on attachment 422443 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=422443&action=review

> Source/WebCore/ChangeLog:11
> +        Update ApplicationCacheStorage::originsWithCache() to return a HashSet<SecurityOriginData> instead of a Vector<Ref<SecurityOrigin>>.
> +        Using a HashSet makes sure we do not return duplicates (which do occur as per ApplicationCacheStorage implementation) and makes typing
> +        more consistent with other similar storage functions. Using SecurityOriginData is more lightweight and sufficient for our use cases.
> +        It also takes care of a FIXME comment in WebsiteDataStore::fetchDataAndApply about switching to SecurityOriginData.

The downside of switching to HashSet is that any algorithms that involve iterating through the origins now iterate in an unpredictable order. As long as that’s not an issue, this will be great.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:536
> +            for (auto& origin : storage->originsWithCache()) {

This is a situation where we might want to consider some kind of sorting so we have predictable behavior. I worry about creating a vector that preserves the randomness of hash table order and doing anything with that vector.

In the early days of Safari some of our most difficult to diagnose performance and correctness bugs were hard to reproduce because they depended on hash ordering. Although the biggest one had a property that this does not, which is that the hash was almost global (I think it was used by NSNotificationCenter) so seemingly unrelated code to add something was having an effect on something seemingly unrelated.
Comment 5 Chris Dumez 2021-03-05 17:23:59 PST
Comment on attachment 422443 [details]
Patch

Clearing flags on attachment: 422443

Committed r274022 (234958@main): <https://commits.webkit.org/234958@main>
Comment 6 Chris Dumez 2021-03-05 17:24:02 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 EWS 2021-03-05 17:24:15 PST
Tools/Scripts/svn-apply failed to apply attachment 422443 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 8 Radar WebKit Bug Importer 2021-03-05 17:24:18 PST
<rdar://problem/75120436>