Bug 79269 – Separate PageCachePolicy from PageCache action

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Gavin Peters]

Separate PageCachePolicy from PageCache action

[Gavin Peters]

Created attachment 128270 [details]
Patch

[Philippe Normand]

Comment on attachment 128270 [details]
Patch

Attachment 128270 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11569131

[Early Warning System Bot]

Comment on attachment 128270 [details]
Patch

Attachment 128270 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11561618

[Gyuyoung Kim]

Comment on attachment 128270 [details]
Patch

Attachment 128270 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11556609

[Gavin Peters]

This isn't ready for landing, as I haven't added the build files for the other platforms.

However, here's my first thoughts on how to do this.

I really didn't like that PageCache had two implementations of policy.  One was for the debug logging, and one was for making the actual policy decisions.  They were inconsistant in a few areas; notably error pages.  Scary!  I don't like inconsistency!

So the new code does both, always: the logging compiles to a NOP on non-debug builds.  But, I don't short circuit the evaluation, even on non-debug builds.  This means we could go deep into a frame hierarchy even if we fail the pagecache early.  Is this cost worth worrying about?  If you think so, I can add it, at the cost of some simplicity in the code.

[Nate Chapin]

Comment on attachment 128270 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128270&action=review

This general strategy seems reasonable to me.

I wish we could make PageCachePolicy less verbose, but I don't see a good way to do it.  We could always go to idls and perl scripts, but I feel dirty just mentioning that idea.

> Source/WebCore/history/PageCachePolicy.cpp:53
> +
> +PageCachePolicy::FactoryFunction* PageCachePolicy::s_factory = PageCachePolicy::DefaultFactoryFunction;
> +

I'm not enough of a C++ guru to know whether this sort of static initialization is acceptable. My intuition says it is, since PageCachePolicy::DefaultFactoryFunction is a constant and shouldn't run a constructor, but I'd like to hear from someone more knowledgeable.

[Adam Barth]

Comment on attachment 128270 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128270&action=review

>> Source/WebCore/history/PageCachePolicy.cpp:53
>> +PageCachePolicy::FactoryFunction* PageCachePolicy::s_factory = PageCachePolicy::DefaultFactoryFunction;
>> +
> 
> I'm not enough of a C++ guru to know whether this sort of static initialization is acceptable. My intuition says it is, since PageCachePolicy::DefaultFactoryFunction is a constant and shouldn't run a constructor, but I'd like to hear from someone more knowledgeable.

This shouldn't introduce a static initializer.  The AppleMac build will fail if it does so you don't need to worry too much about it as long as the file is in the mac build.

[Gavin Peters]

Created attachment 128295 [details]
Patch

[Gavin Peters]

This latest patch is still not ready to land, but I added the Mac build files so we could find out about static initializers, courtesy the Mac build.

[Philippe Normand]

Comment on attachment 128295 [details]
Patch

Attachment 128295 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11574003

[Early Warning System Bot]

Comment on attachment 128295 [details]
Patch

Attachment 128295 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11574005

[Adam Barth]

Comment on attachment 128295 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128295&action=review

> Source/WebCore/ChangeLog:10
> +        This refactor separates out pagecache policy from the page cache
> +        itself.  It also adds a factory mechanism, so that ports that want
> +        to have distinct policies can override the default.

Can we skip the factory method until needed?  Generally, the way we handle this is with a static create() function in different CPP files for each port/configuration rather than using the factory pattern directly.

Also, WebKit tends to avoid introducing these sorts of abstract mechanisms until we have at least one use case.  Is the Chromium port planning to use a different PageCachePolicy?  My understanding was that Chromium doesn't use the PageCache at all.

> Source/WebCore/history/PageCachePolicy.h:53
> +    PageCachePolicy(Page*);

explicit

> Source/WebCore/history/PageCachePolicy.h:54
> +    bool CanCachePage();

WebKit uses lower-case for the initial letter of member functions.

> Source/WebCore/history/PageCachePolicy.h:59
> +protected:

private ?  Are you expecting subclasses?

[Gavin Peters]

(didn't work; i'm preparing another mac upload now, as well as other sundries, like adding the missing virtual destructor and fixing the includes).

[Brady Eidson]

Chromium doesn't even use the page cache, so I'm curious why playing in this code has sparked your interest?

Please do not land this without letting me review the final patch.

[Brady Eidson]

Please explain what you're actually trying to do with this code change.

Some (most) of things you are calling "policy" are not policy at all, but technical reasons why we can't pause a page.

Breaking them out in to some abstract design-patterny "policy factory" doesn't make the page cache better.  Solving the technical limitations on a case-by-case basis does.

[Gavin Peters]

Created attachment 129314 [details]
Patch

[Gavin Peters]

beidson, thanks for your interest!  abarth, thanks for your review!

Chromium doesn't use the page cache now, but we're considering turning it on.  In the meantime, I want to start gathering statistics from the wild, using Chrome's histogram mechanism, so I know how often pages are rejected or accepted in page cache in actual back/forward navigation in the wild, and when they are rejected, how many distinguishable causes there are, and what those causes are.  I'm particularly interested in knowing about single-cause but-not-for page cache failure rates, and double-cause but-not-for page cache failures.  How often would relaxing one restriction have gotten us more page cache?

