Bug 22214 - Keep dead resources in memory cache in purgeable memory.
Summary: Keep dead resources in memory cache in purgeable memory.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-12 14:57 PST by Antti Koivisto
Modified: 2008-11-24 19:29 PST (History)
5 users (show)

See Also:


Attachments
first attempt (31.60 KB, patch)
2008-11-12 20:39 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
some renaming and cleanups (31.23 KB, patch)
2008-11-12 21:51 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
some more cleanups (32.04 KB, patch)
2008-11-13 11:57 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
updated patch (36.77 KB, patch)
2008-11-20 20:06 PST, Antti Koivisto
ggaren: review+
Details | Formatted Diff | Diff
patch that applies cleanly and builds with current tot (31.79 KB, patch)
2008-11-21 18:10 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
this time with the build file changes too (36.00 KB, patch)
2008-11-21 19:13 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
updated with Geoff's comments plus some other fixes (39.96 KB, patch)
2008-11-23 21:24 PST, Antti Koivisto
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2008-11-12 14:57:33 PST
OS X 10.5 has kernel facility for purgable memory that allows marking memory area as less important. Under memory pressure system can steal pages that have been marked purgable for better uses. We should use this mechanism for dead resources in WebCore memory cache.
Comment 1 Geoffrey Garen 2008-11-12 16:06:08 PST
What buffers, specifically, can we mark as purgable?
Comment 2 Antti Koivisto 2008-11-12 16:19:40 PST
(In reply to comment #1)
> What buffers, specifically, can we mark as purgable?

Dead encoded data, though it is bit more complicated than just marking any existing buffer.
Comment 3 Antti Koivisto 2008-11-12 20:39:13 PST
Created attachment 25114 [details]
first attempt 

This patch uses purgable memory for dead encoded image data greter than 16kb in size.
Comment 4 mitz 2008-11-12 20:50:24 PST
Comment on attachment 25114 [details]
first attempt 

> +    static PassRefPtr<SharedBuffer> createWithVolatileBufferTakingOwnership(VolatileBuffer* buffer) { return adoptRef(new SharedBuffer(buffer)); }

I think the usual term for "taking ownership" in WebCore is "adopting". Maybe this can be called createByAdoptingVolatileBuffer().

> +        // call makeNonVolatile() and check the return value before accessing data

I think you renamed it to tryMakeNonVolatile()

> +#if !PLATFORM(DARWIN) || defined(BUILDING_ON_TIGER)
> +    inline VolatileBuffer* VolatileBuffer::create(const char*, unsigned) { return 0; }
> +    inline VolatileBuffer::~VolatileBuffer() { }
> +    inline const char* VolatileBuffer::data() const { return 0; }
> +    inline void VolatileBuffer::setVolatileCategory(Category) { }
> +    inline bool VolatileBuffer::testPurged() { return false; }
> +    inline bool VolatileBuffer::makeVolatile() { return false; }
> +    inline bool VolatileBuffer::makeNonVolatile() { return false; }
> +#endif

Same above.

> + static const unsigned minVolatileBufferSize = 4096; // one page

Is there a Mach constant or function that returns the page size?
Comment 5 Antti Koivisto 2008-11-12 21:51:41 PST
Created attachment 25115 [details]
some renaming and cleanups

Some renaming as suggested by bdash and mitz

createWithVolatileBufferTakingOwnership -> adoptVolatileBuffer
VolatileBuffer::Category -> VolatileBuffer::PurgePriority
takeVolatileBuffer -> releaseVolatileBuffer
Comment 6 Antti Koivisto 2008-11-13 11:57:44 PST
Created attachment 25129 [details]
some more cleanups

-rename testPurged -> wasPurged
- use size_t
- make VolatileBuffer state private
Comment 7 Antti Koivisto 2008-11-20 20:06:50 PST
Created attachment 25331 [details]
updated patch

- fix a bug where resource would fail to load first time after it had been purged
- some cleanups in Cache.cpp
- better statistics dumps
- make JS and CSS resources purgable too
Comment 8 Geoffrey Garen 2008-11-21 13:44:32 PST
Comment on attachment 25331 [details]
updated patch

+    if (m_volatileBuffer) {
+        ASSERT(!m_volatileBuffer->isVolatile());
+        return m_volatileBuffer->data();
+    }

The ASSERT here is redundant with an ASSERT inside the VolatileBuffer class. You should probably remove it.
Comment 9 Geoffrey Garen 2008-11-21 16:48:20 PST
Comment on attachment 25331 [details]
updated patch

It's possible that I'm missing something, but I think it's a problem that both CachedResource and SharedBuffer keep a pointer to volatile data. This distributes responsibility for some tricky ownership between the SharedBuffer and its client. I'd prefer for SharedBuffer to have complete ownership of, and responsibility for, a volatile backing store. Then, the client can just make calls like "tryMakeVolatile" without worrying about lower-level details.

I think "wasPurged" is probably a better name than "wasVolatileDataPurged". I'd like to keep the idea of "volatile data" an implementation detail if possible.

The word "purgable" might be better than the word "volatile", since "purgable" describes the advantage -- why you use this feature -- while "volatile" just sounds scary :).

