RESOLVED FIXED 82674
WebKit should throttle memory pressure notifications in proportion to handler time
https://bugs.webkit.org/show_bug.cgi?id=82674
Summary WebKit should throttle memory pressure notifications in proportion to handler...
Michael Saboff
Reported 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>
Attachments
Patch (3.49 KB, patch)
2012-03-29 17:05 PDT, Michael Saboff
ggaren: review+
Michael Saboff
Comment 1 2012-03-29 17:05:41 PDT
Geoffrey Garen
Comment 2 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.
Alexey Proskuryakov
Comment 3 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.
Michael Saboff
Comment 4 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.
Michael Saboff
Comment 5 2012-04-02 11:15:09 PDT
Darin Adler
Comment 6 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.
Michael Saboff
Comment 7 2012-04-02 13:18:09 PDT
Reopening to address Darin's comments.
Michael Saboff
Comment 8 2012-04-02 13:35:36 PDT
Note You need to log in before you can comment on or make changes to this bug.