Bug 48684 – Should make use of purge priorities for different resource types

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Pratik Solanki]

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

[Pratik Solanki]

<rdar://problem/8611802>

[Pratik Solanki]

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.

[Darin Adler]

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

[Pratik Solanki]

Ok. I can do that.

[Pratik Solanki]

Created attachment 72537 [details]
Patch.v2

[Pratik Solanki]

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.

[Pratik Solanki]

Created attachment 72573 [details]
Patch.v3

[Early Warning System Bot]

Attachment 72573 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4902002

[Pratik Solanki]

Created attachment 72581 [details]
Patch.v4

[Pratik Solanki]

Fixed the Qt build error. Thank you, Qt EWS bot!

[Darin Adler]

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.

[Pratik Solanki]

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

[Pratik Solanki]

Created attachment 72879 [details]
Patch.v5

[Pratik Solanki]

Updated patch attached based on feedback. Changed function name to purgePriority() and it's now private.

[WebKit Commit Bot]

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 [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 72879 [details]
Patch.v5

Clearing flags on attachment: 72879

Committed r71318: <http://trac.webkit.org/changeset/71318>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.