Bug 75991 - make the code in MemoryPressureHandler::respondToMemoryPressure shareable.
Summary: make the code in MemoryPressureHandler::respondToMemoryPressure shareable.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-10 13:52 PST by Yongjun Zhang
Modified: 2012-01-12 16:00 PST (History)
7 users (show)

See Also:


Attachments
Move memory pressure handling code inside a new function (releaseMemory) so that we could shared it between mac and iOS. (4.68 KB, patch)
2012-01-10 14:31 PST, Yongjun Zhang
benjamin: review+
benjamin: commit-queue-
Details | Formatted Diff | Diff
Address review comments on previous patch. (5.29 KB, patch)
2012-01-12 11:10 PST, Yongjun Zhang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yongjun Zhang 2012-01-10 13:52:33 PST
It would be nice that the implementation inside MemoryPressureHander::respondToMemoryPressure are shareable between mac and iOS.
Comment 1 Yongjun Zhang 2012-01-10 14:31:44 PST
Created attachment 121913 [details]
Move memory pressure handling code inside a new function (releaseMemory) so that we could shared it between mac and iOS.
Comment 2 Yongjun Zhang 2012-01-10 14:34:49 PST
<rdar://problem/10670958>
Comment 3 Benjamin Poulain 2012-01-11 19:08:31 PST
Comment on attachment 121913 [details]
Move memory pressure handling code inside a new function (releaseMemory) so that we could shared it between mac and iOS.

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

> Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:118
> +    releaseMemory(false);

This boolean should ideally be an enum:
releaseMemory(Normal)
releaseMemory(Critical)

> Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:123
> +void MemoryPressureHandler::releaseMemory(bool critical)
> +{

What do you think about moving this to a separate common file? As it is, 90% of this file is blacklisted for iOS. Separate files are easier to maintain that #ifdef imho (although the file would be very simple in this case).

If you choose the #ifdef, do not forget to add || PLATFORM(IOS) to MemoryPressureHandler.cpp.

> Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:125
> -    pageCache()->setCapacity(pageCache()->pageCount()/2);
> +    pageCache()->setCapacity(critical ? 0 : pageCache()->pageCount()/2);

Spaces around operator.

> Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:131
> +    [nsurlCache setMemoryCapacity:critical ? 0 : [nsurlCache currentMemoryUsage]/2];

Spaces around operator.
Comment 4 Yongjun Zhang 2012-01-12 10:33:19 PST
(In reply to comment #3)
> (From update of attachment 121913 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121913&action=review
> 
> > Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:118
> > +    releaseMemory(false);
> 
> This boolean should ideally be an enum:
> releaseMemory(Normal)
> releaseMemory(Critical)
> 

Agree.  Currently we only have two levels, we can consider changing this to enum when new levels are introduced.

> > Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:123
> > +void MemoryPressureHandler::releaseMemory(bool critical)
> > +{
> 
> What do you think about moving this to a separate common file? As it is, 90% of this file is blacklisted for iOS. Separate files are easier to maintain that #ifdef imho (although the file would be very simple in this case).
>

I thought about that, but didn't like the idea of having a separate file with just this one function.
 
> If you choose the #ifdef, do not forget to add || PLATFORM(IOS) to MemoryPressureHandler.cpp.

Ok, looks like we already had that for iOS.
> 
> > Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:125
> > -    pageCache()->setCapacity(pageCache()->pageCount()/2);
> > +    pageCache()->setCapacity(critical ? 0 : pageCache()->pageCount()/2);
> 
> Spaces around operator.
> 
> > Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:131
> > +    [nsurlCache setMemoryCapacity:critical ? 0 : [nsurlCache currentMemoryUsage]/2];
> 
> Spaces around operator.
Comment 5 Yongjun Zhang 2012-01-12 11:10:36 PST
Created attachment 122275 [details]
Address review comments on previous patch.

Address Benjamin's review comments, adding || PLATFORM(IOS) to MemoryPressureHander.cpp.
Comment 6 Benjamin Poulain 2012-01-12 13:44:38 PST
Comment on attachment 122275 [details]
Address review comments on previous patch.

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

> Source/WebCore/platform/MemoryPressureHandler.h:48
> +    void releaseMemory(bool critical);

I still feel the enum is better, even for only two values here.
Comment 7 WebKit Review Bot 2012-01-12 16:00:08 PST
Comment on attachment 122275 [details]
Address review comments on previous patch.

Clearing flags on attachment: 122275

Committed r104873: <http://trac.webkit.org/changeset/104873>
Comment 8 WebKit Review Bot 2012-01-12 16:00:13 PST
All reviewed patches have been landed.  Closing bug.