Bug 61222 – Improve handling in WebCore of low memory situations

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Michael Saboff]

WebCore has several caches and generally cleans up those caches and GC's as needed.  During a system low memory situation, it makes sense to be more aggressive in returning such memory to the system.

[Michael Saboff]

Created attachment 94287 [details]
Patch implementing memory pressure framework and handling for Mac

[Simon Fraser (smfr)]

Comment on attachment 94287 [details]
Patch implementing memory pressure framework and handling for Mac

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

> Source/WebCore/platform/MemoryPressure.h:33
> +class MemoryPressure {

I don't really like the name of this class; it doesn't say what it does. MemoryPressureResponder?

> Source/WebCore/platform/mac/MemoryPressureMac.mm:88
> +    // FIXME: releaseFastMallocFreeMemory() seems to deadlock or buzy the TCMalloc 

buzy?

[Geoffrey Garen]

Comment on attachment 94287 [details]
Patch implementing memory pressure framework and handling for Mac

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?

[Mark Rowe (bdash)]

Comment on attachment 94287 [details]
Patch implementing memory pressure framework and handling for Mac

Marking as r- as Geoff is right, the libdispatch header you’re using isn’t a public header so can’t be used from WebCore as it will break builds.

[Michael Saboff]

Created attachment 94708 [details]
Updated patch

Addressed the concerns in the prior review.  Made suggested name changes. Added new files to WebCore.vcproj.

Provided private definitions of VM dispatch #defines.

Kept empty MemoryPressureHandler constructor, seems to be needed to compile for singleton pattern.  Probably due to being private.

Using dispatch_async as I lifted example code from another source.

[WebKit Review Bot]

Attachment 94708 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/MemoryPressureHandler.cpp:39:  Missing space inside { }.  [whitespace/braces] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 94708 [details]
Updated patch

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

> Source/WebCore/loader/cache/MemoryCache.h:197
> +    void PruneDeadResourcesToPercentage(float prunePercentage);
> +    void PruneLiveResourcesToPercentage(float prunePercentage);

Percentage of what?

[Geoffrey Garen]

Comment on attachment 94708 [details]
Updated patch

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

r=me

> Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:89
> +    pageCache()->setCapacity(pageCache()->pageCount()>>1);

The compiler will do this for you, so there's no need to be clever.

> Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:95
> +    [nsurlCache setMemoryCapacity:[nsurlCache currentMemoryUsage]>>1];

The compiler will do this for you, so there's no need to be clever.

[Michael Saboff]

Committed r87228: <http://trac.webkit.org/changeset/87228>