WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Michael Saboff
Comment 1
2012-03-29 17:05:41 PDT
Created
attachment 134696
[details]
Patch
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
Committed
r112910
: <
http://trac.webkit.org/changeset/112910
>
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
Committed
r112930
: <
http://trac.webkit.org/changeset/112930
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug