Bug 98084

Summary:

Block freeing thread should sleep indefinitely when there's no work to do

Product:

WebKit

Reporter:

Mark Hahnenberg <mhahnenberg>

Component:

JavaScriptCore

Assignee:

Mark Hahnenberg <mhahnenberg>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ggaren, levin+threading, webkit.review.bot, yong.li.webkit

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Mark Hahnenberg

Reported 2012-10-01 15:44:24 PDT

Current the block freeing thread wakes up once a second to check if there are any blocks for it to release back to the OS. This is wasteful. We should change it to sleep when it realizes there are no more blocks to free. Any thread that returns a block to the BlockAllocator should then notify the block freeing thread that there is more work to do now.

Attachments
Patch (3.55 KB, patch) 2012-10-01 15:59 PDT, Mark Hahnenberg	no flags	Details Formatted Diff Diff
Patch (2.87 KB, patch) 2012-10-01 18:29 PDT, Mark Hahnenberg	no flags	Details Formatted Diff Diff
Patch (3.17 KB, patch) 2012-10-17 09:45 PDT, Mark Hahnenberg	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Mark Hahnenberg

Comment 1 2012-10-01 15:59:24 PDT

Created attachment 166562 [details] Patch

Geoffrey Garen

Comment 2 2012-10-01 16:24:59 PDT

Comment on attachment 166562 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166562&action=review This fix looks correct, but I think it can get better. > Source/JavaScriptCore/heap/BlockAllocator.h:111 > + m_numberOfFreeBlocks++; Can we just use m_numberOfFreeBlocks != 0, instead of a separate m_blocksNeedFreeing boolean, to tell if we need to free anything? > Source/JavaScriptCore/heap/BlockAllocator.h:118 > + MutexLocker mutexLocker(m_freeBlockConditionLock); > + if (!m_blocksNeedFreeing) { > + m_blocksNeedFreeing = true; > + m_freeBlockCondition.signal(); > + } Rather than doing this unconditionally, I'd prefer to see this done only if m_numberOfFreeBlocks was 0 when we acquired the lock.

Mark Hahnenberg

Comment 3 2012-10-01 18:29:46 PDT

Created attachment 166587 [details] Patch

Geoffrey Garen

Comment 4 2012-10-01 21:11:55 PDT

Comment on attachment 166587 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166587&action=review r=me > Source/JavaScriptCore/heap/BlockAllocator.h:115 > + MutexLocker mutexLocker(m_freeBlockConditionLock); > + if (!numberOfFreeBlocks) "mutexLocker" should be scoped inside "if (!numberOfFreeBlocks)", so we only acquire the mutex on the transition from zero to non-zero. Please make this change before landing.

Mark Hahnenberg

Comment 5 2012-10-02 14:24:04 PDT

Committed r130212: <http://trac.webkit.org/changeset/130212>

Mark Hahnenberg

Comment 6 2012-10-02 14:24:59 PDT

<rdar://problem/12350667>

Yong Li

Comment 7 2012-10-05 08:40:21 PDT

*** Bug 90622 has been marked as a duplicate of this bug. ***

Mark Hahnenberg

Comment 8 2012-10-16 23:40:46 PDT

Reopening because this wasn't completely fixed.