[webkit-reviews] review granted: [Bug 61222] Improve handling in WebCore of low memory situations : [Attachment 94287] Patch implementing memory pressure framework and handling for Mac

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 61222: Improve handling in WebCore of low memory situations
https://bugs.webkit.org/show_bug.cgi?id=61222

Attachment 94287: Patch implementing memory pressure framework and handling for
Mac
https://bugs.webkit.org/attachment.cgi?id=94287&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=94287&action=review

r=me, but:
- I think you should make the changes specified here;
- Please don't commit this if <dispatch/source_private.h> is not a publicly
available header.

> Source/WebCore/loader/cache/MemoryCache.h:197
> +    void pruneDeadResources();
> +    void pruneLiveResources();
> +    void forcePruneDeadResources(float prunePercentage);
> +    void forcePruneLiveResources(float prunePercentage);
> +    void pruneDeadResourcesToTarget(unsigned targetSize); // Flush decoded
and encoded data from resources not referenced by Web pages.
> +    void pruneLiveResourcesToTarget(unsigned targetSize); // Flush decoded
data from resources still referenced by Web pages.

"forcePrune" is kind of a funny name, since it doesn't actually force -- there
are conditions under which it returns early. The real difference between these
functions is not between force and not, but between percentages as units and
bytes as units. I'd recommend:

pruneDeadResources(); // Automatically decides how much to prune
pruneDeadResourcesToSize(size_t)
pruneDeadResourcesToPercentage(float)
// Same for LiveResources

> Source/WebCore/platform/MemoryPressure.cpp:33
> +    static MemoryPressure* staticMemoryPressure = new MemoryPressure;

Please use DEFINE_STATIC_LOCAL, as is our custom.

> Source/WebCore/platform/MemoryPressure.cpp:39
> +MemoryPressure::MemoryPressure()
> +{
> +}

You can remove this -- the compiler will generate it automatically.

> Source/WebCore/platform/MemoryPressure.h:37
> +    void installMemoryPressureHandler();

I'd suggest "MemoryPressureHandler" as the name of the class, and "install" as
the name of this function.

Then you have "memoryPressureHandler()->install()", which tells a noun to
perform a verb, instead of "memoryPressure()->installMemoryPressureHandler()",
which tells an abstract concept ("memory pressure") to perform an action with
some unspecified other noun ("memory pressure handler").

> Source/WebCore/platform/mac/MemoryPressureMac.mm:35
> +#if !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)
> +#import <dispatch/source_private.h>
> +#endif

Is this available as a public header? If not, your patch will break OpenSource
builds.

> Source/WebCore/platform/mac/MemoryPressureMac.mm:47
> +	   dispatch_async(dispatch_get_main_queue(), ^{

Why does installation of the handler need to be asynchronous?