We should add the capability to cap the maximum time we'll use a partitioned cache entry for prevalent resources.
<rdar://problem/39246837>
Created attachment 350075 [details] Patch
Created attachment 350077 [details] Patch
New patch to fix the non-Cocoa platform issues.
Created attachment 350079 [details] Patch
Created attachment 350080 [details] Patch
Comment on attachment 350080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350080&action=review > Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:194 > + if (maxAge && hasReachedPrevalentResourceAgeCap(entry.response(), entry.timeStamp(), maxAge.value())) > + return UseDecision::NoDueToPrevalentResourceAgeCap; What does 'Prevalent' signify here? It seems to be just an age cap.
Comment on attachment 350080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350080&action=review > Source/WebCore/platform/network/NetworkStorageSession.h:193 > + std::optional<Seconds> m_cacheMaxAgeCapForPrevalentResources { std::nullopt }; nit: { std::nullopt } is not needed, this is the default behavior. > Source/WebKit/NetworkProcess/NetworkProcess.cpp:477 > +void NetworkProcess::setCacheMaxAgeCapForPrevalentResources(PAL::SessionID sessionID, double seconds, uint64_t contextId) Should use Seconds type. > Source/WebKit/NetworkProcess/NetworkProcess.h:149 > + void setCacheMaxAgeCapForPrevalentResources(PAL::SessionID, double seconds, uint64_t contextId); Should use Seconds type. > Source/WebKit/NetworkProcess/NetworkProcess.messages.in:91 > + SetCacheMaxAgeCapForPrevalentResources(PAL::SessionID sessionID, double seconds, uint64_t contextId) Should use Seconds type, see for e.g.: Source/WebKit/WebProcess/FullScreen/WebFullScreenManager.messages.in: SetFullscreenAutoHideDuration(Seconds duration) > Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:343 > + std::optional<Seconds> maxAgeCap = std::nullopt; = std::nullopt is not needed here. > Source/WebKit/NetworkProcess/cache/NetworkCache.h:116 > + void retrieve(const WebCore::ResourceRequest&, const GlobalFrameID&, const PAL::SessionID, RetrieveCompletionHandler&&); const is not needed here since it is not a reference. > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.h:96 > + typedef void (*WKWebsiteDataStoreStatisticsResetToConsistentStateFunction)(void* functionContext); Indentation problem. > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:472 > + send(Messages::NetworkProcess::SetCacheMaxAgeCapForPrevalentResources(sessionID, seconds.seconds(), contextId), 0); seconds.seconds() -> seconds > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:840 > + RunLoop::main().dispatch([weakThis = makeWeakPtr(*this), store = makeRef(m_store), seconds] () { weakThis is not needed here, it is unused. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1351 > + if (auto networkProcess = processPool->networkProcess()) nit: auto* > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1366 > + if (auto networkProcess = processPool->networkProcess()) nit: auto* > LayoutTests/http/tests/resourceLoadStatistics/cap-cache-max-age-for-prevalent-resource.html:26 > + setTimeout(secondFetch, 1500); This test is likely very slow. > LayoutTests/http/tests/resourceLoadStatistics/resources/cached-permanent-redirect.php:10 > +// header("Last-Modified: Wed, 21 Aug 2018 07:28:00 GMT", true); Should this be removed?
In the case of resources used everyday, we would refetch them from scratch after the cap. This is a bit suboptimal here. Shouldn't we restrict that to only the resources that are not used at all for a given amount of time? Or maybe, have two caps, one for revalidating the resource even if still fresh and one for fully removing the resource. If the resource does not get revalidated, the second cap will kick in at some point and remove it.
(In reply to Antti Koivisto from comment #7) > Comment on attachment 350080 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=350080&action=review > > > Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:194 > > + if (maxAge && hasReachedPrevalentResourceAgeCap(entry.response(), entry.timeStamp(), maxAge.value())) > > + return UseDecision::NoDueToPrevalentResourceAgeCap; > > What does 'Prevalent' signify here? It seems to be just an age cap. It is the term used by Resource Load Statistics. "Prevalent" == has cross-site tracking abilities.
(In reply to youenn fablet from comment #9) > In the case of resources used everyday, we would refetch them from scratch > after the cap. > This is a bit suboptimal here. > > Shouldn't we restrict that to only the resources that are not used at all > for a given amount of time? > Or maybe, have two caps, one for revalidating the resource even if still > fresh and one for fully removing the resource. > If the resource does not get revalidated, the second cap will kick in at > some point and remove it. Yes, you and I talked about this and I got some further suggestions from Antti offline. I will land this, file follow-up bugs, and iterate until we have something we fell is worth doing perf testing on.
(In reply to Chris Dumez from comment #8) > Comment on attachment 350080 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=350080&action=review > > > Source/WebCore/platform/network/NetworkStorageSession.h:193 > > + std::optional<Seconds> m_cacheMaxAgeCapForPrevalentResources { std::nullopt }; > > nit: { std::nullopt } is not needed, this is the default behavior. > > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:477 > > +void NetworkProcess::setCacheMaxAgeCapForPrevalentResources(PAL::SessionID sessionID, double seconds, uint64_t contextId) > > Should use Seconds type. > > > Source/WebKit/NetworkProcess/NetworkProcess.h:149 > > + void setCacheMaxAgeCapForPrevalentResources(PAL::SessionID, double seconds, uint64_t contextId); > > Should use Seconds type. > > > Source/WebKit/NetworkProcess/NetworkProcess.messages.in:91 > > + SetCacheMaxAgeCapForPrevalentResources(PAL::SessionID sessionID, double seconds, uint64_t contextId) > > Should use Seconds type, see for e.g.: > Source/WebKit/WebProcess/FullScreen/WebFullScreenManager.messages.in: > SetFullscreenAutoHideDuration(Seconds duration) > > > Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:343 > > + std::optional<Seconds> maxAgeCap = std::nullopt; > > = std::nullopt is not needed here. > > > Source/WebKit/NetworkProcess/cache/NetworkCache.h:116 > > + void retrieve(const WebCore::ResourceRequest&, const GlobalFrameID&, const PAL::SessionID, RetrieveCompletionHandler&&); > > const is not needed here since it is not a reference. > > > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.h:96 > > + typedef void (*WKWebsiteDataStoreStatisticsResetToConsistentStateFunction)(void* functionContext); > > Indentation problem. > > > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:472 > > + send(Messages::NetworkProcess::SetCacheMaxAgeCapForPrevalentResources(sessionID, seconds.seconds(), contextId), 0); > > seconds.seconds() -> seconds > > > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:840 > > + RunLoop::main().dispatch([weakThis = makeWeakPtr(*this), store = makeRef(m_store), seconds] () { > > weakThis is not needed here, it is unused. > > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1351 > > + if (auto networkProcess = processPool->networkProcess()) > > nit: auto* > > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1366 > > + if (auto networkProcess = processPool->networkProcess()) > > nit: auto* > > > LayoutTests/http/tests/resourceLoadStatistics/cap-cache-max-age-for-prevalent-resource.html:26 > > + setTimeout(secondFetch, 1500); > > This test is likely very slow. I'll see if I can allow a cache cap setting of zero for testing. > > LayoutTests/http/tests/resourceLoadStatistics/resources/cached-permanent-redirect.php:10 > > +// header("Last-Modified: Wed, 21 Aug 2018 07:28:00 GMT", true); > > Should this be removed? Thanks! I will address all of the above before landing.
Created attachment 350137 [details] Patch for landing
Comment on attachment 350137 [details] Patch for landing Clearing flags on attachment: 350137 Committed r236216: <https://trac.webkit.org/changeset/236216>
All reviewed patches have been landed. Closing bug.
The three follow-up bugs have been filed and related. Thanks for all the comments!
I also had some doubts about whether this strategy will really solve any problems at all.