Bug 143879 – Network Cache: Read resource record and body in parallel

• Component: Page Loading
• Status: RESOLVED FIXED

[Antti Koivisto]

We can lower retrieve latency by fetching in parallel.

[Antti Koivisto]

Created attachment 251018 [details]
patch

[Antti Koivisto]

Created attachment 251023 [details]
more thread safety

[Chris Dumez]

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.

[Chris Dumez]

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.

[Antti Koivisto]

https://trac.webkit.org/r182954

[Antti Koivisto]

(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.