[webkit-reviews] review denied: [Bug 85411] Opportunistic GC should give up if the Heap is paged out : [Attachment 139882] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 85411: Opportunistic GC should give up if the Heap is paged out
https://bugs.webkit.org/show_bug.cgi?id=85411

Attachment 139882: Patch
https://bugs.webkit.org/attachment.cgi?id=139882&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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.