Bug 122142 – Release expired pages in the page cache more often

• Component: History
• Status: UNCONFIRMED

[Iago Toral]

Expired pages in the page cache can stay there for quite some time even though they will not be reused again. Currently, as far as I understand the code, these pages will stay in the cache until the page is requested again through the get() method (at which point it will be removed if it has expired) or until the cache is pruned due to the addition of new pages (which will only happen if new pages are loaded that meet the criteria to be stored in the page cache). This creates situations where an expired page can stay in memory for quite some time.

Even though the page cache is typically small in size, cached pages hold references to objects stored in the memory cache, so it probably makes sense to get rid of them early if possible, specially in environments where memory efficiency is particularly relevant and/or the memory cache is small.

If the rationale above makes sense, I can provide a patch to release expired pages more often. Considering that the number of pages in the cache is very small I think it should not have a significant performance penalty to check for expired pages on frequently used methods. I'd suggest doing it when calling canCache() since I think this would make sure that we release expired pages at least every time we load a new page, but it is currently a const method so maybe you have other preferences.

[Iago Toral]

Created attachment 213757 [details]
Patch

Suggested patch, prunes expired pages every time we check if a new page can be cached.

[Brady Eidson]

Comment on attachment 213757 [details]
Patch

This is a good idea.

I think relying on canCachePage() is kind of a crutch.  If we want to free pages when they expire, we should set a timer for when they expire.

This change can be tested with a WK2 API test.

[Iago Toral]

Thanks for the feedback Brady, using a timer makes sense, I'll update the patch accordingly.

[Iago Toral]

Created attachment 213862 [details]
Patch using timers

Use a timer to remove expired pages.

I have not done any API tests before so I still need to find how to do that. Looks like Tools/TestWebKitAPI/Tests/WebKit2/ShouldGoToBackForwardListItem.cpp also deals with the page cache so hopefully I can find there all I need to write the test for this. I'll update the patch when I have a working test for it.

[zalan]

Comment on attachment 213862 [details]
Patch using timers

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

> Source/WebCore/history/HistoryItem.cpp:254
> +    if (!m_expirationTimer->isActive())

Wouldn't be better to assert this? Calling startExpirationTimer() multiple times without calling stop() could indicate defective behaviour. The patch in current form would ignore that and probably release the history item sooner than it is intended (because in PageCache::add(), we recreate CachedPage object so item->m_cachedPage has a new expiration time now, and when we try to (re)start the timer (item->startExpirationTimer()), it sees it as active and fails to reset it to the new expiration timestamp.)

[Brady Eidson]

Comment on attachment 213862 [details]
Patch using timers

I don't think history items should have the timers.  HistoryItem being so intertwined with page cache and cached pages is a mistake we'd like to try to reverse.  Also we don't need multiple timers for this (increasing the size of history item for something that's used somewhat infrequently)

The page cache itself should have the timer.  When the first CachedPage goes in, the timer is started.  Additional CachedPages don't change the timer.  And when the timer fires and one page is purged out, we set it to the expiration of the next page.

[Iago Toral]

Created attachment 213977 [details]
Patch using a timer in PageCache

Updated patch according to review comments.

[Build Bot]

Comment on attachment 213977 [details]
Patch using a timer in PageCache

Attachment 213977 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/3776114

[Build Bot]

Comment on attachment 213977 [details]
Patch using a timer in PageCache

Attachment 213977 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/3954058

[Iago Toral]

Created attachment 214238 [details]
Patch using a timer in PageCache, avoids changing public method signature

I don't know why the Mac build fails with the previous patch but I guess it is related with the fact that the patch changes the signature of the public remove method to add a parameter with a default value. This new patch keeps the original method and adds a private version that takes the additional argument. Hopefully this will fix the build problem on Mac.

[Iago Toral]

Any thoughts on my last patch?

[Brady Eidson]

(In reply to comment #11)
> Any thoughts on my last patch?

My thoughts are that it looks good but - reiterating my Comment #2 - this changes WebKit2 API behavior, and therefore it'd be great to write a WebKit2 API test with it.

[Iago Toral]

Created attachment 214829 [details]
Updated patch

Updated patch: while working on the API test for this I realized that HistoryItem::hasCachedPageExpired and HistoryItem::isInPageCache are now equivalent since an expired item will no longer be in the page cache with this patch. Probably one of these should be deprecated in the future.

[Iago Toral]

Created attachment 214830 [details]
WK2 API test

WK2 API test for the new page cache behavior. The test loads two pages, then goes back to the first one after the page expiration time and checks that the page was not loaded from the page cache.

This test has an obvious problem: the default page expiration time is set to 1800 seconds but I have not found a way to alter this setting from the test, so it will timeout in its current form. Do we need new API in the injected bundle to do this or am I missing something?

[Iago Toral]

Created attachment 214932 [details]
Updated patch

Previous patch was not generated from the root directory. This one should apply fine.

[Iago Toral]

Created attachment 214933 [details]
WK2 API test

Previous patch was not generated from the root directory. This one should apply fine.

[Iago Toral]

Any comments on the WK2 API patch?
Any pointers to address the issue with the default expiration time setting?

[Michael Catanzaro]

Comment on attachment 214932 [details]
Updated patch

(In reply to comment #14)
> Do we need new API in the injected bundle to do this

Yes, I think so!

r- since the test should be included in the same patch, and it should work.