Bug 75991 – make the code in MemoryPressureHandler::respondToMemoryPressure shareable.

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Yongjun Zhang]

It would be nice that the implementation inside MemoryPressureHander::respondToMemoryPressure are shareable between mac and iOS.

[Yongjun Zhang]

Created attachment 121913 [details]
Move memory pressure handling code inside a new function (releaseMemory) so that we could shared it between mac and iOS.

[Yongjun Zhang]

<rdar://problem/10670958>

[Benjamin Poulain]

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.

[Yongjun Zhang]

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

[Yongjun Zhang]

Created attachment 122275 [details]
Address review comments on previous patch.

Address Benjamin's review comments, adding || PLATFORM(IOS) to MemoryPressureHander.cpp.

[Benjamin Poulain]

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.

[WebKit Review Bot]

Comment on attachment 122275 [details]
Address review comments on previous patch.

Clearing flags on attachment: 122275

Committed r104873: <http://trac.webkit.org/changeset/104873>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.