Bug 111443 - [Qt][WK1] MemoryCache is not cleaned by default
Summary: [Qt][WK1] MemoryCache is not cleaned by default
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Allan Sandfeld Jensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-05 08:29 PST by Allan Sandfeld Jensen
Modified: 2013-04-23 01:32 PDT (History)
3 users (show)

See Also:


Attachments
Patch (2.51 KB, patch)
2013-03-05 08:33 PST, Allan Sandfeld Jensen
jturcotte: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Allan Sandfeld Jensen 2013-03-05 08:29:36 PST
By default the MemoryCache does not remove dead resources, this only happens if setDeadDecodedDataDeletionInterval() has been called. In WK2 we do this in WebProcess based on the CacheModel, but in WK1 we only do it based on an undocumented dynamic property calle _q_deadDecodedDataDeletionInterval.

By default we should enable cleaning of the memoryCache as long as it is enabled following the setting of WK2.
Comment 1 Allan Sandfeld Jensen 2013-03-05 08:33:17 PST
Created attachment 191502 [details]
Patch
Comment 2 Arunprasad 2013-03-05 09:12:56 PST
Is it really needed? Because QWebPage has a dynamic property(_q_deadDecodedDataDeletionInterval) to tune this as mentioned by you. Also if we have a room for dead decoded data in the MemoryCache why should we delete the dead decoded data? Incase of cache pressure first it will remove all the dead decoded data from the MemoryCache.
Comment 3 Allan Sandfeld Jensen 2013-03-05 09:37:45 PST
(In reply to comment #2)
> Is it really needed? Because QWebPage has a dynamic property(_q_deadDecodedDataDeletionInterval) to tune this as mentioned by you. Also if we have a room for dead decoded data in the MemoryCache why should we delete the dead decoded data? Incase of cache pressure first it will remove all the dead decoded data from the MemoryCache.

Note we do not have memory pressure events in QtWebKit. Only Mac uses those. Keeping dead things out of memory means that we are less of a burden to the operating system, and less likely to have something we actually need swapped out. Plus it makes everything else faster if the operating system has more free memory.
Comment 4 Arunprasad 2013-03-05 09:53:40 PST
(In reply to comment #3)
> (In reply to comment #2)
> > Is it really needed? Because QWebPage has a dynamic property(_q_deadDecodedDataDeletionInterval) to tune this as mentioned by you. Also if we have a room for dead decoded data in the MemoryCache why should we delete the dead decoded data? Incase of cache pressure first it will remove all the dead decoded data from the MemoryCache.
> 
> Note we do not have memory pressure events in QtWebKit. Only Mac uses those. Keeping dead things out of memory means that we are less of a burden to the operating system, and less likely to have something we actually need swapped out. Plus it makes everything else faster if the operating system has more free memory.

Yes. We don't have memory pressure events. Suppose WebCore requests a new resource then MemoryCache will be audited to ensure whether it has enough space, if it don't have then it starts purging the things from DeadDecoded resource to Live Resources which is used long back(based on LRU). It is not a kind of event, but it is like a checking for the room before adding new resources. :)
Comment 5 Allan Sandfeld Jensen 2013-03-07 05:39:29 PST
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Is it really needed? Because QWebPage has a dynamic property(_q_deadDecodedDataDeletionInterval) to tune this as mentioned by you. Also if we have a room for dead decoded data in the MemoryCache why should we delete the dead decoded data? Incase of cache pressure first it will remove all the dead decoded data from the MemoryCache.
> > 
> > Note we do not have memory pressure events in QtWebKit. Only Mac uses those. Keeping dead things out of memory means that we are less of a burden to the operating system, and less likely to have something we actually need swapped out. Plus it makes everything else faster if the operating system has more free memory.
> 
> Yes. We don't have memory pressure events. Suppose WebCore requests a new resource then MemoryCache will be audited to ensure whether it has enough space, if it don't have then it starts purging the things from DeadDecoded resource to Live Resources which is used long back(based on LRU). It is not a kind of event, but it is like a checking for the room before adding new resources. :)

True, but imagine you have a large memory cache setup the way we do WK2. 128 Mbyte total with a maxDeadbytes of 64MByte. Every time the cache reached 128, it will be pruned to 64, and then slowly grow again back to 128Mbyte. If this patch was applied it woul instead be pruned every minute to try to hit the target of 64mbyte cache in use.
Comment 6 Arunprasad 2013-03-07 05:55:42 PST
> True, but imagine you have a large memory cache setup the way we do WK2. 128 Mbyte total with a maxDeadbytes of 64MByte. Every time the cache reached 128, it will be pruned to 64, and then slowly grow again back to 128Mbyte. If this patch was applied it woul instead be pruned every minute to try to hit the target of 64mbyte cache in use.

Yeah!. So by default it is 1 minute, if the library user wants then they can override using _q_deadDecodedDataDeletionInterval.
Comment 7 Jocelyn Turcotte 2013-04-22 03:34:10 PDT
Comment on attachment 191502 [details]
Patch

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

> Source/WebKit/qt/WebCoreSupport/InitWebCoreQt.cpp:108
> +        WebCore::memoryCache()->setDeadDecodedDataDeletionInterval(60);

If you can think of any way to avoid hard-coding this value in two different files it would be nice.
I can't think of any simple way so r=me in any case.
Comment 8 Allan Sandfeld Jensen 2013-04-22 03:48:38 PDT
(In reply to comment #7)
> (From update of attachment 191502 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191502&action=review
> 
> > Source/WebKit/qt/WebCoreSupport/InitWebCoreQt.cpp:108
> > +        WebCore::memoryCache()->setDeadDecodedDataDeletionInterval(60);
> 
> If you can think of any way to avoid hard-coding this value in two different files it would be nice.
> I can't think of any simple way so r=me in any case.

Ideally we would expose an API to set the cacheModel, just like WK2 and the other ports does.
Comment 9 Allan Sandfeld Jensen 2013-04-23 01:32:19 PDT
Committed r148951: <http://trac.webkit.org/changeset/148951>