WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
Bug 85411
Opportunistic GC should give up if the Heap is paged out
https://bugs.webkit.org/show_bug.cgi?id=85411
Summary
Opportunistic GC should give up if the Heap is paged out
Mark Hahnenberg
Reported
2012-05-02 13:10:37 PDT
Opportunistic GC is punishing us severely in limited memory situations because its assumptions about how much time a collection will take are way out of whack when the Heap has been paged out by the OS. We should add a simple detection function to the Heap that detects if its is paged out. It will do this by iterating each block of both the MarkedSpace and CopiedSpace. If that operation takes longer than a fixed amount of time (e.g. 100ms), the function returns true. This function will only be run prior to an opportunistic collection (i.e. it will not run during our normal allocation-triggered collections).
Attachments
Patch
(10.38 KB, patch)
2012-05-02 14:02 PDT
,
Mark Hahnenberg
no flags
Details
Formatted Diff
Diff
Patch
(12.82 KB, patch)
2012-05-02 16:44 PDT
,
Mark Hahnenberg
fpizlo
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Mark Hahnenberg
Comment 1
2012-05-02 14:02:39 PDT
Created
attachment 139882
[details]
Patch
Geoffrey Garen
Comment 2
2012-05-02 16:06:34 PDT
Comment on
attachment 139882
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=139882&action=review
Looks good, but needs refinement. Please file a bug about the incremental sweep issue you mentioned, and the "calling didAllocate" issue you mentioned in person.
> Source/JavaScriptCore/ChangeLog:14 > + and CopiedSpace. If that operation takes longer than a fixed amount of time (e.g. 100ms), > + the function returns true. This function will only be run prior to an opportunistic > + collection (i.e. it will not run during our normal allocation-triggered collections).
Bug # or it didn't happen ;).
> Source/JavaScriptCore/heap/CopiedSpace.cpp:291 > + if (itersSinceLastTimeCheck >= 16) {
16 should be a static const at the top of the file.
> Source/JavaScriptCore/heap/CopiedSpace.cpp:293 > + if (currentTime > timeOut)
timeOut implies an amount of time, not a deadline in absolute time. How about using the word "deadline"?
> Source/JavaScriptCore/heap/MarkedAllocator.cpp:17 > + if (itersSinceLastTimeCheck >= 16) {
Shared constant, please.
> Source/JavaScriptCore/runtime/GCActivityCallbackCF.cpp:68 > + double startTime = WTF::monotonicallyIncreasingTime(); > + if (heap->isPagedOut(startTime + pagingTimeOut)) {
This should only happen on systems that page, so !PLATFORM(IOS).
> Source/JavaScriptCore/runtime/GCActivityCallbackCF.cpp:69 > + heap->activityCallback()->willCollect();
I think it would be better just to cancel the timer, rather than calling this API duplicitously.
Mark Hahnenberg
Comment 3
2012-05-02 16:44:00 PDT
Created
attachment 139914
[details]
Patch
Filip Pizlo
Comment 4
2012-05-02 17:00:07 PDT
Comment on
attachment 139914
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=139914&action=review
R=me with one fix for that comment.
> Source/JavaScriptCore/heap/CopiedSpace.cpp:295 > + if (itersSinceLastTimeCheck >= Heap::s_timeCheckResolution) { > + double currentTime = WTF::monotonicallyIncreasingTime(); > + if (currentTime > deadline) > + return true; > + }
Shouldn't this be setting itersSinceLastTimeCheck to 0 if we don't return true?
Mark Hahnenberg
Comment 5
2012-05-02 17:13:14 PDT
Committed
r115915
: <
http://trac.webkit.org/changeset/115915
>
Mark Hahnenberg
Comment 6
2012-05-03 15:37:34 PDT
<
rdar://problem/11294420
>
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