Bug 98084 - Block freeing thread should sleep indefinitely when there's no work to do
Summary: Block freeing thread should sleep indefinitely when there's no work to do
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
: 90622 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-10-01 15:44 PDT by Mark Hahnenberg
Modified: 2012-10-17 10:30 PDT (History)
4 users (show)

See Also:

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 an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 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.
Comment 1 Mark Hahnenberg 2012-10-01 15:59:24 PDT 
Created attachment 166562 [details]
Patch
Comment 2 Geoffrey Garen 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.
Comment 3 Mark Hahnenberg 2012-10-01 18:29:46 PDT 
Created attachment 166587 [details]
Patch
Comment 4 Geoffrey Garen 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.
Comment 5 Mark Hahnenberg 2012-10-02 14:24:04 PDT 
Committed r130212: <http://trac.webkit.org/changeset/130212>
Comment 6 Mark Hahnenberg 2012-10-02 14:24:59 PDT 
<rdar://problem/12350667>
Comment 7 Yong Li 2012-10-05 08:40:21 PDT 
*** Bug 90622 has been marked as a duplicate of this bug. ***
Comment 8 Mark Hahnenberg 2012-10-16 23:40:46 PDT 
Reopening because this wasn't completely fixed.
Comment 9 Mark Hahnenberg 2012-10-17 09:45:24 PDT 
Created attachment 169199 [details]
Patch
Comment 10 Geoffrey Garen 2012-10-17 10:18:12 PDT 
Comment on attachment 169199 [details]
Patch

r=me
Comment 11 WebKit Review Bot 2012-10-17 10:30:41 PDT 
Comment on attachment 169199 [details]
Patch

Clearing flags on attachment: 169199

Committed r131619: <http://trac.webkit.org/changeset/131619>
Comment 12 WebKit Review Bot 2012-10-17 10:30:44 PDT 
All reviewed patches have been landed.  Closing bug.