Bug 189711 – Resource Load Statistics: Add optional cap on partitioned cache max age

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[John Wilander]

We should add the capability to cap the maximum time we'll use a partitioned cache entry for prevalent resources.

[John Wilander]

<rdar://problem/39246837>

[John Wilander]

Created attachment 350075 [details]
Patch

[John Wilander]

Created attachment 350077 [details]
Patch

[John Wilander]

New patch to fix the non-Cocoa platform issues.

[John Wilander]

Created attachment 350079 [details]
Patch

[John Wilander]

Created attachment 350080 [details]
Patch

[Antti Koivisto]

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.

[Chris Dumez]

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?

[youenn fablet]

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.

[John Wilander]

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

[John Wilander]

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

[John Wilander]

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

[John Wilander]

Created attachment 350137 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 350137 [details]
Patch for landing

Clearing flags on attachment: 350137

Committed r236216: <https://trac.webkit.org/changeset/236216>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[John Wilander]

The three follow-up bugs have been filed and related. Thanks for all the comments!

[Antti Koivisto]

I also had some doubts about whether this strategy will really solve any problems at all.