[webkit-reviews] review granted: [Bug 44806] Use purgeable memory to keep more dead resources in cache : [Attachment 65814] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Pratik Solanki <psolanki at apple.com>'s
request for review:
Bug 44806: Use purgeable memory to keep more dead resources in cache
https://bugs.webkit.org/show_bug.cgi?id=44806

Attachment 65814: Patch
https://bugs.webkit.org/attachment.cgi?id=65814&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Thanks, great to see you taking on this work!

> +    bool wasPurgeable = Cache::markMemoryPurgeableOnEvict() && resource &&
resource->isPurgeable();

A better name for this function would be shouldMarkMemoryPurgeableOnEviction.
The phrase "mark memory purgeable on evict" is not as good, because it sounds
like a function that will take action, not return a boolean, and "on evict" is
unnecessarily ungrammatical and hence slightly confusing.

> +	   // Add the size back since we had subtracted it when we marked the
memory
> +	   // as purgeable.

I suggest putting this comment all in one line. The line wouldn't be that long.


> +	   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());

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

> +		       // Pass false if we had already subtracted the resource
size
> +		       // when we marked it as purgeable.

This comment would be better if it explained that passing false means. It's
tantalizingly close to a comment that explains things, but I have to look up
the contents of the evict function to understand what this mean. Also, would
read better as a single line instead of broken up like this. It wouldn't be too
long.

> +		       // evict the resource since we couldn't use purgeable
> +		       // memory.

Should capitalize the word "evict" and put this comment all on one line.

> +	   if (!resource->makePurgeable(true))
> +	       return false;

Not your fault, but call sites like this are the reason we don’t like

> +	   // 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?

> +	   return true;
> +    }
> +    return false;

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

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

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.

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

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

I believe the word is spelled "likelihood", not "likelyhood".

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.


> +#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:

    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.

> +    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?

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