Bug 22214 – Keep dead resources in memory cache in purgeable memory.

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Antti Koivisto]

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.

[Geoffrey Garen]

What buffers, specifically, can we mark as purgable?

[Antti Koivisto]

(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.

[Antti Koivisto]

Created attachment 25114 [details]
first attempt 

This patch uses purgable memory for dead encoded image data greter than 16kb in size.

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?

[Antti Koivisto]

Created attachment 25115 [details]
some renaming and cleanups

Some renaming as suggested by bdash and mitz

createWithVolatileBufferTakingOwnership -> adoptVolatileBuffer
VolatileBuffer::Category -> VolatileBuffer::PurgePriority
takeVolatileBuffer -> releaseVolatileBuffer

[Antti Koivisto]

Created attachment 25129 [details]
some more cleanups

-rename testPurged -> wasPurged
- use size_t
- make VolatileBuffer state private

[Antti Koivisto]

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

[Geoffrey Garen]

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.

[Geoffrey Garen]

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.

[Antti Koivisto]

Created attachment 25369 [details]
patch that applies cleanly and builds with current tot

for testing, does not contain changes suggested by geoff

[Antti Koivisto]

Created attachment 25371 [details]
this time with the build file changes too

[Stephanie Lewis]

membuster and the PLT look good, although it's debatable how useful of a test those are in this case.

[Antti Koivisto]

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.

[Geoffrey Garen]

Comment on attachment 25410 [details]
updated with Geoff's comments plus some other fixes

r=me

[Antti Koivisto]

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.

[Antti Koivisto]

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().