Bug 48684 - Should make use of purge priorities for different resource types
Summary: Should make use of purge priorities for different resource types
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-10-29 14:55 PDT by Pratik Solanki
Modified: 2010-11-04 03:11 PDT (History)
8 users (show)

See Also:


Attachments
Patch (12.02 KB, patch)
2010-10-29 17:44 PDT, Pratik Solanki
no flags Details | Formatted Diff | Diff
Patch.v2 (15.00 KB, patch)
2010-11-01 12:11 PDT, Pratik Solanki
no flags Details | Formatted Diff | Diff
Patch.v3 (13.95 KB, patch)
2010-11-01 15:17 PDT, Pratik Solanki
no flags Details | Formatted Diff | Diff
Patch.v4 (14.44 KB, patch)
2010-11-01 16:01 PDT, Pratik Solanki
no flags Details | Formatted Diff | Diff
Patch.v5 (14.51 KB, patch)
2010-11-03 15:45 PDT, Pratik Solanki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pratik Solanki 2010-10-29 14:55:40 PDT
PurgeableBuffer has support for purge priorities. We use this priority to set the volatile group when we call vm_purgable_control for the memory. Currently, all memory is marked as PurgeMiddle which get passed to the kernel as VM_VOLATILE_GROUP_4. Since some resource types (e.g. javascript) are more important than others, we should change this behavior to have different priority levels, e.g.

Default CachedResource priority - PurgeMiddle (which is the same as PurgeDefault)
CachedImage - PurgeFirst
CachedCSSStyleSheet - PurgeLast
CachedScript - PurgeLast
Comment 1 Pratik Solanki 2010-10-29 14:56:31 PDT
<rdar://problem/8611802>
Comment 2 Pratik Solanki 2010-10-29 17:44:26 PDT
Created attachment 72421 [details]
Patch

In order to use PurgeableBuffer::PurgePriority in CachedResource.h, I had to make the PurgeableBuffer.h private (otherwise WebKit fails to compile). I considered moving the PurgePriority enum to CachedResource but thought that it was more appropriate to leave it in PurgeableBuffer.
Comment 3 Darin Adler 2010-10-30 07:21:15 PDT
(In reply to comment #2)
> In order to use PurgeableBuffer::PurgePriority in CachedResource.h, I had to make the PurgeableBuffer.h private (otherwise WebKit fails to compile). I considered moving the PurgePriority enum to CachedResource but thought that it was more appropriate to leave it in PurgeableBuffer.

I think that the purge priority enum should move to its own header. We don’t want everyone who includes CachedResource.h to have to include all of PurgeableBuffer.h just because of a single enum that’s needed.

If you did that, the new header would be private and PurgeableBuffer.h would remain project.
Comment 4 Pratik Solanki 2010-10-30 08:46:26 PDT
Ok. I can do that.
Comment 5 Pratik Solanki 2010-11-01 12:11:29 PDT
Created attachment 72537 [details]
Patch.v2
Comment 6 Pratik Solanki 2010-11-01 13:52:42 PDT
Comment on attachment 72537 [details]
Patch.v2

Actually, we might be able to fix this bug by just changing the implementation of setPurgePriority() a bit and using that. Let me look at this a bit more. Removing this from the review queue for now while I try an alternate implementation.
Comment 7 Pratik Solanki 2010-11-01 15:17:12 PDT
Created attachment 72573 [details]
Patch.v3
Comment 8 Early Warning System Bot 2010-11-01 15:58:17 PDT
Attachment 72573 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4902002
Comment 9 Pratik Solanki 2010-11-01 16:01:47 PDT
Created attachment 72581 [details]
Patch.v4
Comment 10 Pratik Solanki 2010-11-01 16:03:23 PDT
Fixed the Qt build error. Thank you, Qt EWS bot!
Comment 11 Darin Adler 2010-11-03 11:54:01 PDT
Comment on attachment 72581 [details]
Patch.v4

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

Looks fine to me, but I’m not setting commit-queue+ at this time because you may want to make some of the changes I suggest here.

> WebCore/loader/CachedCSSStyleSheet.h:63
>      protected:
> +        virtual PurgePriority resourcePurgePriority() const { return PurgeLast; }
> +
>          RefPtr<TextResourceDecoder> m_decoder;
>          String m_decodedSheetText;

I think these members should be private, not protected, unless there is a derived class that needs to access these.

> WebCore/loader/CachedImage.h:91
> +protected:
> +    virtual PurgePriority resourcePurgePriority() const { return PurgeFirst; }

This should be private, not protected, unless derived classes have to call it for some reason.

> WebCore/loader/CachedResource.h:212
> +    virtual PurgePriority resourcePurgePriority() const { return PurgeDefault; }

Given that this class is already named CachedResource, it seems this message should be called purgePriority, not resourcePurgePriority. The prefix doesn’t add anything.

This function should be private, not protected. There is no reason for a derived class to call it.

> WebCore/loader/CachedScript.h:56
> +    protected:
> +        virtual PurgePriority resourcePurgePriority() const { return PurgeLast; }

This should be private, not protected.
Comment 12 Pratik Solanki 2010-11-03 13:54:42 PDT
(In reply to comment #11)
> (From update of attachment 72581 [details])
> > WebCore/loader/CachedCSSStyleSheet.h:63
> >      protected:
> > +        virtual PurgePriority resourcePurgePriority() const { return PurgeLast; }
> > +
> >          RefPtr<TextResourceDecoder> m_decoder;
> >          String m_decodedSheetText;
> 
> I think these members should be private, not protected, unless there is a derived class that needs to access these.

Interesting. I didn't know private functions could be overridden by subclasses and that it would work as expected. Yes, making it private would be much better. New updated patch coming up.
Comment 13 Pratik Solanki 2010-11-03 15:45:18 PDT
Created attachment 72879 [details]
Patch.v5
Comment 14 Pratik Solanki 2010-11-03 15:46:41 PDT
Updated patch attached based on feedback. Changed function name to purgePriority() and it's now private.
Comment 15 WebKit Commit Bot 2010-11-04 03:08:25 PDT
The commit-queue encountered the following flaky tests while processing attachment 72879 [details]:

animations/dynamic-stylesheet-loading.html

Please file bugs against the tests.  These tests were authored by cmarrin@apple.com.  The commit-queue is continuing to process your patch.
Comment 16 WebKit Commit Bot 2010-11-04 03:11:03 PDT
Comment on attachment 72879 [details]
Patch.v5

Clearing flags on attachment: 72879

Committed r71318: <http://trac.webkit.org/changeset/71318>
Comment 17 WebKit Commit Bot 2010-11-04 03:11:10 PDT
All reviewed patches have been landed.  Closing bug.