[webkit-reviews] review granted: [Bug 22214] Keep dead resources in memory cache in purgable memory. : [Attachment 25331] updated patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Antti Koivisto
<koivisto at iki.fi>'s request for review:
Bug 22214: Keep dead resources in memory cache in purgable memory.
https://bugs.webkit.org/show_bug.cgi?id=22214

Attachment 25331: updated patch
https://bugs.webkit.org/attachment.cgi?id=25331&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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.