Bug 82674 - WebKit should throttle memory pressure notifications in proportion to handler time
Summary: WebKit should throttle memory pressure notifications in proportion to handler...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-03-29 16:14 PDT by Michael Saboff
Modified: 2012-04-02 13:35 PDT (History)
3 users (show)

See Also:


Attachments
Patch (3.49 KB, patch)
2012-03-29 17:05 PDT, Michael Saboff
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2012-03-29 16:14:54 PDT
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>
Comment 1 Michael Saboff 2012-03-29 17:05:41 PDT
Created attachment 134696 [details]
Patch
Comment 2 Geoffrey Garen 2012-03-29 20:00:16 PDT
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.
Comment 3 Alexey Proskuryakov 2012-04-01 01:03:00 PDT
> 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.
Comment 4 Michael Saboff 2012-04-02 11:07:29 PDT
(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.
Comment 5 Michael Saboff 2012-04-02 11:15:09 PDT
Committed r112910: <http://trac.webkit.org/changeset/112910>
Comment 6 Darin Adler 2012-04-02 11:59:09 PDT
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.
Comment 7 Michael Saboff 2012-04-02 13:18:09 PDT
Reopening to address Darin's comments.
Comment 8 Michael Saboff 2012-04-02 13:35:36 PDT
Committed r112930: <http://trac.webkit.org/changeset/112930>