[webkit-reviews] review denied: [Bug 45186] Use the Windows thread pool instead of an extra thread for FastMalloc scavenging : [Attachment 78976] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Patrick R. Gansterer
<paroga at paroga.com>'s request for review:
Bug 45186: Use the Windows thread pool instead of an extra thread for
FastMalloc scavenging
https://bugs.webkit.org/show_bug.cgi?id=45186

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

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=78976&action=review

> Source/JavaScriptCore/wtf/FastMalloc.cpp:1540
> +ALWAYS_INLINE bool TCMalloc_PageHeap::isScavengerSuspended()
> +{
> +    return m_scavengingSuspended;
> +}
> +ALWAYS_INLINE void TCMalloc_PageHeap::scheduleScavenger()
> +{
> +    m_scavengingSuspended = false;
> +    dispatch_resume(m_scavengeTimer);
> +}
> +
> +ALWAYS_INLINE void TCMalloc_PageHeap::rescheduleScavenger()
> +{
> +    // Nothing to do here for libdispatch.
> +}
> +
> +ALWAYS_INLINE void TCMalloc_PageHeap::suspendScavenger()
> +{
> +    m_scavengingSuspended = true;
> +    dispatch_suspend(m_scavengeTimer);
> +}

These functions (and their Windows equivalents) should probably assert that the
pageheap lock is held.

> Source/JavaScriptCore/wtf/FastMalloc.cpp:1561
> +    CreateTimerQueueTimer(&m_scavengeQueueTimer, 0, scavengerTimerFired, 0,
kScavengeDelayInSeconds * 1000, 0, WT_EXECUTEONLYONCE);

It would be helpful to add a comment here about how rescheduling a
non-repeating timer doesn't work.

> Source/JavaScriptCore/wtf/FastMalloc.cpp:2459
> +    {
> +	   SpinLockHolder h(&pageheap_lock);
> +	   pageheap->scavenge();
> +    }
> +
> +    if (shouldScavenge()) {
> +	   suspendScavenger();
> +	   return;
> +    }
> +
> +    rescheduleScavenger();

This doesn't look right. I think we need to hold pageheap_lock when calling the
various *scavenge* functions.