This patch is a refactoring, and it doesn't add the abovementioned statistics.  What it does do is pull out the logic that decides on page cache out from pagecache.cpp, and unifies the debug and actual logic.  The debug logic had drifted significantly from what was actually being tested.  The debug logic was pretty awful in parts too: check out how those LOG messages are generated in the original...

In parallel I have a patch I'm working on to add histograms in Chrome using a separate PageCachePolicyChromium: this chrome specific PageCachePolicy object won't actually change Chrome's use of the PageCache, but it will record the causes of rejections in an uploaded histogram.  The histogram methods are also just generally available in WebCore (see platform/HistogramSupport.h, which compile to NOOPs on non-chrome platforms), but given how many I'm adding, I felt a separate PageCachePolicy object for chrome was justified.

I could also add these tests in #if PLATFORM(CHROMIUM)...?

The newest patch should have more hope (some hope?) of passing on more platforms.  I'm doing testing in parallel, but I wanted to share the upload.

- Per abarth's review, I've removed the factory mechanism.  I have a Chromium factory for this I'll probably upload soon, and I may re-add it there in the preferred form abarth asked for.

- I fixed some of the tests, in particular I made sure it passed beidson's latest https page cache tests.  They were good tests that caught some negations.

Questions that I still have:

- Do we want to short-circuit this logic, like the original code does, in the non-debug case?

[Early Warning System Bot]

Comment on attachment 129314 [details]
Patch

Attachment 129314 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11710066

[Gavin Peters]

Doh: I only added Mac build rules.  I'll add the rest of the platforms and re-upload my patch.

[Gyuyoung Kim]

Comment on attachment 129314 [details]
Patch

Attachment 129314 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11710069

[Gavin Peters]

Created attachment 129318 [details]
Patch

[Brady Eidson]

(In reply to comment #17)
> beidson, thanks for your interest!  abarth, thanks for your review!
> 
> Chromium doesn't use the page cache now, but we're considering turning it on.  In the meantime, I want to start gathering statistics from the wild, using Chrome's histogram mechanism, so I know how often pages are rejected or accepted in page cache in actual back/forward navigation in the wild, and when they are rejected, how many distinguishable causes there are, and what those causes are.  I'm particularly interested in knowing about single-cause but-not-for page cache failure rates, and double-cause but-not-for page cache failures.  How often would relaxing one restriction have gotten us more page cache?

Doing it in Chromium-only would be a mistake for the WebKit project.  All ports that use the PageCache would be interested in this data, and that number of ports is apparently growing.

Please find a way to do this in WebCore.

As for the substance of the patch that's here now, I hate the name Policy for this, because...

> In parallel I have a patch I'm working on to add histograms in Chrome using a separate PageCachePolicyChromium: 

In WebCore, Policy means 1 of 2 things.
1 - Implementing a policy API callback for the embedding application for some high level operation.  "decidePolicyForMIMEType", etc.
2 - Implementing a Web Platform policy, such as SecurityPolicy.h/cpp

I don't think "Policy" is a naming precedent we should apply to a platform layer, when you are now introducing to the PageCache.

Also, where's the webkit.org bug for that work, as it sounds like you're doing it at the WebCore layer?

> Questions that I still have:
> 
> - Do we want to short-circuit this logic, like the original code does, in the non-debug case?

Yes.

> This patch is a refactoring,

Refactoring needs to be a zero behavior change.  I will r- patches that don't short-circuit the logic.

[Brady Eidson]

(In reply to comment #22)

> I don't think "Policy" is a naming precedent we should apply to a platform layer, when you are now introducing to the PageCache.

s/when/which/

[Brady Eidson]

Comment on attachment 129318 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129318&action=review

> Source/WebCore/history/PageCachePolicy.cpp:125
> +    framedata.responseDenies = frame->loader()->documentLoader()->response().cacheControlContainsNoCache()

.responseDenies doesn't really fit as a name.

> Source/WebCore/history/PageCachePolicy.cpp:140
> +    return canCacheFrameImpl(framedata);

It seems entirely unnecessary to break up the gathering of all this data from the decision-making based on the data gathered.  The current code is "one big if-statement" that returns early to avoid lots of extra work, and this approach regresses that.

> Source/WebCore/history/PageCachePolicy.cpp:186
> +bool PageCachePolicy::canCacheFrameImpl(const FrameData& framedata)
> +{
> +    if (!framedata.hasDocumentLoader) {
> +        policyLog("   -There is no DocumentLoader object");
> +        return false;
> +    }

These changes to split out the data gathering then intertwine the debug-only logging code and the actual release-build logic accomplishes 3 things:
1 - Makes it harder for the debug logging to get out of sync with the actual logic.  This is good.
2 - Makes it so release builds can't short circuit during all of that data gathering when the first disqualifying condition is reached.  This is bad.
3 - Makes the code harder to follow.  This is ugly.

I strongly urge you to reconsider your approach.

[Gavin Peters]

I'm closing this issue, I'm going to take beidson's advice and upload patches that use a different approach.