Bug 122142 - Release expired pages in the page cache more often
Summary: Release expired pages in the page cache more often
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Performance
Depends on:
Blocks:
 
Reported: 2013-09-30 23:54 PDT by Iago Toral
Modified: 2016-01-02 08:52 PST (History)
10 users (show)

See Also:


Attachments
Patch (2.58 KB, patch)
2013-10-09 00:36 PDT, Iago Toral
beidson: review-
Details | Formatted Diff | Diff
Patch using timers (5.67 KB, patch)
2013-10-10 03:00 PDT, Iago Toral
beidson: review-
Details | Formatted Diff | Diff
Patch using a timer in PageCache (6.09 KB, patch)
2013-10-11 02:35 PDT, Iago Toral
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch using a timer in PageCache, avoids changing public method signature (6.30 KB, patch)
2013-10-15 00:13 PDT, Iago Toral
no flags Details | Formatted Diff | Diff
Updated patch (6.91 KB, patch)
2013-10-22 02:51 PDT, Iago Toral
no flags Details | Formatted Diff | Diff
WK2 API test (11.01 KB, patch)
2013-10-22 02:55 PDT, Iago Toral
no flags Details | Formatted Diff | Diff
Updated patch (7.03 KB, patch)
2013-10-22 23:27 PDT, Iago Toral
mcatanzaro: review-
Details | Formatted Diff | Diff
WK2 API test (11.12 KB, patch)
2013-10-22 23:28 PDT, Iago Toral
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Iago Toral 2013-09-30 23:54:14 PDT
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.
Comment 1 Iago Toral 2013-10-09 00:36:30 PDT
Created attachment 213757 [details]
Patch

Suggested patch, prunes expired pages every time we check if a new page can be cached.
Comment 2 Brady Eidson 2013-10-09 08:49:58 PDT
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.
Comment 3 Iago Toral 2013-10-09 23:04:36 PDT
Thanks for the feedback Brady, using a timer makes sense, I'll update the patch accordingly.
Comment 4 Iago Toral 2013-10-10 03:00:24 PDT
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.
Comment 5 zalan 2013-10-10 09:30:53 PDT
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.)
Comment 6 Brady Eidson 2013-10-10 09:46:56 PDT
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.
Comment 7 Iago Toral 2013-10-11 02:35:43 PDT
Created attachment 213977 [details]
Patch using a timer in PageCache

Updated patch according to review comments.
Comment 8 Build Bot 2013-10-11 03:02:55 PDT
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
Comment 9 Build Bot 2013-10-11 03:16:56 PDT
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
Comment 10 Iago Toral 2013-10-15 00:13:13 PDT
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.
Comment 11 Iago Toral 2013-10-20 23:34:03 PDT
Any thoughts on my last patch?
Comment 12 Brady Eidson 2013-10-21 10:08:32 PDT
(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.
Comment 13 Iago Toral 2013-10-22 02:51:29 PDT
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.
Comment 14 Iago Toral 2013-10-22 02:55:46 PDT
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?
Comment 15 Iago Toral 2013-10-22 23:27:26 PDT
Created attachment 214932 [details]
Updated patch

Previous patch was not generated from the root directory. This one should apply fine.
Comment 16 Iago Toral 2013-10-22 23:28:24 PDT
Created attachment 214933 [details]
WK2 API test

Previous patch was not generated from the root directory. This one should apply fine.
Comment 17 Iago Toral 2013-10-29 00:35:12 PDT
Any comments on the WK2 API patch?
Any pointers to address the issue with the default expiration time setting?
Comment 18 Michael Catanzaro 2016-01-02 08:52:14 PST
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.