[Webkit-unassigned] [Bug 44806] Use purgeable memory to keep more dead resources in cache

Sun Aug 29 23:46:03 PDT 2010

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

--- Comment #4 from Pratik Solanki <psolanki at apple.com>  2010-08-29 23:46:03 PST ---
(In reply to comment #3)
> (From update of attachment 65814 [details])

> > +        int delta = resource->size();
> > +        if (delta)
> > +            adjustSize(resource->hasClients(), delta);
> 
> Do we really need the optimization for a resource with a size of zero? Is that a common case that needs to be faster than other cases? I would write:
> 
>     adjustSize(resource->hasClients(), resource->size());

That part of the code was copied from existing code. It seems to be the pattern in Cache.cpp to test for a non-zero delta before calling adjustSize). Looking at the code though, I don't see why we need to test for 0. I could change it if you would prefer.

> By the way, if we did need it, I still think that scoping the local variable to the if would be nicer style.

Sure I can do that.

> > +        if (!resource->makePurgeable(true))
> > +            return false;
> 
> Not your fault, but call sites like this are the reason we don’t like

I think you forgot to finish this sentence.

> > +        // Subtract from our size totals.
> > +        int delta = -static_cast<int>(resource->size());
> > +        if (delta)
> > +            adjustSize(resource->hasClients(), delta);
> 
> Is the static_cast here really needed? Why exactly?
> 
> I don't think the comment says anything the code doesn't already say, so you should omit it.
> 
> Same comment as earlier about optimizing the zero size case. Why do we need to optimize that? Is it particularly common?

This part of the code was copied from evict(). It does the static_cast as well. resource->size() returns an unsigned int so maybe the cast was added for that reason?

> > +        return true;
> > +    }
> > +    return false;
> 
> I think an early return would be better style than nesting all the code inside the resource->inCache check.

Hazards of copy-paste code... But you're right. I'll fix that.

> > +        if (decrementSize) {
> > +            // Subtract from our size totals.
> > +            int delta = -static_cast<int>(resource->size());
> > +            if (delta)
> > +                adjustSize(resource->hasClients(), delta);
> > +        }
> 
> Same comments as above again.
> 
> > +#if ENABLE(PURGEABLE_MEMORY_MARK_ON_EVICTION)
> > +/* With this define enabled, we no longer have an explicit bound on the number
> > + * of dead resources. The normal behaviour is that dead resources have their
> > + * memory marked as purgeable and as we pruned the dead resources list, we
> > + * evicted them from cache and deleted them. We change this behaviour to the
> > + * following
> > + *
> > + * 1. Dead resources in the cache are kept in non-purgeable memory.
> > + * 2. When we prune dead resources, instead of freeing them, we mark their
> > + *    memory as purgeable and keep the resources until the kernel reclaims the
> > + *    purgeable memory.
> > + *
> > + * By leaving the in-cache dead resources in dirty resident memory, we decrease
> > + * the likelyhood of the kernel claiming that memory and forcing us to refetch
> > + * the resource (for example when a user presses back).
> > + *
> > + * And by having an unbounded number of resource objects using purgeable
> > + * memory, we can utilize as much memory as is available on the machine. We are
> > + * bounded by how much purgeable memory the kernel will alow us to have. We do
> > + * delete CachedResource objects - it happens as part of pruneDeadResources if
> > + * we notice that the memory used by the resource has been purged by the
> > + * kernel. The trade-off here is that the CachedResource object (and its member
> > + * variables) itself is allocated in non-purgeable TC-malloc'd memory so we
> > + * would see slightly more memory use due to this.
> > + */
> > +#endif
> 
> We use // for comments, not /* */. If your reason to use /* */ is because it's a long comment and you want to make it easier to edit, that is defeated by putting a * on every line.

Ok. I can change it to //. /* */ is just a style I like using. I use vim so editing such comments is actually not a problem at all for me :). But I'll fix it. 

> It doesn't make a lot of sense to have a comment inside an #if, so you should find another way to tie it together.

It explains what happens when the macro is defined. Sort of like saying, this comment only valid when the #if is 1. Again, something I had seen in other projects and liked. I can just change the comment to say that it only applies when the ENABLE is 1.

> We use US spelling in comments in WebKit, so it should be "behavior" rather than "behaviour".

Sure. I can fix that.

> The word "utilize" is no better than the word "use", so please don't utilize it. ;-)

:)

> You should also revisit this comment and see if you can say the same thing with fewer words. It's a little too long for people to read, so not as valuable as a terser comment that says the same thing.

Good point. I'll try to strip it down to fewer words. 

> > +#if ENABLE(PURGEABLE_MEMORY_MARK_ON_EVICTION)
> > +    static bool markMemoryPurgeableOnEvict() { return true; }
> > +#else
> > +    static bool markMemoryPurgeableOnEvict() { return false; }
> > +#endif
> 
> It would be better to have the #if around the body of the function rather than the definition. I suggest moving the function outside the class definition. It can still be inline:

Ok.

>     inline bool Cache::shouldMarkMemoryPurgableOnEviction()
>     {
> #if /* SUITABLE CONDITION HERE, PERHAPS PLATFORM(IOS) */
>         return true;
> #else
>         return false;
> #endif
>     }
> 
> I'm not sure we should have a special #if for this, given that it's controlled in exactly one place. It seems that one place could have the suitable #if. But I suppose you can leave that for now. It's a real shame to have something in Platform.h that really only needs to be in Cache.h, though.

I'll try to rework this. Seems like it might be possible to remove this from Platform.h and just have it in Cache.h

> > +    void evict(CachedResource*, bool decrementSize = true);
> 
> It's quite unpleasant to have a boolean argument to this function that tells it to "decrement size". First of all, it should be "shouldDecrementSize", but also, that's too low level a concept for the function argument. It's just not clear why would you pass false in some cases and true in others? Can't evict make the decision itself based on looking at resource->wasPurged()? That would be cleaner. Or the boolean could be "size already removed from cache at purge time", which would be clearer and easier to set correctly for callers.
> 
> > -    if (isSafeToMakePurgeable())
> > -        makePurgeable(true);
> > +    if (!Cache::markMemoryPurgeableOnEvict()) {
> > +        if (isSafeToMakePurgeable())
> > +            makePurgeable(true);
> > +    }
> 
> I think && would be better here than nesting ifs, but also, can the check go lower level, inside isSafeToMakePurgeable and markPurgeable?

&& is easy to do. I'll have to investigate if the check can go lower into makePurgeable(). 

> > -        makePurgeable(true);
> > +        if (!Cache::markMemoryPurgeableOnEvict())
> > +            makePurgeable(true);
> 
> Same question.
> 
> > -    if (isSafeToMakePurgeable())
> > -        makePurgeable(true);
> > +    if (!Cache::markMemoryPurgeableOnEvict()) {
> > +        if (isSafeToMakePurgeable())
> > +            makePurgeable(true);
> > +    }
> 
> And again.
> 
> I'm sure at least some of my suggestions aren't practical, but at least some of them are good ideas. I'm going to say review+, but I think you may want another round of review after taking at least some of my advice.

I'll have a new patch up addressing the comments. Thanks a lot for the review!

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