Bug 82674 – WebKit should throttle memory pressure notifications in proportion to handler time

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Michael Saboff]

Currently MemoryPressureHandler::respondToMemoryPressure() starts a "holdoff" timer at the beginning of it's processing for a fixed 5 seconds. After that timer fires, it can receive another memory pressure event.  In the case that the processing itself takes 5 or more seconds, the system can spend its time trying to free memory. The long duration might be due to paging or swapping and the processing itself may exacerbate the situation.

Instead the handler should start the time after trying to free memory and delay in proportion to the time it took for the handler to run.

<rdar://problem/11130218>

[Michael Saboff]

Created attachment 134696 [details]
Patch

[Geoffrey Garen]

Comment on attachment 134696 [details]
Patch

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

r=me

> Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:52
> +// this is 1/s_holdOffMultiplier percent of the time.

Space around the "/", please.

> Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:55
>  static const time_t s_secondsBetweenMemoryCleanup = 5;

This name isn't accurate anymore. It should be "s_minimumHoldOffTime".

> Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:124
> +    time(&startTime);

Slightly better to use WTF::currentTime(), since that's the custom in WebKit.

[Alexey Proskuryakov]

> Slightly better to use WTF::currentTime(), since that's the custom in WebKit.

I wonder why we don't use monotonicallyIncreasingTime almost everywhere. This case in particular looks like it shouldn't be affected by clock adjustments.

[Michael Saboff]

(In reply to comment #2)
> (From update of attachment 134696 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134696&action=review
> 
> r=me
> 
> > Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:52
> > +// this is 1/s_holdOffMultiplier percent of the time.
> 
> Space around the "/", please.
> 
> > Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:55
> >  static const time_t s_secondsBetweenMemoryCleanup = 5;
> 
> This name isn't accurate anymore. It should be "s_minimumHoldOffTime".
> 
> > Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:124
> > +    time(&startTime);
> 
> Slightly better to use WTF::currentTime(), since that's the custom in WebKit.

Made these changes except changed to using monotonicallyIncreasingTime() per Alexey's comment.

[Michael Saboff]

Committed r112910: <http://trac.webkit.org/changeset/112910>

[Darin Adler]

The final version of this patch could use a little cleanup.

- The local variables should be defined where they are initialized instead of at the top of the function.
- The cast to unsigned is OK if the code won’t compile without it, but it should be static_cast instead of a C-style cast.
- The cast to double is not needed.

Other things I would have done differently, but more “questionable”:

- I’d have used the max function instead of an if statement for s_minimumHoldOffTime.
- I’d have gone without a local variable for endTime.

[Michael Saboff]

Reopening to address Darin's comments.

[Michael Saboff]

Committed r112930: <http://trac.webkit.org/changeset/112930>