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
Patch (12.82 KB, patch)
2012-05-02 16:44 PDT, Mark Hahnenberg
fpizlo: review+
Mark Hahnenberg
Comment 1 2012-05-02 14:02:39 PDT
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
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
Mark Hahnenberg
Comment 6 2012-05-03 15:37:34 PDT
Note You need to log in before you can comment on or make changes to this bug.