+bool CachedResource::tryMakeNonVolatile() 
+{ 
+    if (m_data || !m_volatileData)
+        return true;

This test confused me, since m_data and m_volatileData are supposed to be mutually exclusive. Like we discussed on IRC, how about: if (!m_volatileData) { return true; } ASSERT(!m_data);

Or, if SharedBuffer had complete control over the volatile data:

if (!m_data)
    return true;

if (m_data->isPurgable())
    return true;

return m_data->makePurgable();

+    if (m_volatileData->tryMakeNonVolatile())
+        m_data = SharedBuffer::adoptVolatileBuffer(m_volatileData.release());
+    else 
+        m_volatileData.clear();
+    
+    return m_data; 

I think this would be simpler (and more MSVC-friendly) like this:

if (m_volatileData->tryMakeNonVolatile()) {
    m_data = SharedBuffer::adoptVolatileBuffer(m_volatileData.release());
    return true;
}

m_volatileData.clear();
return false;

+bool CachedResource::tryMakeVolatile() 

Looks like the bool result here is unused. Let's return void instead, to avoid bitrot.

+    const size_t volatileThreshold = 4 * 4096;

Since this is cross-platform code, it might be worth commenting that 4096 is the page size on Mac OS X.

+SharedBuffer::~SharedBuffer()
+{
+}

Can this be removed?

+        if(doc && !isPreload)

Missing space after "if".

+bool CachedResource::tryMakeVolatile() 
+{

Would be helpful to ASSERT(!hasClients()) at the top of this function.

I'm going to say "r+" because I don't see any serious bugs, but I'd really like you to consider removing the volatile data pointer from CachedResource.
Comment 10 Antti Koivisto 2008-11-21 18:10:15 PST
Created attachment 25369 [details]
patch that applies cleanly and builds with current tot

for testing, does not contain changes suggested by geoff
Comment 11 Antti Koivisto 2008-11-21 19:13:31 PST
Created attachment 25371 [details]
this time with the build file changes too
Comment 12 Stephanie Lewis 2008-11-21 22:14:54 PST
membuster and the PLT look good, although it's debatable how useful of a test those are in this case.
Comment 13 Antti Koivisto 2008-11-23 21:24:56 PST
Created attachment 25410 [details]
updated with Geoff's comments plus some other fixes

- Implemented naming changes as discussed with Geoff in IRC. VolatileBuffer is renamed to PurgableBuffer and word purgable generally replaces word volatile in the API.
- Kept the buffer ownership scheme (as discussed with Geoff), clarified the API with some comments and by removing purgableBuffer() accessor from SharedBuffer.
- Changed CachedImage to delete m_image (if it is safe) rather than just deleting its decoded data when turning buffer volatile. The Image object would still hold a reference of the old non-purgable data buffer.
- Changed CachedScript to turn the buffer purgable asynchronously instead of synchronously in allClientsRemoved(). This is safer.
- Added more asserts to guarantee consistent state.

> +bool CachedResource::tryMakeVolatile() 
> Looks like the bool result here is unused. Let's return void instead, to avoid
> bitrot.

I kept the return values as a way of documenting which exits will actually results in volatile buffer. I think it is helpful even though current call sited don't check the return value.

>+    const size_t volatileThreshold = 4 * 4096;
>
>Since this is cross-platform code, it might be worth commenting that 4096 is
>the page size on Mac OS X.

Added a comment. It is really more like the "page size of the CPUs most likely to run WebKit", not just Mac.

>+SharedBuffer::~SharedBuffer()
>+{
>+}
>Can this be removed?

It was needed so I can forward declare PugrableBuffer in SharedBuffer header. SharedBuffer is included by half of the WebCore.

>+bool CachedResource::tryMakeVolatile() 
>+{
>Would be helpful to ASSERT(!hasClients()) at the top of this function.

Done.

Requesting re-review since there were some non-trivial changes.
Comment 14 Geoffrey Garen 2008-11-24 17:14:22 PST
Comment on attachment 25410 [details]
updated with Geoff's comments plus some other fixes

r=me
Comment 15 Antti Koivisto 2008-11-24 19:18:37 PST
Sending        WebCore/ChangeLog
Sending        WebCore/WebCore.base.exp
Sending        WebCore/WebCore.vcproj/WebCore.vcproj
Sending        WebCore/WebCore.xcodeproj/project.pbxproj
Sending        WebCore/loader/Cache.cpp
Sending        WebCore/loader/Cache.h
Sending        WebCore/loader/CachedCSSStyleSheet.cpp
Sending        WebCore/loader/CachedCSSStyleSheet.h
Sending        WebCore/loader/CachedImage.cpp
Sending        WebCore/loader/CachedResource.cpp
Sending        WebCore/loader/CachedResource.h
Sending        WebCore/loader/CachedScript.cpp
Adding         WebCore/platform/PurgeableBuffer.h
Sending        WebCore/platform/SharedBuffer.cpp
Sending        WebCore/platform/SharedBuffer.h
Sending        WebCore/platform/cf/SharedBufferCF.cpp
Adding         WebCore/platform/mac/PurgeableBufferMac.cpp
Transmitting file data .................
Committed revision 38741.
Comment 16 Antti Koivisto 2008-11-24 19:29:27 PST
Spelling was changed to "purgeable" since http://kxr.bitcanvas.net/kxr/source/osfmk/vm/vm_purgeable_internal.h?v=xnu-1228 says:

026  * It is believed that the correct spelling is 
027  * { 'p', 'u', 'r', 'g', 'e', 'a', 'b', 'l', 'e' }.
028  * However, there is one published API that likes to spell it without the
029  * first 'e', vm_purgable_control().