Bug 45186 – Use the Windows thread pool instead of an extra thread for FastMalloc scavenging

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Adam Roben (:aroben)]

FastMalloc creates a background thread for scavenging free memory.  The thread wakes up every five seconds, scavenges some free memory, then goes back to sleep.  It may be more efficient to schedule this small amount of work on the Windows thread pool, perhaps using a timer-queue timer.  It'd certainly be nicer than having a new thread run for the lifetime of the application just because JavaScriptCore was touched.

[Adele Peterson]

<rdar://problem/8393129>

[Patrick R. Gansterer]

Created attachment 78526 [details]
Patch

[WebKit Review Bot]

Attachment 78526 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/wtf/FastMalloc.cpp:1442:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1444:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1506:  TCMalloc_PageHeap::initializeScavenger is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1550:  TCMalloc_PageHeap::scavengerThread is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:2436:  TCMalloc_PageHeap::runScavenge is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 5 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Roben (:aroben)]

Comment on attachment 78526 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78526&action=review

> Source/JavaScriptCore/wtf/FastMalloc.cpp:1566
> +    while (1) {
> +        if (runScavenge())
> +            continue;
> +
> +        pthread_mutex_lock(&m_scavengeMutex);
> +        m_scavengeThreadActive = false;
> +        // Block until there are enough free committed pages to release back to the system.
> +        pthread_cond_wait(&m_scavengeCondition, &m_scavengeMutex);
> +        m_scavengeThreadActive = true;
> +        pthread_mutex_unlock(&m_scavengeMutex);
> +    }

I think it would be a lot better if we followed the libdispatch implementation. periodicScavenge just performs a single scavenge, then waits for the timer to fire again (or stops the timer if no more scavenging is needed). That way we don't tie up a worker thread while we sleep and block.

[Adam Roben (:aroben)]

Comment on attachment 78526 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78526&action=review

>> Source/JavaScriptCore/wtf/FastMalloc.cpp:1566
>> +    }
> 
> I think it would be a lot better if we followed the libdispatch implementation. periodicScavenge just performs a single scavenge, then waits for the timer to fire again (or stops the timer if no more scavenging is needed). That way we don't tie up a worker thread while we sleep and block.

Patrick pointed out that I was misreading the patch and that this code isn't executing on Windows anymore. However, runScavenge still includes a sleep() call, which I don't think we should be doing on the worker thread. We should use a timer queue timer so that the waiting happens on a thread pool wait thread. Then when the timer fires, we should just scavenge immediately.

[Patrick R. Gansterer]

Created attachment 78538 [details]
Patch

[WebKit Review Bot]

Attachment 78538 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/wtf/FastMalloc.cpp:1442:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1446:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1447:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1448:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1450:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1498:  TCMalloc_PageHeap::initializeScavenger is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1500:  Use 0 instead of NULL.  [readability/null] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1504:  More than one command on the same line  [whitespace/newline] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1504:  Missing space before {  [whitespace/braces] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1524:  TCMalloc_PageHeap::initializeScavenger is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:2408:  TCMalloc_PageHeap::periodicScavenge is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 11 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Roben (:aroben)]

Comment on attachment 78538 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78538&action=review

It's really too bad we can't reuse a previously-paused timer queue timer!

> Source/JavaScriptCore/wtf/FastMalloc.cpp:1536
> +ALWAYS_INLINE void TCMalloc_PageHeap::signalScavenger()
> +{
> +    ASSERT(IsHeld(pageheap_lock));
> +    if (!m_scavengingScheduled && shouldScavenge()) {
> +        m_scavengingScheduled = true;
> +        dispatch_resume(m_scavengeTimer);
> +    }
> +}
> +
> +#elif OS(WINDOWS)
> +
> +static void CALLBACK scavengerTimerFired(void* context, BOOLEAN)
> +{
> +    static_cast<TCMalloc_PageHeap*>(context)->periodicScavenge();
> +}
> +
> +void TCMalloc_PageHeap::initializeScavenger()
> +{
> +    m_scavengeQueueTimer = 0;
> +}
> +
> +ALWAYS_INLINE void TCMalloc_PageHeap::signalScavenger()
> +{
> +    ASSERT(IsHeld(pageheap_lock));
> +    if (!m_scavengeQueueTimer && shouldScavenge()) {
> +        DWORD period = kScavengeDelayInSeconds * 1000;
> +        CreateTimerQueueTimer(&m_scavengeQueueTimer, 0, scavengerTimerFired, 0, period, period, WT_EXECUTEDEFAULT);
> +    }
> +}

It would be nice if we could share a little more of the logic in signalScavenger(). Adding some new functions like isScavengerScheduled() (or its opposite, isScavengerSuspended()) and scheduleScavenger() should make that pretty easy.

> Source/JavaScriptCore/wtf/FastMalloc.cpp:2421
> +#if HAVE(DISPATCH_H)
> +        m_scavengingScheduled = false;
> +        dispatch_suspend(m_scavengeTimer);
> +#elif OS(WINDOWS)
> +        HANDLE scavengeQueueTimer = m_scavengeQueueTimer;
> +        m_scavengeQueueTimer = 0;
> +        DeleteTimerQueueTimer(0, scavengeQueueTimer, 0);
>  #endif

