WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
61222
Improve handling in WebCore of low memory situations
https://bugs.webkit.org/show_bug.cgi?id=61222
Summary
Improve handling in WebCore of low memory situations
Michael Saboff
Reported
2011-05-20 16:11:08 PDT
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.
Attachments
Patch implementing memory pressure framework and handling for Mac
(26.15 KB, patch)
2011-05-20 16:42 PDT
,
Michael Saboff
mrowe
: review-
Details
Formatted Diff
Diff
Updated patch
(26.70 KB, patch)
2011-05-24 16:10 PDT
,
Michael Saboff
ggaren
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Michael Saboff
Comment 1
2011-05-20 16:42:48 PDT
Created
attachment 94287
[details]
Patch implementing memory pressure framework and handling for Mac
Simon Fraser (smfr)
Comment 2
2011-05-20 16:53:45 PDT
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 3
2011-05-20 17:16:02 PDT
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 4
2011-05-20 17:47:40 PDT
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
Comment 5
2011-05-24 16:10:39 PDT
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
Comment 6
2011-05-24 16:12:12 PDT
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 7
2011-05-24 16:31:02 PDT
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 8
2011-05-24 16:31:22 PDT
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
Comment 9
2011-05-24 16:44:00 PDT
Committed
r87228
: <
http://trac.webkit.org/changeset/87228
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug