Bug 222555 - Use PriorityQueue in NetworkCache::Storage
Summary: Use PriorityQueue in NetworkCache::Storage
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-03-01 08:44 PST by Antti Koivisto
Modified: 2021-03-02 07:33 PST (History)
7 users (show)

See Also:

Attachments
patch (6.60 KB, patch)
2021-03-01 09:05 PST, Antti Koivisto 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
patch (6.63 KB, patch)
2021-03-01 09:53 PST, Antti Koivisto 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2021-03-01 08:44:45 PST 
Instead of Vector<Deque<>>.
Comment 1 Antti Koivisto 2021-03-01 09:05:27 PST 
Created attachment 421832 [details]
patch
Comment 2 Keith Miller 2021-03-01 09:29:55 PST 
Comment on attachment 421832 [details]
patch

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

Overall, looks good to me. Glad someone else is using the PriorityQueue!

> Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp:797
> +    size_t pendingCount = m_pendingReadOperations.size();

I think you need an UNUSED_PARAM here.
Comment 3 Antti Koivisto 2021-03-01 09:53:49 PST 
Created attachment 421840 [details]
patch
Comment 4 Chris Dumez 2021-03-01 10:00:57 PST 
Comment on attachment 421840 [details]
patch

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

r=me

> Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp:61
> +    static uint64_t ordinal;

I had to look up ordinal in a dictionary but it holds up :)

> Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.h:187
> +    PriorityQueue<std::unique_ptr<ReadOperation>, &isHigherPriority> m_pendingReadOperations;

We may be able to use UniqueRef instead of unique_ptr?
Comment 5 Antti Koivisto 2021-03-01 10:16:19 PST 
> We may be able to use UniqueRef instead of unique_ptr?

Probably better done in a separate cleanup that fixes other cases here too.
Comment 6 EWS 2021-03-01 11:50:29 PST 
Committed r273670: <https://commits.webkit.org/r273670>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421840 [details].
Comment 7 Radar WebKit Bug Importer 2021-03-01 11:51:15 PST 
<rdar://problem/74882108>
Comment 8 Alexey Proskuryakov 2021-03-01 13:41:18 PST 
Comment on attachment 421840 [details]
patch

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

>> Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp:61
>> +    static uint64_t ordinal;
> 
> I had to look up ordinal in a dictionary but it holds up :)

Maybe ASSERT(isMainThread()) or whatever thread this is supposed to be on here? Seems like no reason not to.
Comment 9 Antti Koivisto 2021-03-02 07:33:43 PST 
> Maybe ASSERT(isMainThread()) or whatever thread this is supposed to be on
> here? Seems like no reason not to.

Good idea!