Summary: | Opportunistic GC should give up if the Heap is paged out | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Hahnenberg <mhahnenberg> | ||||||
Component: | JavaScriptCore | Assignee: | Mark Hahnenberg <mhahnenberg> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | fpizlo, ggaren | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Mark Hahnenberg
2012-05-02 13:10:37 PDT
Created attachment 139882 [details]
Patch
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. Created attachment 139914 [details]
Patch
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? Committed r115915: <http://trac.webkit.org/changeset/115915> |