Maybe a suspendScavenger() function would make this cleaner.

[Adam Roben (:aroben)]

Comment on attachment 78538 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78538&action=review

>> Source/JavaScriptCore/wtf/FastMalloc.cpp:1536
>> +}
> 
> It would be nice if we could share a little more of the logic in signalScavenger(). Adding some new functions like isScavengerScheduled() (or its opposite, isScavengerSuspended()) and scheduleScavenger() should make that pretty easy.

I don't think there's anything that prevents m_scavengeQueueTimer from firing again if periodicScavenge is taking a long time. Maybe we should always use a non-repeating timer and delete it every time it fires?

[Patrick R. Gansterer]

Created attachment 78976 [details]
Patch

[WebKit Review Bot]

Attachment 78976 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/FastMalloc.cpp:1450:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1451:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1452:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1453:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1454:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1458:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1459:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1460:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1462:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1510:  TCMalloc_PageHeap::initializeScavenger is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1512:  Use 0 instead of NULL.  [readability/null] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1516:  More than one command on the same line  [whitespace/newline] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1516:  Missing space before {  [whitespace/braces] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1549:  TCMalloc_PageHeap::initializeScavenger is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:2447:  TCMalloc_PageHeap::periodicScavenge is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 15 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Roben (:aroben)]

Comment on attachment 78976 [details]
Patch

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.

[Patrick R. Gansterer]

Created attachment 78983 [details]
Patch

[WebKit Review Bot]

Attachment 78983 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/FastMalloc.cpp:1450:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1451:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1452:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1453:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1454:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1458:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1459:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1460:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1462:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1510:  TCMalloc_PageHeap::initializeScavenger is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1512:  Use 0 instead of NULL.  [readability/null] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1516:  More than one command on the same line  [whitespace/newline] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1516:  Missing space before {  [whitespace/braces] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1552:  TCMalloc_PageHeap::initializeScavenger is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:2455:  TCMalloc_PageHeap::periodicScavenge is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 15 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Roben (:aroben)]

Comment on attachment 78983 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78983&action=review

> Source/JavaScriptCore/wtf/FastMalloc.cpp:1566
> +    // We need to use WT_EXECUTEONLYONCE here and reschedule the timer, because
> +    // Windows will fire the timer event even when the function is already running.

Passing 0 for the repeat duration should be enough to make the timer fire only once. If you pass WT_EXECUTEDEFAULT, are you then able to use ChangeTimerQueueTimer to reuse the same timer? If that still doesn't work, I think it's worth adding a comment in rescheduleScavenger and/or suspendScavenger that explains why we have to delete and recreate the timer.

> Source/JavaScriptCore/wtf/FastMalloc.cpp:2463
> +    if (shouldScavenge()) {
> +        suspendScavenger();
> +        return;
> +    }

I think you're missing a ! in there.

[Patrick R. Gansterer]

Created attachment 78987 [details]
Patch

[WebKit Review Bot]

Attachment 78987 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/FastMalloc.cpp:1450:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1451:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1452:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1453:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1454:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1458:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1459:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1460:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1462:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1510:  TCMalloc_PageHeap::initializeScavenger is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1512:  Use 0 instead of NULL.  [readability/null] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1516:  More than one command on the same line  [whitespace/newline] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1516:  Missing space before {  [whitespace/braces] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1552:  TCMalloc_PageHeap::initializeScavenger is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:2456:  TCMalloc_PageHeap::periodicScavenge is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 15 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 78987 [details]
Patch

Rejecting attachment 78987 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'build'..." exit_code: 2

Last 500 characters of output:
se/testapi.build/Script-14D857B50A469C100032146C.sh


=== BUILD AGGREGATE TARGET All OF PROJECT JavaScriptCore WITH CONFIGURATION Release ===
Check dependencies
** BUILD FAILED **


The following build commands failed:
JavaScriptCore:
	CompileC /mnt/git/webkit-commit-queue/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/x86_64/FastMalloc.o /mnt/git/webkit-commit-queue/Source/JavaScriptCore/wtf/FastMalloc.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://queues.webkit.org/results/7558051

[Patrick R. Gansterer]

Created attachment 78995 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 78995 [details]
Patch

Clearing flags on attachment: 78995

Committed r75819: <http://trac.webkit.org/changeset/75819>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/75819 might have broken Windows Release (Build)

[Jessie Berlin]

(In reply to comment #22)
> http://trac.webkit.org/changeset/75819 might have broken Windows Release (Build)

I think this change might have broken the Windows 7 Release Tests:

http://build.webkit.org/builders/Windows%207%20Release%20%28Tests%29/builds/8236
http://build.webkit.org/builders/Windows%207%20Release%20%28Tests%29/builds/8237

[Adam Roben (:aroben)]

(In reply to comment #23)
> (In reply to comment #22)
> > http://trac.webkit.org/changeset/75819 might have broken Windows Release (Build)
> 
> I think this change might have broken the Windows 7 Release Tests:
> 
> http://build.webkit.org/builders/Windows%207%20Release%20%28Tests%29/builds/8236
> http://build.webkit.org/builders/Windows%207%20Release%20%28Tests%29/builds/8237

It did, but Patrick fixed it in subsequent checkins. Thanks for watching the bots!