Bug 189711 - Resource Load Statistics: Add optional cap on partitioned cache max age
Summary: Resource Load Statistics: Add optional cap on partitioned cache max age
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-09-18 13:54 PDT by John Wilander
Modified: 2018-09-20 04:27 PDT (History)
10 users (show)

See Also:


Attachments
Patch (49.66 KB, patch)
2018-09-18 16:50 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (52.75 KB, patch)
2018-09-18 17:02 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (52.79 KB, patch)
2018-09-18 17:11 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (52.77 KB, patch)
2018-09-18 17:21 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch for landing (52.47 KB, patch)
2018-09-19 11:41 PDT, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 2018-09-18 13:54:07 PDT
We should add the capability to cap the maximum time we'll use a partitioned cache entry for prevalent resources.
Comment 1 John Wilander 2018-09-18 13:54:26 PDT
<rdar://problem/39246837>
Comment 2 John Wilander 2018-09-18 16:50:22 PDT
Created attachment 350075 [details]
Patch
Comment 3 John Wilander 2018-09-18 17:02:15 PDT
Created attachment 350077 [details]
Patch
Comment 4 John Wilander 2018-09-18 17:02:42 PDT
New patch to fix the non-Cocoa platform issues.
Comment 5 John Wilander 2018-09-18 17:11:14 PDT
Created attachment 350079 [details]
Patch
Comment 6 John Wilander 2018-09-18 17:21:23 PDT
Created attachment 350080 [details]
Patch
Comment 7 Antti Koivisto 2018-09-19 06:55:52 PDT
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 8 Chris Dumez 2018-09-19 09:03:07 PDT
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?
Comment 9 youenn fablet 2018-09-19 10:24:45 PDT
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.
Comment 10 John Wilander 2018-09-19 10:36:05 PDT
(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.
Comment 11 John Wilander 2018-09-19 10:37:38 PDT
(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.
Comment 12 John Wilander 2018-09-19 10:38:48 PDT
(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.
Comment 13 John Wilander 2018-09-19 11:41:21 PDT
Created attachment 350137 [details]
Patch for landing
Comment 14 WebKit Commit Bot 2018-09-19 12:20:04 PDT
Comment on attachment 350137 [details]
Patch for landing

Clearing flags on attachment: 350137

Committed r236216: <https://trac.webkit.org/changeset/236216>
Comment 15 WebKit Commit Bot 2018-09-19 12:20:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 John Wilander 2018-09-19 12:23:15 PDT
The three follow-up bugs have been filed and related. Thanks for all the comments!
Comment 17 Antti Koivisto 2018-09-20 04:27:37 PDT
I also had some doubts about whether this strategy will really solve any problems at all.