[Webkit-unassigned] [Bug 22214] Keep dead resources in memory cache in purgable memory.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=22214


koivisto at iki.fi changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #25331|0                           |1
        is obsolete|                            |
  Attachment #25371|0                           |1
        is obsolete|                            |
  Attachment #25410|                            |review?(ggaren at apple.com)
               Flag|                            |




------- Comment #13 from koivisto at iki.fi  2008-11-23 21:24 PDT -------
Created an attachment (id=25410)
 --> (https://bugs.webkit.org/attachment.cgi?id=25410&action=view)
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.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.