| Summary: | Network Cache: Read resource record and body in parallel | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||||
| Component: | Page Loading | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | barraclough, cdumez | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Antti Koivisto
2015-04-17 05:50:23 PDT
Created attachment 251018 [details]
patch
Created attachment 251023 [details]
more thread safety
Comment on attachment 251023 [details] more thread safety View in context: https://bugs.webkit.org/attachment.cgi?id=251023&action=review r=me > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:122 > auto filter = std::make_unique<ContentsFilter>(); Maybe rename this one to recordFilter now that we have another filter? > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:124 > size_t size = 0; Ditto, I would rename this to recordsSize for clarity as it doesn't account for bodies. > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:189 > bool Storage::mayContain(const Key& key) const Maybe we can add a mayContainBodyBlob() now? For consistency. Then rename this one to mayContainRecord(). I know it is only used in one place right now but I still like it. > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:421 > +void Storage::dispatchReadOperation(ReadOperation& read) Not a big fan of 'read' variable name. Maybe something like readOperation would be more readable here. > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:435 > + bool shouldGetBodyBlob = !m_bodyBlobFilter || m_bodyBlobFilter->mayContain(read.key.hash()); I would use a utility function like we do for the records filter already. Comment on attachment 251023 [details] more thread safety View in context: https://bugs.webkit.org/attachment.cgi?id=251023&action=review > Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:110 > + std::atomic<unsigned> finishedCount { 0 }; Not sure it is best but maybe we could use 2 booleans instead (e.g. hasReadRecord, hasReadBody). Might be a little more readable than expecting this integer to be 2. Your call. (In reply to comment #4) > Not sure it is best but maybe we could use 2 booleans instead (e.g. > hasReadRecord, hasReadBody). Might be a little more readable than expecting > this integer to be 2. Your call. That's what the first patch had and it is actually difficult to make thread safe. |